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

          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