Uploaded image for project: 'Moodle'
  1. Moodle
  2. MDL-19039

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

    Details

    • Type: Bug
    • Status: Closed
    • Priority: Critical
    • Resolution: Fixed
    • Affects Version/s: 1.9.4
    • Fix Version/s: 2.0
    • Component/s: Roles / Access
    • Labels:
      None
    • Affected Branches:
      MOODLE_19_STABLE
    • Fixed Branches:
      MOODLE_20_STABLE

      Description

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

          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";

      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.

        Gliffy Diagrams

          Attachments

            Activity

              People

              • Votes:
                0 Vote for this issue
                Watchers:
                6 Start watching this issue

                Dates

                • Created:
                  Updated:
                  Resolved:
                  Fix Release Date:
                  24/Nov/10