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
    • Rank:
      6347

      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).

        Issue Links

          Activity

          Hide
          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
          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
          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
          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
          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
          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
          Aparup Banerjee added a comment - go a https://github.com/nebgor/moodle/compare/mistress...MDL-23939_m20 for peer review
          Hide
          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
          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
          Aparup Banerjee added a comment -

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

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

          Hi Apu,

          Just tested your patch and it works well.

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

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

          Show
          Aparup Banerjee added a comment - all tested fine. Sam's review picked up another query that is now replaced with caching.
          Hide
          Petr Škoda 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
          Petr Škoda 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
          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
          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
          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
          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
          Sam Hemelryk added a comment -

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

          Show
          Sam Hemelryk added a comment - Ahh should add I didn't actually test it, just reviewed the code
          Hide
          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
          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
          Sam Hemelryk added a comment -

          Cool thanks Apu, gets my +1

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

          thanks PULL-241 created.

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

          happy QAing

          Show
          Aparup Banerjee added a comment - happy QAing
          Hide
          Petr Škoda added a comment -

          Reopening, please see the details in pull request.

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

          Thanks for the review Sam, created PULL-256

          Show
          Aparup Banerjee added a comment - Thanks for the review Sam, created PULL-256
          Hide
          Petr Škoda added a comment -

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

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

          Fix included in latest 2.0.1+ weekly. Thanks everyone.

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