Issue Details (XML | Word | Printable)

Key: MDL-19039
Type: Bug Bug
Status: Open Open
Priority: Critical Critical
Assignee: Petr Skoda
Reporter: Tim Hunt
Votes: 0
Watchers: 6
Operations

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

get_users_by_capability sometimes load all users from DB when in the system context

Created: 01/May/09 06:25 PM   Updated: 06/Oct/09 01:28 AM
Return to search
Component/s: Roles
Affects Version/s: 1.9.4
Fix Version/s: 2.0

Participants: Martin Dougiamas, Petr Skoda and Tim Hunt
Security Level: None
Affected Branches: MOODLE_19_STABLE
Fixed Branches: MOODLE_20_STABLE


 Description  « Hide
Sam reported in MDL-19038:

Tim - the function get_u_b_c doesn't actually return all half-million users - but when you make query at CONTEXT_SYSTEM level like this, it really DOES retrieve all half-million users into php, then it filters them. This is about line 4670 in accesslib. Code below.

I think your lazy generation patch is correct (although it would be nice if somebody could make get_u_b_c faster for this case, but that's likely to be an awful lot harder to figure out).

{code}
    if ($context->contextlevel == CONTEXT_SYSTEM
        || $isfrontpage
        || $defaultroleinteresting) {

        // Handle system / sitecourse / defaultrole-with-perhaps-neg-overrides
        // with a SELECT FROM user LEFT OUTER JOIN against ra -
        // This is expensive on the SQL and PHP sides -
        // moves a ton of data across the wire.
        $ss = "SELECT u.id as userid, ra.roleid,
                      ctx.depth
               FROM {$CFG->prefix}user u
               LEFT OUTER JOIN {$CFG->prefix}role_assignments ra
                 ON (ra.userid = u.id
                     AND ra.contextid IN ($ctxids)
                     AND ra.roleid IN (".implode(',',$roleids) .")
                     $condhiddenra)
               LEFT OUTER JOIN {$CFG->prefix}context ctx
                 ON ra.contextid=ctx.id
               WHERE u.deleted=0";
{code}

Clearly this is silly. The CONTEXT_SYSTEM case must be easy to do in pure SQL.

Note that there are quite extensive unit tests for get_users_by_capability in HEAD. If you plan to work on this, you probably should use them to reduce the chance of regressions.

 All   Comments   Change History   Version Control      Sort Order: Ascending order - Click to sort in descending order
Petr Skoda committed 2 files to 'Moodle CVS' on branch 'MOODLE_19_STABLE' - 06/Oct/09 01:06 AM
MDL-19039 improved default frontpage role capability evealuation, needs to be manually enabled in config.php
MODIFY lib/accesslib.php   Rev. 1.421.2.107    (+27 -5 lines)
MODIFY config-dist.php   Rev. 1.103.2.8    (+8 -2 lines)
Petr Skoda committed 2 files to 'Moodle CVS' - 06/Oct/09 01:08 AM
MDL-19039 improved default frontpage role capability evealuation, needs to be manually enabled in config.php
MODIFY lib/accesslib.php   Rev. 1.616    (+25 -3 lines)
MODIFY config-dist.php   Rev. 1.121    (+7 -1 lines)