Moodle
  1. Moodle
  2. MDL-35127

Inefficient use of expensive SQL in assign_print_overview

    Details

    • Testing Instructions:
      Hide
      1. You will need a course with a student and a teacher.
      2. Create an assignment with a due date in the future.
      3. Login and student and view MyMoodle page. Verify that the assignment is present below the course.
      4. As the student, submit to the assignment.
      5. Login as teacher, verify that the assignment, is present below the course (the format may be different in each branch).
      Show
      You will need a course with a student and a teacher. Create an assignment with a due date in the future. Login and student and view MyMoodle page. Verify that the assignment is present below the course. As the student, submit to the assignment. Login as teacher, verify that the assignment, is present below the course (the format may be different in each branch).
    • Affected Branches:
      MOODLE_23_STABLE
    • Fixed Branches:
      MOODLE_23_STABLE, MOODLE_24_STABLE
    • Pull from Repository:
    • Pull Master Branch:
      MDL-35127-master

      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.

        Gliffy Diagrams

          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: