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

Improve category enrolment performance

    Details

      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.

        Gliffy Diagrams

          Attachments

            Issue Links

              Activity

              Hide
              skodak Petr Skoda 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
              skodak Petr Skoda 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
              skodak Petr Skoda added a comment -

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

              Show
              skodak Petr Skoda added a comment - Hmm, it seems we could take out the DISTINCT and change the subquery to nested joins.
              Hide
              mchurch 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
              mchurch 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
              skodak Petr Skoda 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
              skodak Petr Skoda 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
              stronk7 Eloy Lafuente (stronk7) added a comment -

              Integrated, thanks! (23 & master)

              Show
              stronk7 Eloy Lafuente (stronk7) added a comment - Integrated, thanks! (23 & master)
              Hide
              stronk7 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
              stronk7 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
              poltawski Dan Poltawski added a comment -

              (I'll take this for testing)

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

              Setting to failed

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

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

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

              Fred, on master??

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

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

              Show
              fred Frédéric Massart added a comment - No, just realised it was another one... My bad!
              Hide
              skodak Petr Skoda 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
              skodak Petr Skoda 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
              poltawski Dan Poltawski added a comment -

              Pushed fix.

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

              Looks good now, passing.

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

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

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

              YEAR!*

              CAF*, TOT!*

              • Your effort amazingly resulted. (unbelievable :-P)
              • Closing as fixed.
              • Tons of thanks.
              Show
              stronk7 Eloy Lafuente (stronk7) added a comment - YEAR!* CAF*, TOT!* Your effort amazingly resulted. (unbelievable :-P) Closing as fixed. Tons of thanks.
              Hide
              mchurch 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
              mchurch 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
              skodak Petr Skoda added a comment -

              Please use git to find relevant commits, such as:

              $ git log --grep=MDL-34864

              Show
              skodak Petr Skoda 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:
                    Fix Release Date:
                    10/Sep/12