Issue Details (XML | Word | Printable)

Key: MDL-8785
Type: Bug Bug
Status: Resolved Resolved
Resolution: Fixed
Priority: Blocker Blocker
Assignee: Martin Dougiamas
Reporter: Martin Dougiamas
Votes: 0
Watchers: 2
Operations

Add/Edit UI Mockup to this issue
If you were logged in you would be able to see more operations.
Moodle

Remove timestart and timeend from consideration by load_user_capability()

Created: 07/Mar/07 12:15 PM   Updated: 30/Jan/08 04:20 PM
Return to search
Component/s: Enrolments, Roles
Affects Version/s: 1.7, 1.7.1, 1.7.2, 1.8
Fix Version/s: 1.7.2, 1.8

Issue Links:
Relates

Participants: Eloy Lafuente (stronk7), Martin Dougiamas and Martín Langhoff
Security Level: None
Resolved date: 07/Mar/07
Affected Branches: MOODLE_17_STABLE, MOODLE_18_STABLE
Fixed Branches: MOODLE_17_STABLE, MOODLE_18_STABLE


 Description  « Hide
Eloy has correctly identified timestart and timeend as a big database bottleneck in load_user_capability because they prevent caching and deal with inequalities, so let's get rid if them!

These times specify the duration that the enrolment is active. Currently they are only used by self-enrolling courses where the course has a duration. In these instance timestart is set to now and timeend is set to some time in the future. Zero in timestart means inifinite past, and in timeend means inifinite future.

We were checking these to cope with all cases where people might want to set enrolment durations starting and ending in the future, but this is actually not very useful. Let's let the enrolment plugins handle such things (adding role assignments on the fly where necessary).

So instead, I propose:

1) We remove the time checks completely from load_user_capability and assume all role assigments in the database are valid.

2) We add a cron check to delete any role assignments where (timeend > 0 AND timeend < $now)

3) We make sure this is documented well in the code and on the wiki.

 All   Comments   Change History   Version Control      Sort Order: Ascending order - Click to sort in descending order
Martín Langhoff added a comment - 07/Mar/07 01:06 PM
Quick notes:
  • this is great for MySQL, but of medium importance for Pg - I get pretty good stats for the big union query
  • the plan there is ok for removing only timeend - but there's a better approach for 1.8/1.9: having a "current" bool column, and selecting where current = 1 - cheaper than the timestart check

Martin Dougiamas added a comment - 07/Mar/07 02:06 PM
Done in 1.7.x 1.8.x and head. I'm seeing good performance increases :-D

Martin Dougiamas added a comment - 07/Mar/07 02:08 PM
The flag is a nice idea, it makes the table more generic again +1 for 1.9

Eloy Lafuente (stronk7) added a comment - 07/Mar/07 05:40 PM
Cool!

One minor note. There are some lines that have been added both to 1.8 and HEAD that aren't needed now that timestart and timeend are out from the query:

/// Always round timestart downto 100 secs to help DBs to use their own caching algorithms
2112 /// by repeating queries with the same exact parameters in a 100 secs time window
2113 $newra->timestart = round($timestart, -2);

those were part of my initial patch but, IMO now completely un-needed.

Ciao