Moodle
  1. Moodle
  2. MDL-34864

Improve category enrolment performance

    Details

    • Rank:
      43385

      Description

      On a large site with a significant number of category enrolled users, the following query will attempt to run multiple times over, eventually causing performance problems on the database.

      SELECT e.*, ue.userid
                    FROM mdl_enrol e
                    JOIN mdl_context ctx ON (ctx.instanceid = e.courseid AND ctx.contextlevel = '50')
                    JOIN mdl_user_enrolments ue ON (ue.enrolid = e.id)
               LEFT JOIN (SELECT DISTINCT cctx.path, ra.userid
                            FROM mdl_course_categories cc
                            JOIN mdl_context cctx ON (cctx.instanceid = cc.id AND cctx.contextlevel = '40')
                            JOIN mdl_role_assignments ra ON (ra.contextid = cctx.id AND ra.roleid IN ('5','9','11'))
                         ) cat ON (ctx.path LIKE CONCAT(cat.path, '/%') AND cat.userid = ue.userid)
                   WHERE e.enrol = 'category' AND cat.userid IS NULL
      

      Please optimize this query.

        Issue Links

          Activity

          Hide
          Petr Škoda added a comment -

          Which database are you using and what version? If you are using PostgreSQL please try Moodle 2.3 because there is a new index on the category.path field. Also please make sure to test the latest db version (9.1.x for pg and 5.5.x for mysql), recent version have a lot better subquery optimisations.

          The category enrolment was designed mostly for backwards compatibility for sites upgraded from 1.9. I would recommended to use cohorts instead if you need something that performs better. I am afraid I do not know any simple way to improve this query on all supported databases, sorry.

          Show
          Petr Škoda added a comment - Which database are you using and what version? If you are using PostgreSQL please try Moodle 2.3 because there is a new index on the category.path field. Also please make sure to test the latest db version (9.1.x for pg and 5.5.x for mysql), recent version have a lot better subquery optimisations. The category enrolment was designed mostly for backwards compatibility for sites upgraded from 1.9. I would recommended to use cohorts instead if you need something that performs better. I am afraid I do not know any simple way to improve this query on all supported databases, sorry.
          Hide
          Petr Škoda added a comment -

          Hmm, it seems we could take out the DISTINCT and change the subquery to nested joins.

          Show
          Petr Škoda added a comment - Hmm, it seems we could take out the DISTINCT and change the subquery to nested joins.
          Hide
          Mike Churchward added a comment -

          We are running MySQL 5.0.77 and are not able to upgrade our servers to MySQL 5.5 at this time. That is within the recommended minimum version of 5.0.25 though.

          Unfortunately we are seeing the problem with many client sites, and getting them all to switch to cohorts is not realistic in the short term.

          With removal of the subquery, what are you considering? I can get resources to try this and test it out.

          Show
          Mike Churchward added a comment - We are running MySQL 5.0.77 and are not able to upgrade our servers to MySQL 5.5 at this time. That is within the recommended minimum version of 5.0.25 though. Unfortunately we are seeing the problem with many client sites, and getting them all to switch to cohorts is not realistic in the short term. With removal of the subquery, what are you considering? I can get resources to try this and test it out.
          Hide
          Petr Škoda added a comment -

          Here is an untested patch https://github.com/skodak/moodle/compare/master...wip_MDL-34864_m24_enrolcat

          In any case MySQL 5.0.x is ancient, you should consider upgrading to something more up-to-date because 2.3 requires 5.1.x and this might not be backported to 2.2.

          Show
          Petr Škoda added a comment - Here is an untested patch https://github.com/skodak/moodle/compare/master...wip_MDL-34864_m24_enrolcat In any case MySQL 5.0.x is ancient, you should consider upgrading to something more up-to-date because 2.3 requires 5.1.x and this might not be backported to 2.2.
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Integrated, thanks! (23 & master)

          Show
          Eloy Lafuente (stronk7) added a comment - Integrated, thanks! (23 & master)
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Boom! this is causing 23_STABLE tests to fail:

          enrol_category_testcase::test_sync_full
          Argument 2 passed to role_fix_names() must be an instance of context, null given, called in 
          /enrol/category/locallib.php on line 289 and defined
          /lib/accesslib.php:4203
          /enrol/category/locallib.php:289
          /enrol/category/tests/sync_test.php:312
          /lib/phpunit/classes/advanced_testcase.php:76
          

          Ciao

          Show
          Eloy Lafuente (stronk7) added a comment - Boom! this is causing 23_STABLE tests to fail: enrol_category_testcase::test_sync_full Argument 2 passed to role_fix_names() must be an instance of context, null given, called in /enrol/category/locallib.php on line 289 and defined /lib/accesslib.php:4203 /enrol/category/locallib.php:289 /enrol/category/tests/sync_test.php:312 /lib/phpunit/classes/advanced_testcase.php:76 Ciao
          Hide
          Dan Poltawski added a comment -

          (I'll take this for testing)

          Show
          Dan Poltawski added a comment - (I'll take this for testing)
          Hide
          Dan Poltawski added a comment -

          Setting to failed

          Show
          Dan Poltawski added a comment - Setting to failed
          Hide
          Frédéric Massart added a comment -

          I reproduced this testing error while testing MDL-34912 on both master and 2.3.

          Show
          Frédéric Massart added a comment - I reproduced this testing error while testing MDL-34912 on both master and 2.3.
          Hide
          Dan Poltawski added a comment -

          Fred, on master??

          Show
          Dan Poltawski added a comment - Fred, on master??
          Hide
          Frédéric Massart added a comment -

          No, just realised it was another one... My bad!

          Show
          Frédéric Massart added a comment - No, just realised it was another one... My bad!
          Hide
          Petr Škoda added a comment -

          Commit with fix pushed to my 23 branch, please merge it. Sorry for the trouble, I must have executed the test twice in master checkout, but at least this shows that tests are really important...

          Show
          Petr Škoda added a comment - Commit with fix pushed to my 23 branch, please merge it. Sorry for the trouble, I must have executed the test twice in master checkout, but at least this shows that tests are really important...
          Hide
          Dan Poltawski added a comment -

          Pushed fix.

          Show
          Dan Poltawski added a comment - Pushed fix.
          Hide
          Dan Poltawski added a comment -

          Looks good now, passing.

          Show
          Dan Poltawski added a comment - Looks good now, passing.
          Hide
          Dan Poltawski added a comment -

          Ah, no. I need to do it on all the dbs!

          Show
          Dan Poltawski added a comment - Ah, no. I need to do it on all the dbs!
          Hide
          Eloy Lafuente (stronk7) added a comment -

          YEAR!*

          CAF*, TOT!*

          • Your effort amazingly resulted. (unbelievable :-P)
          • Closing as fixed.
          • Tons of thanks.
          Show
          Eloy Lafuente (stronk7) added a comment - YEAR!* CAF*, TOT!* Your effort amazingly resulted. (unbelievable :-P) Closing as fixed. Tons of thanks.
          Hide
          Mike Churchward added a comment - - edited

          Where can I pull this fix from? This really needs to be backported to at least 2.2. (the link to the patch that Petr pointed out no longer works)

          Show
          Mike Churchward added a comment - - edited Where can I pull this fix from? This really needs to be backported to at least 2.2. (the link to the patch that Petr pointed out no longer works)
          Hide
          Petr Škoda added a comment -

          Please use git to find relevant commits, such as:

          $ git log --grep=MDL-34864
          
          Show
          Petr Škoda added a comment - Please use git to find relevant commits, such as: $ git log --grep=MDL-34864

            People

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

              Dates

              • Created:
                Updated:
                Resolved: