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

          Issue Links

            Activity

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

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

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

            Please use git to find relevant commits, such as:

            $ git log --grep=MDL-34864
            

            Show
            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: