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

Slow query on course_display in get_complete_user_data()

    Details

    • Database:
      Any
    • Difficulty:
      Moderate
    • Affected Branches:
      MOODLE_19_STABLE, MOODLE_20_STABLE
    • Fixed Branches:
      MOODLE_20_STABLE

      Description

      In lib/moodlelib.php in the get_complete_user_data() function around line 3777 below the /// Get various settings and preferences comment there's a call to $DB->get_records('course_display', array('userid'=>$user->id)

      This function gets called every time a new session starts. On OpenLearn, there are many guest sessions started frequently, so this particular query gets called a lot. There's no index on course_display for just userid, though there is one for course,userid.

      This query is appearing rather too often in my slow queries log, so I was wondering whether adding an index just for user would help? I could do this just as an OU customisation, but I guess it must affect some other big sites as well, and since its in Moodle 1.9 and Moodle 2.0 perhaps this is a good time to get the change into core? Happy to code it myself if someone would just give me a thumbs up?

      (Note all these code references are for Moodle 2.0 but the same problem exists in Moodle 1.9 which is where I've actually seen the problem on my live system).

        Gliffy Diagrams

          Issue Links

            Activity

            Hide
            stronk7 Eloy Lafuente (stronk7) added a comment -

            Wow,

            what a waste of session information. No matter of the index, IMO it is crazy to keep all that information in the session as far as it scales really bad and it is used only by course main page.

            I'd propose to:

            1) ban that information from session completely
            2) Add one get_field('course_display', 'display', array('user' => XX, 'course'=>YY) somewhere in course/index.php (or another page/header... init code), adding it to USER/PAGE/COURSE or so.
            3) Use it everywhere (mainly course formats and done)

            Adding Petr here as far as seems to be a nice improvement, specially for sites with hundreds of courses and also about session-size contention.

            Thanks for the report. Ciao

            Show
            stronk7 Eloy Lafuente (stronk7) added a comment - Wow, what a waste of session information. No matter of the index, IMO it is crazy to keep all that information in the session as far as it scales really bad and it is used only by course main page. I'd propose to: 1) ban that information from session completely 2) Add one get_field('course_display', 'display', array('user' => XX, 'course'=>YY) somewhere in course/index.php (or another page/header... init code), adding it to USER/PAGE/COURSE or so. 3) Use it everywhere (mainly course formats and done) Adding Petr here as far as seems to be a nice improvement, specially for sites with hundreds of courses and also about session-size contention. Thanks for the report. Ciao
            Hide
            stronk7 Eloy Lafuente (stronk7) added a comment -

            Sending this to stable backlog and raising to critical hoping to get it included in next performance-focussed sprint.

            Petr, any comment about the best way to address this? Is the plan above valid?

            TIA and ciao

            Show
            stronk7 Eloy Lafuente (stronk7) added a comment - Sending this to stable backlog and raising to critical hoping to get it included in next performance-focussed sprint. Petr, any comment about the best way to address this? Is the plan above valid? TIA and ciao
            Hide
            nebgor Aparup Banerjee added a comment -

            E1oy, just having a skim through this. so you're saying instead of loading this info upon session creation we load it based on the course/index.php visit? and caching it in the relevant global to be reused everywhere.
            so all references to that info inside session data would have to be refactored?

            Show
            nebgor Aparup Banerjee added a comment - E1oy, just having a skim through this. so you're saying instead of loading this info upon session creation we load it based on the course/index.php visit? and caching it in the relevant global to be reused everywhere. so all references to that info inside session data would have to be refactored?
            Show
            nebgor Aparup Banerjee added a comment - go a https://github.com/nebgor/moodle/compare/mistress...MDL-23939_m20 for peer review
            Hide
            nebgor Aparup Banerjee added a comment -

            Just noting that course_display info will be changed to not load the data into session during login by fix in MDL-23939

            Show
            nebgor Aparup Banerjee added a comment - Just noting that course_display info will be changed to not load the data into session during login by fix in MDL-23939
            Hide
            nebgor Aparup Banerjee added a comment -

            thanks for testing Rossi!
            pull created on PULL-168 for 2.0.x

            Show
            nebgor Aparup Banerjee added a comment - thanks for testing Rossi! pull created on PULL-168 for 2.0.x
            Hide
            rwijaya Rossiani Wijaya added a comment -

            Hi Apu,

            Just tested your patch and it works well.

            Show
            rwijaya Rossiani Wijaya added a comment - Hi Apu, Just tested your patch and it works well.
            Hide
            nebgor Aparup Banerjee added a comment -

            all tested fine. Sam's review picked up another query that is now replaced with caching.

            Show
            nebgor Aparup Banerjee added a comment - all tested fine. Sam's review picked up another query that is now replaced with caching.
            Hide
            skodak Petr Skoda added a comment - - edited

            Reopening, please review the possibility of
            1/ removing all 0 from the table because they do not seem to be necessary (smaller table === faster queries)
            2/ do not try to load this info for the guest account

            I like the current proposed patch because it also makes the session smaller.

            Eloy just proposed to keep in session only the last accessed course - this could help with performance a lot because we definitely need to make the session data smaller.

            Petr

            Show
            skodak Petr Skoda added a comment - - edited Reopening, please review the possibility of 1/ removing all 0 from the table because they do not seem to be necessary (smaller table === faster queries) 2/ do not try to load this info for the guest account I like the current proposed patch because it also makes the session smaller. Eloy just proposed to keep in session only the last accessed course - this could help with performance a lot because we definitely need to make the session data smaller. Petr
            Hide
            nebgor Aparup Banerjee added a comment -

            ok, rebased and added on more changes along the lines of suggestions given.. please have a look, anyone. (Rossi might be unavailable to do my peer reviews.)

            https://github.com/nebgor/moodle/compare/mistress...MDL-23939_m20

            Show
            nebgor Aparup Banerjee added a comment - ok, rebased and added on more changes along the lines of suggestions given.. please have a look, anyone. (Rossi might be unavailable to do my peer reviews.) https://github.com/nebgor/moodle/compare/mistress...MDL-23939_m20
            Hide
            samhemelryk Sam Hemelryk added a comment -

            Hi Apu,

            I've read through the changes and made the following notes:

            course/lib.php

            • Line 1254~1257: What is the deal with $swap? Why lose all the display settings for other courses?
            • Line 1272,1273,1276: White space
            • Line 2866: Clean up double bracketing

            The rest looks good.

            Cheers
            Sam

            Show
            samhemelryk Sam Hemelryk added a comment - Hi Apu, I've read through the changes and made the following notes: course/lib.php Line 1254~1257: What is the deal with $swap? Why lose all the display settings for other courses? Line 1272,1273,1276: White space Line 2866: Clean up double bracketing The rest looks good. Cheers Sam
            Hide
            samhemelryk Sam Hemelryk added a comment -

            Ahh should add I didn't actually test it, just reviewed the code

            Show
            samhemelryk Sam Hemelryk added a comment - Ahh should add I didn't actually test it, just reviewed the code
            Hide
            nebgor Aparup Banerjee added a comment -

            all gd, thanks for the code review, i've cleaned up spaces and pushed stuff up again. losing other courses display setting is as per Eloy's suggestion (via Petr's comment above)

            Show
            nebgor Aparup Banerjee added a comment - all gd, thanks for the code review, i've cleaned up spaces and pushed stuff up again. losing other courses display setting is as per Eloy's suggestion (via Petr's comment above)
            Hide
            samhemelryk Sam Hemelryk added a comment -

            Cool thanks Apu, gets my +1

            Show
            samhemelryk Sam Hemelryk added a comment - Cool thanks Apu, gets my +1
            Hide
            nebgor Aparup Banerjee added a comment -

            thanks PULL-241 created.

            Show
            nebgor Aparup Banerjee added a comment - thanks PULL-241 created.
            Hide
            nebgor Aparup Banerjee added a comment -

            happy QAing

            Show
            nebgor Aparup Banerjee added a comment - happy QAing
            Hide
            skodak Petr Skoda added a comment -

            Reopening, please see the details in pull request.

            Show
            skodak Petr Skoda added a comment - Reopening, please see the details in pull request.
            Hide
            nebgor Aparup Banerjee added a comment -
            Show
            nebgor Aparup Banerjee added a comment - new changes in https://github.com/nebgor/moodle/compare/mistress...MDL-23939_m20 ready for peer review.
            Hide
            nebgor Aparup Banerjee added a comment -

            Thanks for the review Sam, created PULL-256

            Show
            nebgor Aparup Banerjee added a comment - Thanks for the review Sam, created PULL-256
            Hide
            skodak Petr Skoda added a comment -

            I have improved the patch a bit and filed new PULL-286 request, thanks!

            Show
            skodak Petr Skoda added a comment - I have improved the patch a bit and filed new PULL-286 request, thanks!
            Hide
            tsala Helen Foster added a comment -

            Fix included in latest 2.0.1+ weekly. Thanks everyone.

            Show
            tsala Helen Foster added a comment - Fix included in latest 2.0.1+ weekly. Thanks everyone.

              People

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

                Dates

                • Created:
                  Updated:
                  Resolved:
                  Fix Release Date:
                  21/Feb/11