Moodle

admin_setting_users_with_capability sometimes load all uusers in admin/settings/courses.php

Details

  • Type: Bug Bug
  • Status: Closed Closed
  • Priority: Minor Minor
  • Resolution: Fixed
  • Affects Version/s: 1.9.4
  • Fix Version/s: 1.9.5
  • Component/s: Administration
  • Labels:
    None
  • Affected Branches:
    MOODLE_19_STABLE
  • Fixed Branches:
    MOODLE_19_STABLE

Description

Reported by Sam in Moodle HQ chat:

with moodle 1.9.4 we see a critical performance problem on the root /index.php (or anywhere that draws the site admin block). We've tracked this down to a usage in courses.php of admin_setting_users_with_capability which ends up doing a pretty horrible get_users_with_capability that returns all 500,000 users. This line of code is still there in current MOODLE_19, but I wonder if it's been fixed in any way - does anyone know?

(unfortunately our moodle 1.9 test installs don't have large user numbers)

(ie basically, is this our system being slow, or is there something wrong with our settings for the capability in question, moodle/site:approvecourse)

Activity

Ascending order - Click to sort in descending order
Hide
Permalink
Tim Hunt added a comment -

sam,

that code should be doing
get_users_by_capability(get_context_instance(CONTEXT_SYSTEM), 'moodle/site:approvecourse', ...)
which should be a very short list of users.

(Can you check that it is doing that, and that it is returning a plausible number of users on learn.)

However, the admin_setting_configselect superclass has a separate load_choices option so that the choices can be lazy-loaded when needed, rather than having to be set in the constructor, and that is definitely a better way to do things here.

I think the attached patch correctly implements that, but I have only tested it very briefly. Would be good if you could review/test it some more, and even merge to 2.0 and commit it if you like. (I don't want to get too distracted from blocklib.)

Show
Tim Hunt added a comment - sam, that code should be doing get_users_by_capability(get_context_instance(CONTEXT_SYSTEM), 'moodle/site:approvecourse', ...) which should be a very short list of users. (Can you check that it is doing that, and that it is returning a plausible number of users on learn.) However, the admin_setting_configselect superclass has a separate load_choices option so that the choices can be lazy-loaded when needed, rather than having to be set in the constructor, and that is definitely a better way to do things here. I think the attached patch correctly implements that, but I have only tested it very briefly. Would be good if you could review/test it some more, and even merge to 2.0 and commit it if you like. (I don't want to get too distracted from blocklib.)
Hide
Permalink
Sam Marshall added a comment - - edited

Rod has investigated this.

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).

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";
Show
Sam Marshall added a comment - - edited Rod has investigated this. 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). 
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";
Hide
Permalink
Tim Hunt added a comment -

Marking this issue fixed, since I have committed the lazy-load thing.

I created MDL-19039 for gubc suckyness.

Show
Tim Hunt added a comment - Marking this issue fixed, since I have committed the lazy-load thing. I created MDL-19039 for gubc suckyness.
Hide
Permalink
Petr Škoda (skodak) added a comment -

thanks Tim

Show
Petr Škoda (skodak) added a comment - thanks Tim

People

Vote (0)
Watch (1)

Dates

  • Created:
    Updated:
    Resolved:
Atlassian JIRA (v4.4.3#663-r165197) | Report a problem
Powered by a free Atlassian JIRA open source license for Moodle. Try JIRA - bug tracking software for your team.