Moodle
  1. Moodle
  2. MDL-35127

Inefficient use of expensive SQL in assign_print_overview

    Details

    • Rank:
      43756

      Description

      After updating to a version for 2.3.x, we began seeing large DB loads from two SQL queries in assign_print_overview.

      The intent of the queries is to cache data for use in the following loop, saving load, but the queries are used for every page view, even when they are not needed.

      One in particular is used only for people with the mod/assign:grade permission, which in most instances is a fraction of the userbase.

        Activity

        Hide
        Eric Merrill added a comment -

        The two queries build up a cache, regardless of if they are going to be used. The mod/assign:grade cache is particularly harmful, because it is only used by a fraction of users.

        In my patch, I move both queries into the loop, and fill them only if needed. They are still only executed once, but are filled when required, instead of on all calls to the function. The additional load is one if(isset()) check per loop, which I think is more than paid for by the decreased database load.

        Show
        Eric Merrill added a comment - The two queries build up a cache, regardless of if they are going to be used. The mod/assign:grade cache is particularly harmful, because it is only used by a fraction of users. In my patch, I move both queries into the loop, and fill them only if needed. They are still only executed once, but are filled when required, instead of on all calls to the function. The additional load is one if(isset()) check per loop, which I think is more than paid for by the decreased database load.
        Hide
        Eric Merrill added a comment -

        Since it has been about 2 weeks, this is just a bump. Daymon, can you take a look at this when you get a change.

        Show
        Eric Merrill added a comment - Since it has been about 2 weeks, this is just a bump. Daymon, can you take a look at this when you get a change.
        Hide
        Damyon Wiese added a comment -

        Happy with the change Eric, but it needs rebasing as it conflicts with an oracle fix that was added recently.

        "a.nosubmissions AS offline" was changed to "a.nosubmissions AS nosubmissions" because offline is a keyword in oracle.

        This requires a change to the if statement further down:

        • if ($submission->offline) {
          + if ($submission->nosubmissions) {

        If you can rebase this it looks ready for integration.

        Thanks!

        Show
        Damyon Wiese added a comment - Happy with the change Eric, but it needs rebasing as it conflicts with an oracle fix that was added recently. "a.nosubmissions AS offline" was changed to "a.nosubmissions AS nosubmissions" because offline is a keyword in oracle. This requires a change to the if statement further down: if ($submission->offline) { + if ($submission->nosubmissions) { If you can rebase this it looks ready for integration. Thanks!
        Hide
        Eric Merrill added a comment -

        I see that change is currently in integration. As soon as the weeklies roll, I update this.

        Thanks

        Show
        Eric Merrill added a comment - I see that change is currently in integration. As soon as the weeklies roll, I update this. Thanks
        Hide
        Eric Merrill added a comment -

        I've updated the commits for this, so you can push this for integration.

        I also note that I can't seem to make 'Submissions not graded' happen, even without this patch, I'm not sure if there has been a regression somewhere since I first did this. I'm still looking into that problem, but for now I'm going to update the testing instructions for this one.

        Show
        Eric Merrill added a comment - I've updated the commits for this, so you can push this for integration. I also note that I can't seem to make 'Submissions not graded' happen, even without this patch, I'm not sure if there has been a regression somewhere since I first did this. I'm still looking into that problem, but for now I'm going to update the testing instructions for this one.
        Hide
        Eric Merrill added a comment -

        Damyon,

        I've updated all the branches, can you send this for integration?

        Thanks
        -Eric

        Show
        Eric Merrill added a comment - Damyon, I've updated all the branches, can you send this for integration? Thanks -Eric
        Hide
        Damyon Wiese added a comment -

        Thanks Erik,

        I am pushing this for integration now - I just added another patch to declare the variables (as null) outside the foreach loop for readability.

        This is covered by unit tests on the master branch only.

        Show
        Damyon Wiese added a comment - Thanks Erik, I am pushing this for integration now - I just added another patch to declare the variables (as null) outside the foreach loop for readability. This is covered by unit tests on the master branch only.
        Hide
        Eloy Lafuente (stronk7) added a comment -

        The main moodle.git repository has just been updated with latest weekly modifications. You may wish to rebase your PULL branches to simplify history and avoid any possible merge conflicts. This would also make integrator's life easier next week.

        TIA and ciao

        Show
        Eloy Lafuente (stronk7) added a comment - The main moodle.git repository has just been updated with latest weekly modifications. You may wish to rebase your PULL branches to simplify history and avoid any possible merge conflicts. This would also make integrator's life easier next week. TIA and ciao
        Hide
        Sam Hemelryk added a comment -

        Thanks guys, this has been integrated now.

        Show
        Sam Hemelryk added a comment - Thanks guys, this has been integrated now.
        Hide
        Andrew Davis added a comment -

        Works as described. Passing.

        Show
        Andrew Davis added a comment - Works as described. Passing.
        Hide
        Eloy Lafuente (stronk7) added a comment -

        This is valid for unlimited entries to the, soon to be unveiled, Moodle Codebase Gardens. It includes free access to all facilities.

        Personal and non-transferable to all assignees, reviewers and testers in this issue. Valid until switching to Blackboard (100000€ penalization will be applied).

        Thanks, closing as fixed!

        Show
        Eloy Lafuente (stronk7) added a comment - This is valid for unlimited entries to the, soon to be unveiled, Moodle Codebase Gardens. It includes free access to all facilities. Personal and non-transferable to all assignees, reviewers and testers in this issue. Valid until switching to Blackboard (100000€ penalization will be applied). Thanks, closing as fixed!

          People

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

            Dates

            • Created:
              Updated:
              Resolved: