Moodle
  1. Moodle
  2. MDL-30509

Unwanted roles query slows down cron

    Details

    • Type: Improvement Improvement
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: 2.1, 2.2
    • Fix Version/s: 2.1.5, 2.2.2
    • Component/s: Cohorts
    • Labels:
    • Database:
      MySQL
    • Testing Instructions:
      Hide

      1/ enable cohort enrol plugin
      2/ create a script that executes enrol_cohort_sync(); from require_once("$CFG->dirroot/enrol/cohort/locallib.php");
      3/ no errors expected

      It should not be necessary to test the complete enrol plugin because there are know problems that were only resolved in master.

      Show
      1/ enable cohort enrol plugin 2/ create a script that executes enrol_cohort_sync(); from require_once("$CFG->dirroot/enrol/cohort/locallib.php"); 3/ no errors expected It should not be necessary to test the complete enrol plugin because there are know problems that were only resolved in master.
    • Workaround:
      Hide

      This is the query I've re-written which works much faster in MySQL (and as far as I can tell, gives the same result):

              SELECT ra.roleid, ra.userid, ra.contextid, ra.itemid
              FROM {role_assignments} ra
              JOIN {context} c ON ( c.id = ra.contextid AND c.contextlevel = :coursecontext $onecourse )
              WHERE ra.component = 'enrol_cohort'
              AND NOT EXISTS (
                  SELECT e.id
                  FROM {user_enrolments} ue
                  JOIN {enrol} e ON ( e.id = ue.enrolid AND e.enrol = 'cohort' )
                  WHERE e.id = ra.itemid
                  AND roleid = ra.roleid
                  AND ue.userid = ra.userid
              )
      
      Show
      This is the query I've re-written which works much faster in MySQL (and as far as I can tell, gives the same result): SELECT ra.roleid, ra.userid, ra.contextid, ra.itemid FROM {role_assignments} ra JOIN {context} c ON ( c.id = ra.contextid AND c.contextlevel = :coursecontext $onecourse ) WHERE ra.component = 'enrol_cohort' AND NOT EXISTS ( SELECT e.id FROM {user_enrolments} ue JOIN {enrol} e ON ( e.id = ue.enrolid AND e.enrol = 'cohort' ) WHERE e.id = ra.itemid AND roleid = ra.roleid AND ue.userid = ra.userid )
    • Affected Branches:
      MOODLE_21_STABLE, MOODLE_22_STABLE
    • Fixed Branches:
      MOODLE_21_STABLE, MOODLE_22_STABLE
    • Pull from Repository:
    • Pull Master Branch:
      w04_MDL-30509_m22_slowcohort
    • Rank:
      33203

      Description

      A site that has thousands of users enrolled by cron synch has flagged a significant delay in the cron with the query to "remove unwanted roles" in enrol/cohort/locallib.php function enrol_cohort_sync()

      It does admit to "take a long time" during upgrade, but it does on every cron execution, and I think it can be a lot faster (less than 1/10th the time according to my testing in MySQL) by rewriting the outer join as a WHERE NOT EXISTS clause.

        Issue Links

          Activity

          Hide
          Michael de Raadt added a comment -

          Thanks for suggesting that.

          Show
          Michael de Raadt added a comment - Thanks for suggesting that.
          Hide
          Petr Škoda added a comment -

          hello, which version of MySQL are you using? Is it faster in both 5.1 and 5.5? Could this be resolved by the new optimiser in mariaDB 5.3?

          Show
          Petr Škoda added a comment - hello, which version of MySQL are you using? Is it faster in both 5.1 and 5.5? Could this be resolved by the new optimiser in mariaDB 5.3?
          Hide
          Yair Spielmann added a comment -

          It is faster in both 5.1 and 5.5. We don't have mariaDB installed - it looks worth trying though. Thanks

          Show
          Yair Spielmann added a comment - It is faster in both 5.1 and 5.5. We don't have mariaDB installed - it looks worth trying though. Thanks
          Hide
          Petr Škoda added a comment -

          Hello, I have ended up rewriting the cohort sync code completely, the result is in MDL-30944. Could you please test performance of the new code? It is using simpler LEFT JOIN, it seems to perform fine on my small test server with a few thousands of courses + users. Thanks.

          Show
          Petr Škoda added a comment - Hello, I have ended up rewriting the cohort sync code completely, the result is in MDL-30944 . Could you please test performance of the new code? It is using simpler LEFT JOIN, it seems to perform fine on my small test server with a few thousands of courses + users. Thanks.
          Hide
          Yair Spielmann added a comment -

          Thanks - this is indeed a significant improvement.

          The unwanted roles query now averages 12% the time of the original version I reported.

          The bottleneck query is now "now assign all necessary roles to enrolled users", which averaged 29% of the original "unwanted roles" query.

          I think this should be enough, especially with the cron running less frequently. I can test this with the site that flagged the issue, but this may take a while as we would need to upgrade the site to the required Moodle version.

          Show
          Yair Spielmann added a comment - Thanks - this is indeed a significant improvement. The unwanted roles query now averages 12% the time of the original version I reported. The bottleneck query is now "now assign all necessary roles to enrolled users", which averaged 29% of the original "unwanted roles" query. I think this should be enough, especially with the cron running less frequently. I can test this with the site that flagged the issue, but this may take a while as we would need to upgrade the site to the required Moodle version.
          Hide
          Petr Škoda added a comment -

          Thanks, let's see how the integration+testing of MDL-30944 goes, then we can use your patch in older stable branches.

          Show
          Petr Škoda added a comment - Thanks, let's see how the integration+testing of MDL-30944 goes, then we can use your patch in older stable branches.
          Hide
          Petr Škoda added a comment -

          Hello, I have backported part of the patch from master and submitted this for integration, thanks a lot for your report and proposed patch.

          Show
          Petr Škoda added a comment - Hello, I have backported part of the patch from master and submitted this for integration, thanks a lot for your report and proposed patch.
          Hide
          Sam Hemelryk added a comment -

          Hi Petr,
          Should this be cherry-picked to MOODLE_21_STABLE as well, or is this strictly a 22 improvement (given master already has this)?

          Cheers
          Sam

          Show
          Sam Hemelryk added a comment - Hi Petr, Should this be cherry-picked to MOODLE_21_STABLE as well, or is this strictly a 22 improvement (given master already has this)? Cheers Sam
          Hide
          Sam Hemelryk added a comment -

          Just spotted from the calendar you are away at the moment Petr.

          Eloy I've added you as a watcher to this, would you mind having a peak at it and seeing what you think, should it be backported to 21.
          Gets a +1 from me to backport to 21.

          Cheers
          Sam

          Show
          Sam Hemelryk added a comment - Just spotted from the calendar you are away at the moment Petr. Eloy I've added you as a watcher to this, would you mind having a peak at it and seeing what you think, should it be backported to 21. Gets a +1 from me to backport to 21. Cheers Sam
          Hide
          Eloy Lafuente (stronk7) added a comment -

          1+1=2 (if cleanly applies)

          Show
          Eloy Lafuente (stronk7) added a comment - 1+1=2 (if cleanly applies)
          Hide
          Sam Hemelryk added a comment -

          Discussed with Eloy and backported to 21 as well.
          This has been integrated now

          Show
          Sam Hemelryk added a comment - Discussed with Eloy and backported to 21 as well. This has been integrated now
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Passing, I've executed the enrol_cohort_sync() method with DB enabled and the query has been executed and the method ended without any visible notice/warn/error.

          Show
          Eloy Lafuente (stronk7) added a comment - Passing, I've executed the enrol_cohort_sync() method with DB enabled and the query has been executed and the method ended without any visible notice/warn/error.
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Your nice code represents only 1/46 of the issues that have been sent upstream this week, so thanks, but not many.

          Nah, joking, many thanks! Closing this a fixed, ciao

          Show
          Eloy Lafuente (stronk7) added a comment - Your nice code represents only 1/46 of the issues that have been sent upstream this week, so thanks, but not many. Nah, joking, many thanks! Closing this a fixed, ciao

            People

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

              Dates

              • Created:
                Updated:
                Resolved: