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

Unwanted roles query slows down cron

    Details

    • Type: Improvement
    • Status: Closed
    • Priority: 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

      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.

        Gliffy Diagrams

          Issue Links

            Activity

            Hide
            salvetore Michael de Raadt added a comment -

            Thanks for suggesting that.

            Show
            salvetore Michael de Raadt added a comment - Thanks for suggesting that.
            Hide
            skodak Petr Skoda 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
            skodak Petr Skoda 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
            yairspielmann 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
            yairspielmann 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
            skodak Petr Skoda 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
            skodak Petr Skoda 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
            yairspielmann 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
            yairspielmann 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
            skodak Petr Skoda 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
            skodak Petr Skoda 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
            skodak Petr Skoda 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
            skodak Petr Skoda 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
            samhemelryk 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
            samhemelryk 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
            samhemelryk 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
            samhemelryk 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
            stronk7 Eloy Lafuente (stronk7) added a comment -

            1+1=2 (if cleanly applies)

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

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

            Show
            samhemelryk Sam Hemelryk added a comment - Discussed with Eloy and backported to 21 as well. This has been integrated now
            Hide
            stronk7 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
            stronk7 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
            stronk7 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
            stronk7 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:
                3 Start watching this issue

                Dates

                • Created:
                  Updated:
                  Resolved:
                  Fix Release Date:
                  12/Mar/12