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

Query slowness on grade_items lookup

    Details

    • Testing Instructions:
      Hide

      Validate existing grades

      1. Before patch is applied:
        1. Create or re-use an existing course that has multiple grade items and categories.
        2. Ensure that at least one of the grade categories has aggregatesubcats enabled, and at least one has aggregatesubcats disabled
        3. Ensure that at least one of the grade categories has aggregateoutcomes enabled, and at least one has aggregateoutcomes disabled
        4. Ensure that there are grades present for multiple students
        5. Take screenshots of the gradebook for future verification
      2. After the patch is applied
        1. Visit the gradebook. Ensure the results are consistent before and after.

      Validate grading

      1. In one of the graded activities, enter a grade for a student
      2. In one of the graded activities, edit a grade for a student
      3. Force a regrade (e.g. move a grade item into a new category)
      4. Validate that the changes took effect and are properly aggregated in the gradebook
      Show
      Validate existing grades Before patch is applied: Create or re-use an existing course that has multiple grade items and categories. Ensure that at least one of the grade categories has aggregatesubcats enabled, and at least one has aggregatesubcats disabled Ensure that at least one of the grade categories has aggregateoutcomes enabled, and at least one has aggregateoutcomes disabled Ensure that there are grades present for multiple students Take screenshots of the gradebook for future verification After the patch is applied Visit the gradebook. Ensure the results are consistent before and after. Validate grading In one of the graded activities, enter a grade for a student In one of the graded activities, edit a grade for a student Force a regrade (e.g. move a grade item into a new category) Validate that the changes took effect and are properly aggregated in the gradebook
    • Affected Branches:
      MOODLE_24_STABLE, MOODLE_25_STABLE
    • Fixed Branches:
      MOODLE_24_STABLE, MOODLE_25_STABLE
    • Pull Master Branch:
      MDL-42065_master

      Description

      We are noticing significant slowness on a site that has a large number of total grade items (in some cases > 1,000 grade items in multiple courses). Both queries could be optimized to include the grade_category or grade_item courseid (both indexed).

      Query 1

      SELECT gi.id
      FROM mdl_grade_items gi
      WHERE (gi.gradetype = '1' OR gi.gradetype = '2')
      AND gi.outcomeid IS NULL
      AND gi.categoryid IN (
          SELECT gc.id
          FROM mdl_grade_categories gc
          WHERE gc.path LIKE '%/9820/%'
      )
      

      Query 2

      SELECT gi.id
      FROM mdl_grade_items gi
      WHERE (gi.gradetype = ? OR gi.gradetype = ?) 
      AND gi.categoryid = ? 
      AND gi.outcomeid IS NULL 
       
      UNION
       
      SELECT gi.id FROM mdl_grade_items gi, mdl_grade_categories gc 
      WHERE (gi.itemtype = ? OR gi.itemtype = ?) 
      AND gi.iteminstance=gc.id
      AND gc.parent = ? 
      AND (gi.gradetype = ? OR gi.gradetype = ?) 
      AND gi.outcomeid IS NULL
      

        Gliffy Diagrams

          Activity

          Hide
          sbc24 Sam Chaffee added a comment -

          Adding pull branches and diff urls for a potential fix for 2.4, 2.5, and master.

          Show
          sbc24 Sam Chaffee added a comment - Adding pull branches and diff urls for a potential fix for 2.4, 2.5, and master.
          Hide
          salvetore Michael de Raadt added a comment -

          Thanks for reporting that and sharing a solution.

          Show
          salvetore Michael de Raadt added a comment - Thanks for reporting that and sharing a solution.
          Hide
          salvetore Michael de Raadt added a comment -

          Sam: Feel free to put this up for peer review when you thin it's ready.

          Show
          salvetore Michael de Raadt added a comment - Sam: Feel free to put this up for peer review when you thin it's ready.
          Hide
          kstokking Kris Stokking added a comment -

          Thanks for tagging this ticket Michael. I'm not sure why the priority has been demoted, but to give some context these non-optimized queries account for > 50% of all database traffic for this particular site. Grading is crippled - it takes on average over 30 seconds for each student. It would be great to expedite a fix.

          Show
          kstokking Kris Stokking added a comment - Thanks for tagging this ticket Michael. I'm not sure why the priority has been demoted, but to give some context these non-optimized queries account for > 50% of all database traffic for this particular site. Grading is crippled - it takes on average over 30 seconds for each student. It would be great to expedite a fix.
          Hide
          fred Frédéric Massart added a comment -

          Hi Sam,

          thanks for working on this. Here are a few suggestions:

          1. It seems that the code becomes less readable due to the usage of ? for SQL parameters, perhaps that would be a good time to switch to named parameters. That is not blocking your issue though.
          2. I think we really need Unit Tests for this, there is one in place, and it passes, but I am not sure if it is handles aggregatesubcats or not, it definitely should.
          3. You could state the component name in your commit message. http://docs.moodle.org/dev/Commit_cheat_sheet#Provide_clear_commit_messages
          4. This needs some good testing instructions covering the different queries modified.
          5. To optimise a bit more, the line performing the query with get_records_sql() could be changed to a get_fieldset_sql() to avoid the usage of array_keys() for no apparent reason.

          Many thanks!
          Fred

          Show
          fred Frédéric Massart added a comment - Hi Sam, thanks for working on this. Here are a few suggestions: It seems that the code becomes less readable due to the usage of ? for SQL parameters, perhaps that would be a good time to switch to named parameters. That is not blocking your issue though. I think we really need Unit Tests for this, there is one in place, and it passes, but I am not sure if it is handles aggregatesubcats or not, it definitely should. You could state the component name in your commit message. http://docs.moodle.org/dev/Commit_cheat_sheet#Provide_clear_commit_messages This needs some good testing instructions covering the different queries modified. To optimise a bit more, the line performing the query with get_records_sql() could be changed to a get_fieldset_sql() to avoid the usage of array_keys() for no apparent reason. Many thanks! Fred
          Hide
          kstokking Kris Stokking added a comment -

          Hi Fred,

          2. I would completely agree with you, but I am also a bit concerned that this will be a major undertaking. Who will be taking on the effort? In the past, contributors of bug fixes have not been responsible for creating missing unit tests.
          4. We're working with live data on a course that has hundreds of grade items. Do you have a way to simulate that with data creation scripts? We tested that the functionality of quiz and assignment grading, the user grader report, and the course grader report were all consistent prior to and after the change. So far so good, but I can't say whether that list is exhaustive (does it need to be?)

          In the end, I think we need to be reasonable - the change in the query does 2 things:

          1. It changes the way grade item categoryid's are matched from using an IN clause to an INNER JOIN on the grade category table. This does not change the behavior of the query, but does allow MySQL Query Optimizer to optimize it better.
          2. We've added courseid's so that the query does not issue a full table scan, and can take advantage of the fact that both the grade_category and grade_items tables have indexes on courseid.

          The query should return the exact same result set in all cases as long as grade items/categories live within the same course (which they must by design, but I can verify that across all Joule sites anyway). So what we have is an optimization - the new query should run much faster than the original version. Does that change your requirements?

          Show
          kstokking Kris Stokking added a comment - Hi Fred, 2. I would completely agree with you, but I am also a bit concerned that this will be a major undertaking. Who will be taking on the effort? In the past, contributors of bug fixes have not been responsible for creating missing unit tests. 4. We're working with live data on a course that has hundreds of grade items. Do you have a way to simulate that with data creation scripts? We tested that the functionality of quiz and assignment grading, the user grader report, and the course grader report were all consistent prior to and after the change. So far so good, but I can't say whether that list is exhaustive (does it need to be?) In the end, I think we need to be reasonable - the change in the query does 2 things: 1. It changes the way grade item categoryid's are matched from using an IN clause to an INNER JOIN on the grade category table. This does not change the behavior of the query, but does allow MySQL Query Optimizer to optimize it better. 2. We've added courseid's so that the query does not issue a full table scan, and can take advantage of the fact that both the grade_category and grade_items tables have indexes on courseid. The query should return the exact same result set in all cases as long as grade items/categories live within the same course (which they must by design, but I can verify that across all Joule sites anyway). So what we have is an optimization - the new query should run much faster than the original version. Does that change your requirements?
          Hide
          kstokking Kris Stokking added a comment -

          Fred - I was originally concerned that we might be kicking a hornet's nest by trying to test every permutation of the gradebook. After a chat with Sam today, I realize that won't be necessary, and he will be submitting some unit tests tonight and tomorrow. I'll also toss up some testing instructions to go along with them, but be advised that the performance impact won't be noticed unless it's used on a site with a very large number of grade items (several hundred thousand).

          Show
          kstokking Kris Stokking added a comment - Fred - I was originally concerned that we might be kicking a hornet's nest by trying to test every permutation of the gradebook. After a chat with Sam today, I realize that won't be necessary, and he will be submitting some unit tests tonight and tomorrow. I'll also toss up some testing instructions to go along with them, but be advised that the performance impact won't be noticed unless it's used on a site with a very large number of grade items (several hundred thousand).
          Hide
          sbc24 Sam Chaffee added a comment -

          Hi Fred,

          Thanks for the feedback. As Kris mentioned, I've added a unit test to cover the aggregatesubcats case (the first query that has changed). For now I've only added it to my master branch changes. If it looks good I'll add it to the 2.5 and 2.4 branches as well along with some other tests to cover a couple cases.

          Show
          sbc24 Sam Chaffee added a comment - Hi Fred, Thanks for the feedback. As Kris mentioned, I've added a unit test to cover the aggregatesubcats case (the first query that has changed). For now I've only added it to my master branch changes. If it looks good I'll add it to the 2.5 and 2.4 branches as well along with some other tests to cover a couple cases.
          Hide
          fred Frédéric Massart added a comment -

          Thanks Kris and Sam,

          I agree that the Unit Tests should have been written previously, but I was just raising a concern that integrators would have had anyway. I am not very familiar with the gradebook, but I know that it's a sensitive part of Moodle and it would be horrible to introduce regressions there. Now with the Unit Tests I think everyone can be more confident.

          Not sure if we have generators to populate a large amount of data here. I think it's fair to say that your script will not negatively affect performances so my point was just about testing (during the testing phase) that those queries are doing what there were doing before.

          Thanks for the patch, report and feedback, I think you can freely push this for integration after writing some basic testing instructions.

          Cheers,
          Fred

          Show
          fred Frédéric Massart added a comment - Thanks Kris and Sam, I agree that the Unit Tests should have been written previously, but I was just raising a concern that integrators would have had anyway. I am not very familiar with the gradebook, but I know that it's a sensitive part of Moodle and it would be horrible to introduce regressions there. Now with the Unit Tests I think everyone can be more confident. Not sure if we have generators to populate a large amount of data here. I think it's fair to say that your script will not negatively affect performances so my point was just about testing (during the testing phase) that those queries are doing what there were doing before. Thanks for the patch, report and feedback, I think you can freely push this for integration after writing some basic testing instructions. Cheers, Fred
          Hide
          sbc24 Sam Chaffee added a comment -

          Hi Fred,

          Thanks again for your earlier review. I added the additional unit tests to all 3 integration branches and rebased my changes on Friday's weekly release. Would you mind taking another look before we send this for integration?

          Show
          sbc24 Sam Chaffee added a comment - Hi Fred, Thanks again for your earlier review. I added the additional unit tests to all 3 integration branches and rebased my changes on Friday's weekly release. Would you mind taking another look before we send this for integration?
          Hide
          fred Frédéric Massart added a comment -

          Hi Sam,

          This looks good, I just spotted a tiny thing in the Unit Tests. Instead of assuming the default value of $CFG->grade_includescalesinaggregation, you could probably set it to what you are expecting. Also, as there is no reset between all the testcases, I think it's better if you check what are the values of the $CFG->foo before altering it, and restoring it to that value at the end.

          This still needs some testing instructions in order to be ready for integration, apart from those comments, it looks good to me.

          Thanks!
          Fred

          Show
          fred Frédéric Massart added a comment - Hi Sam, This looks good, I just spotted a tiny thing in the Unit Tests. Instead of assuming the default value of $CFG->grade_includescalesinaggregation, you could probably set it to what you are expecting. Also, as there is no reset between all the testcases, I think it's better if you check what are the values of the $CFG->foo before altering it, and restoring it to that value at the end. This still needs some testing instructions in order to be ready for integration, apart from those comments, it looks good to me. Thanks! Fred
          Hide
          sbc24 Sam Chaffee added a comment -

          Hi Fred,

          I've updated the unit tests to explicitly set the $CFG values and to store the original values and reset them at the end of the subtest.

          Thanks,

          Sam

          Show
          sbc24 Sam Chaffee added a comment - Hi Fred, I've updated the unit tests to explicitly set the $CFG values and to store the original values and reset them at the end of the subtest. Thanks, Sam
          Hide
          sbc24 Sam Chaffee added a comment -

          I think this is ready for the integrators to have a look. Fred, if you agree, would you please move this along to the next step in the workflow?

          Show
          sbc24 Sam Chaffee added a comment - I think this is ready for the integrators to have a look. Fred, if you agree, would you please move this along to the next step in the workflow?
          Hide
          skodak Petr Skoda added a comment -

          +1

          Show
          skodak Petr Skoda added a comment - +1
          Hide
          damyon Damyon Wiese added a comment -

          Still looking but there are oracle unit test fails which are taking some time to determine if they are introduced or existing.

          Show
          damyon Damyon Wiese added a comment - Still looking but there are oracle unit test fails which are taking some time to determine if they are introduced or existing.
          Hide
          damyon Damyon Wiese added a comment -

          It's not related to this patch - but testing this discovered: MDL-42380

          Show
          damyon Damyon Wiese added a comment - It's not related to this patch - but testing this discovered: MDL-42380
          Hide
          damyon Damyon Wiese added a comment -

          Thanks Sam,

          Oracle passed without any new errors (eventually). Integrated to 24, 25 and master.

          Show
          damyon Damyon Wiese added a comment - Thanks Sam, Oracle passed without any new errors (eventually). Integrated to 24, 25 and master.
          Hide
          damyon Damyon Wiese added a comment -

          ping

          Show
          damyon Damyon Wiese added a comment - ping
          Hide
          damyon Damyon Wiese added a comment - - edited

          starting testing on 24 - looks ok - passing for 24.

          Show
          damyon Damyon Wiese added a comment - - edited starting testing on 24 - looks ok - passing for 24.
          Hide
          dobedobedoh Andrew Nicols added a comment -

          starting testing on master

          Show
          dobedobedoh Andrew Nicols added a comment - starting testing on master
          Hide
          damyon Damyon Wiese added a comment -

          passing on 25 - looks ok.

          Show
          damyon Damyon Wiese added a comment - passing on 25 - looks ok.
          Hide
          dobedobedoh Andrew Nicols added a comment -

          passing on master - looks okay

          Show
          dobedobedoh Andrew Nicols added a comment - passing on master - looks okay
          Hide
          poltawski Dan Poltawski added a comment -

          Hurrah! Thanks for your contribution - this fix is part of Moodle.

          Show
          poltawski Dan Poltawski added a comment - Hurrah! Thanks for your contribution - this fix is part of Moodle.

            People

            • Votes:
              1 Vote for this issue
              Watchers:
              7 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:
                Fix Release Date:
                11/Nov/13