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

Course category tree on frontpage costs a db query per course

    Details

    • Type: Bug
    • Status: Open
    • Priority: Minor
    • Resolution: Unresolved
    • Affects Version/s: 2.1.1, 2.7
    • Fix Version/s: BACKEND
    • Component/s: Performance
    • Labels:

      Description

      If the course category tree is used it costs a db query per course, this come from lots of enrol queries generated from the call to:

      enrol_get_course_info_icons()

      in course/renderer.php function course_category_tree_category()

      This means that page doesn't scale well if the site has thousands of courses

        Gliffy Diagrams

          Attachments

            Issue Links

              Activity

              poltawski Dan Poltawski created issue -
              salvetore Michael de Raadt made changes -
              Field Original Value New Value
              Assignee moodle.com [ moodle.com ] Sam Marshall [ quen ]
              Component/s Navigation [ 10599 ]
              Hide
              salvetore Michael de Raadt added a comment -

              Sorry, I clicked on the wrong Sam.

              Show
              salvetore Michael de Raadt added a comment - Sorry, I clicked on the wrong Sam.
              salvetore Michael de Raadt made changes -
              Assignee Sam Marshall [ quen ] Sam Hemelryk [ samhemelryk ]
              Hide
              samhemelryk Sam Hemelryk added a comment -

              Marking this triaged, confirmed it will in deed still be bad for performance.
              Perhaps a great candidate for MUC.

              Many thanks
              Sam

              Show
              samhemelryk Sam Hemelryk added a comment - Marking this triaged, confirmed it will in deed still be bad for performance. Perhaps a great candidate for MUC. Many thanks Sam
              samhemelryk Sam Hemelryk made changes -
              Fix Version/s STABLE backlog [ 10463 ]
              Labels triaged
              Assignee Sam Hemelryk [ samhemelryk ] moodle.com [ moodle.com ]
              Component/s Navigation [ 10599 ]
              Hide
              samhemelryk Sam Hemelryk added a comment -

              Linking to the MUC stage 2 issue as this is probably low hanging fruit.

              Show
              samhemelryk Sam Hemelryk added a comment - Linking to the MUC stage 2 issue as this is probably low hanging fruit.
              samhemelryk Sam Hemelryk made changes -
              Link This issue has a non-specific relationship to MDL-34224 [ MDL-34224 ]
              samhemelryk Sam Hemelryk made changes -
              Assignee moodle.com [ moodle.com ] Sam Hemelryk [ samhemelryk ]
              Hide
              samhemelryk Sam Hemelryk added a comment -

              Putting this up for peer-review.
              The solution I am purposing simply introduces a cache to store the icons for a course (as an array of objects).
              It is pretty simple + safe and if we hit the cache we avoid the need to include enrollment plugins and save ourselves 1 query per course.

              Show
              samhemelryk Sam Hemelryk added a comment - Putting this up for peer-review. The solution I am purposing simply introduces a cache to store the icons for a course (as an array of objects). It is pretty simple + safe and if we hit the cache we avoid the need to include enrollment plugins and save ourselves 1 query per course.
              samhemelryk Sam Hemelryk made changes -
              Status Open [ 1 ] Waiting for peer review [ 10012 ]
              Pull Master Diff URL https://github.com/samhemelryk/moodle/compare/master...wip-MDL-29594-m25
              Pull Master Branch wip-MDL-29594-m25
              Pull from Repository git://github.com/samhemelryk/moodle.git
              Hide
              samhemelryk Sam Hemelryk added a comment -

              Hi Petr,

              I know you are busy but you know the enrolment code better than anyone.
              Would you mind having a look at the cache solution I am purposing and seeing what you think.

              Many thanks
              Sam

              Show
              samhemelryk Sam Hemelryk added a comment - Hi Petr, I know you are busy but you know the enrolment code better than anyone. Would you mind having a look at the cache solution I am purposing and seeing what you think. Many thanks Sam
              Hide
              poltawski Dan Poltawski added a comment -

              I had a look at this and it looks nice I don't know enough about other enrollment things to say confidently that we don't need to invlidate elsewhere.

              Show
              poltawski Dan Poltawski added a comment - I had a look at this and it looks nice I don't know enough about other enrollment things to say confidently that we don't need to invlidate elsewhere.
              Hide
              skodak Petr Skoda added a comment -

              This looks like a big change of API to me, previously everybody could see a different set of API (enrolled x not enrolled, completed, enrolment period open/closed, etc.) after this change the icons would be frozen.

              Maybe there could be created a general enrol instance caching instead because there are several other places that fetch data from enrol table.

              My -1 for current patch, sorry.

              Show
              skodak Petr Skoda added a comment - This looks like a big change of API to me, previously everybody could see a different set of API (enrolled x not enrolled, completed, enrolment period open/closed, etc.) after this change the icons would be frozen. Maybe there could be created a general enrol instance caching instead because there are several other places that fetch data from enrol table. My -1 for current patch, sorry.
              samhemelryk Sam Hemelryk made changes -
              Status Waiting for peer review [ 10012 ] Development in progress [ 3 ]
              Hide
              samhemelryk Sam Hemelryk added a comment -

              Thanks for looking fella's.

              Re: enrollment state + statuses, I was concerned that may be the case.
              I'd had a quick scan through the code and couldn't find anywhere those were being factored in with the icons but I figured they'd be somewhere.
              Not to mention I suppose we can't be sure that the plugins aren't delivering user specific icons as well I suppose.

              No probs, this was a quick knock up. I'll invest more time and look to identify a more general cache.

              Many thanks
              Sam

              Show
              samhemelryk Sam Hemelryk added a comment - Thanks for looking fella's. Re: enrollment state + statuses, I was concerned that may be the case. I'd had a quick scan through the code and couldn't find anywhere those were being factored in with the icons but I figured they'd be somewhere. Not to mention I suppose we can't be sure that the plugins aren't delivering user specific icons as well I suppose. No probs, this was a quick knock up. I'll invest more time and look to identify a more general cache. Many thanks Sam
              Hide
              skodak Petr Skoda added a comment -

              Thanks.

              Show
              skodak Petr Skoda added a comment - Thanks.
              Hide
              samhemelryk Sam Hemelryk added a comment -

              Ok I've a much simpler cache now that simply caches the course enrolment instances that would otherwise be retrieved from the database.

              Show
              samhemelryk Sam Hemelryk added a comment - Ok I've a much simpler cache now that simply caches the course enrolment instances that would otherwise be retrieved from the database.
              samhemelryk Sam Hemelryk made changes -
              Status Development in progress [ 3 ] Waiting for peer review [ 10012 ]
              Pull Master Diff URL https://github.com/samhemelryk/moodle/compare/master...wip-MDL-29594-m25 https://github.com/samhemelryk/moodle/compare/master...wip-MDL-29594-m25-r2
              Pull Master Branch wip-MDL-29594-m25 wip-MDL-29594-m25-r2
              Hide
              samhemelryk Sam Hemelryk added a comment -

              Ready for peer-review once more

              Show
              samhemelryk Sam Hemelryk added a comment - Ready for peer-review once more
              Hide
              skodak Petr Skoda added a comment -

              I am afraid this would not work because at present plugins modify the enrol table directly - see enrol/manual/edit.php for example. We would have to create new update_instance() method in enrol plugins and enforce its usage. I do not think we can do this change in stable branches because it might lead to regressions.

              I also do not understand the persistentmaxsize=2.

              The rest looks ok, I think there might be some more places that could use this. This might even allow me to add more flexibility later, I intentionally simplified some other code to prevent repeated fetching from database.

              Thanks for working on this.

              Show
              skodak Petr Skoda added a comment - I am afraid this would not work because at present plugins modify the enrol table directly - see enrol/manual/edit.php for example. We would have to create new update_instance() method in enrol plugins and enforce its usage. I do not think we can do this change in stable branches because it might lead to regressions. I also do not understand the persistentmaxsize=2. The rest looks ok, I think there might be some more places that could use this. This might even allow me to add more flexibility later, I intentionally simplified some other code to prevent repeated fetching from database. Thanks for working on this.
              Hide
              andyjdavis Andrew Davis added a comment -

              This is not a proper review but simply a reminder to add some proper testing instructions.

              Show
              andyjdavis Andrew Davis added a comment - This is not a proper review but simply a reminder to add some proper testing instructions.
              fred Frédéric Massart made changes -
              Original Estimate 0 minutes [ 0 ]
              Remaining Estimate 0 minutes [ 0 ]
              Status Waiting for peer review [ 10012 ] Peer review in progress [ 10013 ]
              Peer reviewer fred
              Hide
              fred Frédéric Massart added a comment -

              Putting back in development as it requires more work.

              Show
              fred Frédéric Massart added a comment - Putting back in development as it requires more work.
              fred Frédéric Massart made changes -
              Status Peer review in progress [ 10013 ] Development in progress [ 3 ]
              fred Frédéric Massart made changes -
              Peer reviewer fred
              samhemelryk Sam Hemelryk made changes -
              Link This issue is blocked by MDL-45508 [ MDL-45508 ]
              Hide
              samhemelryk Sam Hemelryk added a comment -

              Linking to MDL-45508 which has been created to see an API created for enrol instance interaction.

              Show
              samhemelryk Sam Hemelryk added a comment - Linking to MDL-45508 which has been created to see an API created for enrol instance interaction.
              samhemelryk Sam Hemelryk made changes -
              Status Development in progress [ 3 ] Open [ 1 ]
              samhemelryk Sam Hemelryk made changes -
              Affects Version/s 2.7 [ 12850 ]
              Hide
              samhemelryk Sam Hemelryk added a comment -

              Stopping development of this until the blocker is resolved.

              Show
              samhemelryk Sam Hemelryk added a comment - Stopping development of this until the blocker is resolved.
              Hide
              samhemelryk Sam Hemelryk added a comment -

              I am un-assigning myself from this issue as I am not currently working on this and it will give the opportunity for someone else to work on it.

              Show
              samhemelryk Sam Hemelryk added a comment - I am un-assigning myself from this issue as I am not currently working on this and it will give the opportunity for someone else to work on it.
              samhemelryk Sam Hemelryk made changes -
              Fix Version/s BACKEND [ 12582 ]
              Fix Version/s STABLE backlog [ 10463 ]
              Labels triaged patch triaged
              Assignee Sam Hemelryk [ samhemelryk ]

                People

                • Votes:
                  2 Vote for this issue
                  Watchers:
                  5 Start watching this issue

                  Dates

                  • Created:
                    Updated: