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

get_users_by_capability is inefficient with groups

    XMLWordPrintable

    Details

    • Type: Bug
    • Status: Closed
    • Priority: Minor
    • Resolution: Fixed
    • Affects Version/s: 3.3.4, 3.4.1, 3.4.6, 3.5.2, 3.6, 3.7
    • Fix Version/s: 3.5.5, 3.6.3
    • Component/s: Libraries
    • Labels:
    • Testing Instructions:
      Hide

      Very hard to know how to test this specifically. get_users_by_capability is used in many cases in the code. This change should bring no changes in behaviour, just changes in performance.

      If you wish to test manually, in addition to what the PHPunit and Behat tests already cover, I suggest:

      1. Start with a version of Moodle without this patch.
      2. Load several pages that depend on get_users_by_capability. (See below)
      3. Now apply the patch.
      4. Open the same Moodle pages, in new browser tabs.
      5. For each page you loaded, switch between the before and after patch browser tabs, to make sure the two pages are identical.

      Some pages that most obviously depend on get_users_by_capability:

      Note, the cases most affected by this patch is when you are in separate groups mode, and a group is selected.

      Show
      Very hard to know how to test this specifically. get_users_by_capability is used in many cases in the code. This change should bring no changes in behaviour, just changes in performance. If you wish to test manually, in addition to what the PHPunit and Behat tests already cover, I suggest: Start with a version of Moodle without this patch. Load several pages that depend on get_users_by_capability. (See below) Now apply the patch. Open the same Moodle pages, in new browser tabs. For each page you loaded, switch between the before and after patch browser tabs, to make sure the two pages are identical. Some pages that most obviously depend on get_users_by_capability: SCORM reports. Survey reports. Quiz add User override (e.g. https://qa.moodle.net/mod/quiz/overrideedit.php?action=adduser&cmid=30 ) list of users in the drop-down. [Not helpful here, but https://moodle.org/plugins/mod_forumng relies on it, which is how we found the problem!] Note, the cases most affected by this patch is when you are in separate groups mode, and a group is selected.
    • Affected Branches:
      MOODLE_33_STABLE, MOODLE_34_STABLE, MOODLE_35_STABLE, MOODLE_36_STABLE, MOODLE_37_STABLE
    • Fixed Branches:
      MOODLE_35_STABLE, MOODLE_36_STABLE
    • Pull from Repository:
    • Pull 3.5 Branch:
    • Pull 3.6 Branch:
    • Pull Master Branch:

      Description

      We detected this monitoring our live servers. The query from a call like

      $groups = groups_get_all_groups($this->get_course()->id, 0, $grouping);
      $users = get_users_by_capability($context,
              'mod/forumng:viewdiscussion', 'u.id', 'u.lastaccess DESC', '', 200000,
              array_keys($groups), '', 0, 0, true)
      

      is running in ~7 seconds. A simple re-write of the SQL can bring that down to ~0.2 seconds. This is on our Postgres server. However, I cannot yet see how to do thatrewrite in genereral in the get_users_by_capability PHP code. I am just making this issue to log my findings.

      The SQL that Moodle generates looks like:

      SELECT u.id
      FROM mdl_user u
      JOIN (SELECT DISTINCT userid FROM (
              /* Subquery. wich may be a UNION, to do the has-capability check */
                  ) ra ON ra.userid = u.id
       
       
      WHERE u.deleted = 0 AND u.id <> 1
      /* This is the problem: group membership test done as a Subquery */
      AND (u.id IN (
      				SELECT userid
      				FROM mdl_groups_members gm
      				WHERE gm.groupid IN (/* List of group ids */)
          ) OR u.id IN (/* List of access all groups user ids */)
      )
      ORDER BY u.lastaccess DESC LIMIT 200000
      

      Changing the group-membership test to a JOIN is much faster, however, it requires an extra DISTINCT (or it could be a GROUP BY) which does not interact well with the default ORDER BY.

      SELECT DISTINCT u.id, u.lastaccess
      FROM mdl_user u
      JOIN (SELECT DISTINCT userid FROM (
              /* Subquery. wich may be a UNION, to do the has-capability check */
                  ) ra ON ra.userid = u.id
      /* Now a JOIN, with simplified WHERE, but note the change to the first line above. */
      LEFT JOIN mdl_groups_members gm ON gm.userid = u.id
       
      WHERE u.deleted = 0 AND u.id <> 1
      AND (gm.groupid IN (/* List of group ids */)
          ) OR u.id IN (/* List of access all groups user ids */)
      )
      ORDER BY u.lastaccess DESC LIMIT 200000
      

        Attachments

          Activity

            People

            • Assignee:
              timhunt Tim Hunt
              Reporter:
              timhunt Tim Hunt
              Peer reviewer:
              Andrew Nicols
              Integrator:
              Eloy Lafuente (stronk7)
              Tester:
              CiBoT
              Participants:
              Component watchers:
              Amaia Anabitarte, Carlos Escobedo, Ferran Recio, Sara Arjona (@sarjona), Víctor Déniz Falcón
            • Votes:
              0 Vote for this issue
              Watchers:
              3 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:
                Fix Release Date:
                11/Mar/19