Moodle
  1. Moodle
  2. MDL-29733

Query for Identifying Grade Grades to Precreate is SLOW

    Details

    • Database:
      MySQL
    • Testing Instructions:
      Hide

      This will need testing in mysql and postgres at a minimum.

      Go to the categories and items screen within the gradebook of a course.

      Create a calculated grade item. Make its value equal to another activity. For example if you have a gradeable activity with the label "myactivity" set the calculated to "=[[myactivity]]"

      Save and go to the grader report.

      Override the gradeable activity, the one that the calc item relies upon. Check that you don't get an error and that the calc item has the same value as the original grade item.

      Repeat in other databases.

      For bonus points run "phpunit grade_item_testcase lib/grade/tests/grade_item_test.php"

      Show
      This will need testing in mysql and postgres at a minimum. Go to the categories and items screen within the gradebook of a course. Create a calculated grade item. Make its value equal to another activity. For example if you have a gradeable activity with the label "myactivity" set the calculated to "=[ [myactivity] ]" Save and go to the grader report. Override the gradeable activity, the one that the calc item relies upon. Check that you don't get an error and that the calc item has the same value as the original grade item. Repeat in other databases. For bonus points run "phpunit grade_item_testcase lib/grade/tests/grade_item_test.php"
    • Affected Branches:
      MOODLE_19_STABLE, MOODLE_20_STABLE, MOODLE_21_STABLE, MOODLE_22_STABLE
    • Fixed Branches:
      MOODLE_22_STABLE, MOODLE_23_STABLE, MOODLE_24_STABLE
    • Pull Master Branch:
      MDL-29733_master
    • Rank:
      19248

      Description

      When a large number of students were taking a quiz yesterday, the grade_item::compute() function was being triggered on a fairly regular basis. The query to identify the grade_grades to precreate took an exceedingly long time to execute and used up our available connection pool.

      The query in question joins the monolithic grade_grades table with itself in an optional (LEFT OUTER) fashion which limits the types of query optimizations available. To avoid this, instead of using DISTINCT to achieve uniqueness while also making the optional join unnecessary, I would propose using the available index to GROUP BY userid and identify within an aggregation function whether the user meets the criteria.

        Issue Links

          Activity

          Hide
          Jonathan Champ added a comment -

          Added 2.x changesets.

          Show
          Jonathan Champ added a comment - Added 2.x changesets.
          Hide
          Jonathan Champ added a comment -

          Increasing priority as the entire application ceased to function as a result of these slow queries.

          Show
          Jonathan Champ added a comment - Increasing priority as the entire application ceased to function as a result of these slow queries.
          Hide
          Pau Ferrer Ocaña (crazyserver) added a comment -

          We have this other workarroung in 2.2:

                  if ($userid) {
                  	if (!$DB->record_exists('grade_grades',array('itermid'=>$this->id,'userid'=>$userid))) {
                  		$grade = new grade_grade(array('itemid'=>$this->id, 'userid'=>$userid), false);
                          	$grade->grade_item =& $this;
                         		$grade->insert('system');
                  	}
                  } else {
          	        // precreate grades - we need them to exist
          	        $params = array($this->courseid, $this->id, $this->id);
          	        $sql = "SELECT DISTINCT go.userid
          	                  FROM {grade_grades} go
          	                       JOIN {grade_items} gi
          	                       ON (gi.id = go.itemid AND gi.courseid = ?)
          	                       LEFT OUTER JOIN {grade_grades} g
          	                       ON (g.userid = go.userid AND g.itemid = ?)
          	                 WHERE gi.id <> ? AND g.id IS NULL";
          	        if ($missing = $DB->get_records_sql($sql, $params)) {
          	            foreach ($missing as $m) {
          	                $grade = new grade_grade(array('itemid'=>$this->id, 'userid'=>$m->userid), false);
          	                $grade->grade_item =& $this;
          	                $grade->insert('system');
          	            }
          	        }
                  }
          
          Show
          Pau Ferrer Ocaña (crazyserver) added a comment - We have this other workarroung in 2.2: if ($userid) { if (!$DB->record_exists('grade_grades',array('itermid'=>$ this ->id,'userid'=>$userid))) { $grade = new grade_grade(array('itemid'=>$ this ->id, 'userid'=>$userid), false ); $grade->grade_item =& $ this ; $grade->insert('system'); } } else { // precreate grades - we need them to exist $params = array($ this ->courseid, $ this ->id, $ this ->id); $sql = "SELECT DISTINCT go.userid FROM {grade_grades} go JOIN {grade_items} gi ON (gi.id = go.itemid AND gi.courseid = ?) LEFT OUTER JOIN {grade_grades} g ON (g.userid = go.userid AND g.itemid = ?) WHERE gi.id <> ? AND g.id IS NULL"; if ($missing = $DB->get_records_sql($sql, $params)) { foreach ($missing as $m) { $grade = new grade_grade(array('itemid'=>$ this ->id, 'userid'=>$m->userid), false ); $grade->grade_item =& $ this ; $grade->insert('system'); } } }
          Hide
          Jonathan Champ added a comment - - edited

          What do you think about reorganizing it like this?

          <?php
                  if ($userid) {
                      $missing = array();
                      if (!$DB->record_exists('grade_grades', array('itemid'=>$this->id, 'userid'=>$userid))) {
                          $m = new stdClass();
                          $m->userid = $userid;
                          $missing[] = $m;
                      }
                  } else {
                      // precreate grades - we need them to exist
                      $params = array($this->courseid, $this->id);
                      $sql = "SELECT go.userid
                                  FROM {grade_grades} go
                                      JOIN {grade_items} gi
                                      ON (gi.id = go.itemid AND gi.courseid = ?)
                                  GROUP BY go.userid
                                  HAVING SUM(go.itemid = ?) = 0";
                      $missing = $DB->get_records_sql($sql, $params);
                  }
          
                  if ($missing) {
                      foreach ($missing as $m) {
                          $grade = new grade_grade(array('itemid'=>$this->id, 'userid'=>$m->userid), false);
                          $grade->grade_item =& $this;
                          $grade->insert('system');
                      }
                  }
          
          Show
          Jonathan Champ added a comment - - edited What do you think about reorganizing it like this? <?php if ($userid) { $missing = array(); if (!$DB->record_exists('grade_grades', array('itemid'=>$ this ->id, 'userid'=>$userid))) { $m = new stdClass(); $m->userid = $userid; $missing[] = $m; } } else { // precreate grades - we need them to exist $params = array($ this ->courseid, $ this ->id); $sql = "SELECT go.userid FROM {grade_grades} go JOIN {grade_items} gi ON (gi.id = go.itemid AND gi.courseid = ?) GROUP BY go.userid HAVING SUM(go.itemid = ?) = 0"; $missing = $DB->get_records_sql($sql, $params); } if ($missing) { foreach ($missing as $m) { $grade = new grade_grade(array('itemid'=>$ this ->id, 'userid'=>$m->userid), false ); $grade->grade_item =& $ this ; $grade->insert('system'); } }
          Hide
          Pau Ferrer Ocaña (crazyserver) added a comment -

          Of course, both patchs are compatible!

          Show
          Pau Ferrer Ocaña (crazyserver) added a comment - Of course, both patchs are compatible!
          Hide
          Jonathan Champ added a comment -

          Student shortcut (thanks Pau Ferrer Ocaña (crazyserver)) added to all branches and rebased.

          Show
          Jonathan Champ added a comment - Student shortcut (thanks Pau Ferrer Ocaña (crazyserver) ) added to all branches and rebased.
          Hide
          Pau Ferrer Ocaña (crazyserver) added a comment -

          Thanks to you!

          One more thing, our database is postgreSQL so it is also affected

          Show
          Pau Ferrer Ocaña (crazyserver) added a comment - Thanks to you! One more thing, our database is postgreSQL so it is also affected
          Hide
          Dan Poltawski added a comment -

          Hi Jonathan,

          Thanks a lot for your patch here, sorry we have not managed to review it! I'm putting this patch up for peer review so someone can take a look at it and see if its sensible.

          Show
          Dan Poltawski added a comment - Hi Jonathan, Thanks a lot for your patch here, sorry we have not managed to review it! I'm putting this patch up for peer review so someone can take a look at it and see if its sensible.
          Hide
          Dan Poltawski added a comment -

          Grr. setting this to 'waiting for peer review' state.

          Note: I haven't reviewed this myself.

          Show
          Dan Poltawski added a comment - Grr. setting this to 'waiting for peer review' state. Note: I haven't reviewed this myself.
          Hide
          Andrew Davis added a comment -

          Dan, if you're the assignee you need to look at this before putting it up for peer review. Let me know if you don't have time and I'll assign it to myself, go through it and get someone else to peer review it.

          Additional work that needs doing (either by Jonathon, Dan or myself):

          Adding additional tests to sub_test_grade_item_compute() in lib/grade/tests/grade_item_test.php could possibly be worth doing for additional verification and insurance against future breakage.

          The testing instructions need improving. The tester will be testing this through the Moodle UI, not profiling individual queries. They're primarily concerned with us not having broken anything. It will need testing in mysql and postgres at a minimum.

          Was there a particular reason for switching from the left outer join to "HAVING SUM(go.itemid = ?) = 0"? I personally find the new way relatively unintuitive.

          The table alias "go" for grade_grades doesnt seem to make any sense. I know its not new. Perhaps we can take the opportunity to change it to "gg" or similar.

          A final note. Moodle HQ will only be integrating this into 2.2, 2.3 and the next upcoming release, 2.4. Users of older versions can access the branches above but we encourage you to upgrade at the first available opportunity

          Show
          Andrew Davis added a comment - Dan, if you're the assignee you need to look at this before putting it up for peer review. Let me know if you don't have time and I'll assign it to myself, go through it and get someone else to peer review it. Additional work that needs doing (either by Jonathon, Dan or myself): Adding additional tests to sub_test_grade_item_compute() in lib/grade/tests/grade_item_test.php could possibly be worth doing for additional verification and insurance against future breakage. The testing instructions need improving. The tester will be testing this through the Moodle UI, not profiling individual queries. They're primarily concerned with us not having broken anything. It will need testing in mysql and postgres at a minimum. Was there a particular reason for switching from the left outer join to "HAVING SUM(go.itemid = ?) = 0"? I personally find the new way relatively unintuitive. The table alias "go" for grade_grades doesnt seem to make any sense. I know its not new. Perhaps we can take the opportunity to change it to "gg" or similar. A final note. Moodle HQ will only be integrating this into 2.2, 2.3 and the next upcoming release, 2.4. Users of older versions can access the branches above but we encourage you to upgrade at the first available opportunity
          Hide
          Dan Poltawski added a comment -

          Andrew, I just assigned it to myself to set it into the 'waiting for peer review' state. I didn't have time to look at it myself, but wanted to patch looked at. (So, feel free to take it)

          Show
          Dan Poltawski added a comment - Andrew, I just assigned it to myself to set it into the 'waiting for peer review' state. I didn't have time to look at it myself, but wanted to patch looked at. (So, feel free to take it)
          Hide
          Andrew Davis added a comment -

          Assigning this me and the next stable sprint

          Show
          Andrew Davis added a comment - Assigning this me and the next stable sprint
          Hide
          Dan Poltawski added a comment -

          Thanks for taking it Andrew

          Show
          Dan Poltawski added a comment - Thanks for taking it Andrew
          Hide
          Jonathan Champ added a comment -

          The reasoning is that because the grade_grades table is rather large, doing an extra non-matching join when you already have the necessary data is very wasteful. The performance characteristics of the updated query uses all equal joins (cuts out a duplicate table with a not-exists) and can use indexes more easily (GROUP BY optimizes more easily than DISTINCT).

          -            $sql = "SELECT DISTINCT go.userid
          -                      FROM {grade_grades} go
          -                       JOIN {grade_items} gi
          -                       ON (gi.id = go.itemid AND gi.courseid = ?)
          -                       LEFT OUTER JOIN {grade_grades} g
          -                       ON (g.userid = go.userid AND g.itemid = ?)
          -                 WHERE gi.id <> ? AND g.id IS NULL";
          

          The old query can be read as:
          "Show me the distinct set of userids
          when looking at the grades for the items in this course (but not the item in question, so left join the grades table again for the item in question)
          and make sure there is no grade for the item in question".

          +            $sql = "SELECT go.userid
          +                      FROM {grade_grades} go
          +                           JOIN {grade_items} gi
          +                           ON (gi.id = go.itemid AND gi.courseid = ?)
          +                     GROUP BY go.userid
          +                     HAVING SUM(go.itemid = ?) = 0";
          

          The new query can be read as:
          "Show me the distinct set of userids
          when looking at the grades for the items in this course
          and make sure there is no grade for the item in question".

          Changing the table alias to gg does make a lot more sense. I don't have a lot of experience with the unit tests, so if someone else wants to take on that part of it, please do.

          Thank you for taking the time to look at this!

          Show
          Jonathan Champ added a comment - The reasoning is that because the grade_grades table is rather large, doing an extra non-matching join when you already have the necessary data is very wasteful. The performance characteristics of the updated query uses all equal joins (cuts out a duplicate table with a not-exists) and can use indexes more easily (GROUP BY optimizes more easily than DISTINCT). - $sql = "SELECT DISTINCT go.userid - FROM {grade_grades} go - JOIN {grade_items} gi - ON (gi.id = go.itemid AND gi.courseid = ?) - LEFT OUTER JOIN {grade_grades} g - ON (g.userid = go.userid AND g.itemid = ?) - WHERE gi.id <> ? AND g.id IS NULL"; The old query can be read as: "Show me the distinct set of userids when looking at the grades for the items in this course (but not the item in question, so left join the grades table again for the item in question) and make sure there is no grade for the item in question". + $sql = "SELECT go.userid + FROM {grade_grades} go + JOIN {grade_items} gi + ON (gi.id = go.itemid AND gi.courseid = ?) + GROUP BY go.userid + HAVING SUM(go.itemid = ?) = 0"; The new query can be read as: "Show me the distinct set of userids when looking at the grades for the items in this course and make sure there is no grade for the item in question". Changing the table alias to gg does make a lot more sense. I don't have a lot of experience with the unit tests, so if someone else wants to take on that part of it, please do. Thank you for taking the time to look at this!
          Hide
          Andrew Davis added a comment -

          Adding testing instructions.

          Show
          Andrew Davis added a comment - Adding testing instructions.
          Show
          Andrew Davis added a comment - I'm going to overwrite the branch information in this issue. For future reference here are the original values. Pull from Repository: https://github.com/jrchamp/moodle Pull 1.9 Branch: https://github.com/jrchamp/moodle/tree/MDL-29733 Pull 1.9 Diff URL: https://github.com/jrchamp/moodle/compare/MOODLE_19_STABLE...MDL-29733 Pull 2.1 Branch: https://github.com/jrchamp/moodle/tree/MDL-29733_moodle21 Pull 2.1 Diff URL: https://github.com/jrchamp/moodle/compare/MOODLE_21_STABLE...MDL-29733_moodle21 Pull 2.2 Branch: https://github.com/jrchamp/moodle/tree/MDL-29733_moodle22 Pull 2.2 Diff URL: https://github.com/jrchamp/moodle/compare/MOODLE_22_STABLE...MDL-29733_moodle22 Pull Master Branch: https://github.com/jrchamp/moodle/tree/MDL-29733_master Pull Master Diff URL: https://github.com/jrchamp/moodle/compare/master...MDL-29733_master
          Hide
          Andrew Davis added a comment -

          New branches added. I've added a second commit containing a little clean up. I had a look at the unit tests. They're not great but are good enough.

          Show
          Andrew Davis added a comment - New branches added. I've added a second commit containing a little clean up. I had a look at the unit tests. They're not great but are good enough.
          Hide
          Andrew Davis added a comment -

          I'm putting this up for peer review. If anyone has an non-production environment where they can test this fix that would be great. More testing is always welcome

          Show
          Andrew Davis added a comment - I'm putting this up for peer review. If anyone has an non-production environment where they can test this fix that would be great. More testing is always welcome
          Hide
          David Monllaó added a comment -

          Hi Andrew,

          The query must work in the 4 DB engines; In Postgres I'm receiving the following error:

          Debug info: ERROR: function sum(boolean) does not exist
          LINE 6: HAVING SUM(gg.itemid = $2) = 0
          ^
          HINT: No function matches the given name and argument types. You might need to add explicit type casts.
          SELECT gg.userid
          FROM mdl_grade_grades gg
          JOIN mdl_grade_items gi
          ON (gi.id = gg.itemid AND gi.courseid = $1)
          GROUP BY gg.userid
          HAVING SUM(gg.itemid = $2) = 0
          [array (
          0 => '2',
          1 => '4',
          )]
          Error code: dmlreadexception
          Stack trace:
          line 407 of /lib/dml/moodle_database.php: dml_read_exception thrown
          line 239 of /lib/dml/pgsql_native_moodle_database.php: call to moodle_database->query_end()
          line 715 of /lib/dml/pgsql_native_moodle_database.php: call to pgsql_native_moodle_database->query_end()
          line 1749 of /lib/grade/grade_item.php: call to pgsql_native_moodle_database->get_records_sql()
          line 656 of /lib/grade/grade_item.php: call to grade_item->compute()
          line 1073 of /lib/gradelib.php: call to grade_item->regrade_final_grades()
          line 110 of /grade/edit/tree/index.php: call to grade_regrade_final_grades()
          

          In a Oracle console I receive the following error:

          ORA-00907: missing right parenthesis
          

          I haven't tried MSSQL

          Show
          David Monllaó added a comment - Hi Andrew, The query must work in the 4 DB engines; In Postgres I'm receiving the following error: Debug info: ERROR: function sum( boolean ) does not exist LINE 6: HAVING SUM(gg.itemid = $2) = 0 ^ HINT: No function matches the given name and argument types. You might need to add explicit type casts. SELECT gg.userid FROM mdl_grade_grades gg JOIN mdl_grade_items gi ON (gi.id = gg.itemid AND gi.courseid = $1) GROUP BY gg.userid HAVING SUM(gg.itemid = $2) = 0 [array ( 0 => '2', 1 => '4', )] Error code: dmlreadexception Stack trace: line 407 of /lib/dml/moodle_database.php: dml_read_exception thrown line 239 of /lib/dml/pgsql_native_moodle_database.php: call to moodle_database->query_end() line 715 of /lib/dml/pgsql_native_moodle_database.php: call to pgsql_native_moodle_database->query_end() line 1749 of /lib/grade/grade_item.php: call to pgsql_native_moodle_database->get_records_sql() line 656 of /lib/grade/grade_item.php: call to grade_item->compute() line 1073 of /lib/gradelib.php: call to grade_item->regrade_final_grades() line 110 of /grade/edit/tree/index.php: call to grade_regrade_final_grades() In a Oracle console I receive the following error: ORA-00907: missing right parenthesis I haven't tried MSSQL
          Hide
          Michael de Raadt added a comment -

          Carrying over to the new sprint.

          Show
          Michael de Raadt added a comment - Carrying over to the new sprint.
          Hide
          Jonathan Champ added a comment - - edited

          To remove the confusion for Oracle and Postgre, we could replace

          +            $sql = "SELECT gg.userid
          +                      FROM {grade_grades} gg
          +                           JOIN {grade_items} gi
          +                           ON (gi.id = gg.itemid AND gi.courseid = ?)
          +                     GROUP BY gg.userid
          +                     HAVING SUM(gg.itemid = ?) = 0";
          

          with

          +            $sql = "SELECT gg.userid, SUM(IF(gg.itemid = ?, 1, 0)) AS matching_items
          +                      FROM {grade_grades} gg
          +                           JOIN {grade_items} gi
          +                           ON (gi.id = gg.itemid AND gi.courseid = ?)
          +                     GROUP BY gg.userid
          +                     HAVING matching_items = 0";
          

          Just remember to reverse the SQL parameter order now that the itemid placeholder appears first.

          Show
          Jonathan Champ added a comment - - edited To remove the confusion for Oracle and Postgre, we could replace + $sql = "SELECT gg.userid + FROM {grade_grades} gg + JOIN {grade_items} gi + ON (gi.id = gg.itemid AND gi.courseid = ?) + GROUP BY gg.userid + HAVING SUM(gg.itemid = ?) = 0"; with + $sql = "SELECT gg.userid, SUM(IF(gg.itemid = ?, 1, 0)) AS matching_items + FROM {grade_grades} gg + JOIN {grade_items} gi + ON (gi.id = gg.itemid AND gi.courseid = ?) + GROUP BY gg.userid + HAVING matching_items = 0"; Just remember to reverse the SQL parameter order now that the itemid placeholder appears first.
          Hide
          Andrew Davis added a comment -

          Hi Jonathan. I've gone with your suggestion. The SQL validator I use (http://developer.mimer.com/validator/parser200x/index.tml#parser) complains about the IF however it does seem to work fine.

          I've raised MDL-35610 as I ran into that while working on this.

          I've rolled the alteration of the SQL into my previous commit. Putting this up for peer review.

          Show
          Andrew Davis added a comment - Hi Jonathan. I've gone with your suggestion. The SQL validator I use ( http://developer.mimer.com/validator/parser200x/index.tml#parser ) complains about the IF however it does seem to work fine. I've raised MDL-35610 as I ran into that while working on this. I've rolled the alteration of the SQL into my previous commit. Putting this up for peer review.
          Hide
          Jonathan Champ added a comment -

          Bah, I did not realize that IF was a platform specific extension.

          This appears to pass your Mimer SQL validator:

          +            $sql = "SELECT gg.userid, SUM(CASE WHEN gg.itemid = ? THEN 1 ELSE 0 END) AS matching_items
          +                      FROM {grade_grades} gg
          +                           JOIN {grade_items} gi
          +                           ON (gi.id = gg.itemid AND gi.courseid = ?)
          +                     GROUP BY gg.userid
          +                     HAVING matching_items = 0";
          
          Show
          Jonathan Champ added a comment - Bah, I did not realize that IF was a platform specific extension. This appears to pass your Mimer SQL validator: + $sql = "SELECT gg.userid, SUM(CASE WHEN gg.itemid = ? THEN 1 ELSE 0 END) AS matching_items + FROM {grade_grades} gg + JOIN {grade_items} gi + ON (gi.id = gg.itemid AND gi.courseid = ?) + GROUP BY gg.userid + HAVING matching_items = 0";
          Hide
          David Monllaó added a comment -

          Andrew, the query of the patch fails in an oracle console (Oracle 10g)

          SELECT gg.userid, SUM(IF(gg.itemid = 145, 1, 0)) AS matching_items FROM m_grade_grades gg JOIN m_grade_items gi ON (gi.id = gg.itemid AND gi.courseid = 53) GROUP BY gg.userid HAVING matching_items = 0
                                             *
          ERROR at line 1:
          ORA-00907: missing right parenthesis
          

          Jonathan, your last proposal passes the validator, but I'm finding an error with Postgres 9.1.6

          SELECT gg.userid, SUM(CASE WHEN gg.itemid = 145 THEN 1 ELSE 0 END) AS matching_items 
          FROM mdl_grade_grades gg 
          JOIN mdl_grade_items gi 
          ON (gi.id = gg.itemid AND gi.courseid = 53) 
          GROUP BY gg.userid 
          HAVING matching_items = 0;
          ------------------------------------------------------
          ERROR:  column "matching_items" does not exist
          LINE 6: HAVING matching_items = 0;
                         ^
          
          
          ********** Error **********
          
          ERROR: column "matching_items" does not exist
          SQL state: 42703
          Character: 209
          
          
          Show
          David Monllaó added a comment - Andrew, the query of the patch fails in an oracle console (Oracle 10g) SELECT gg.userid, SUM(IF(gg.itemid = 145, 1, 0)) AS matching_items FROM m_grade_grades gg JOIN m_grade_items gi ON (gi.id = gg.itemid AND gi.courseid = 53) GROUP BY gg.userid HAVING matching_items = 0 * ERROR at line 1: ORA-00907: missing right parenthesis Jonathan, your last proposal passes the validator, but I'm finding an error with Postgres 9.1.6 SELECT gg.userid, SUM(CASE WHEN gg.itemid = 145 THEN 1 ELSE 0 END) AS matching_items FROM mdl_grade_grades gg JOIN mdl_grade_items gi ON (gi.id = gg.itemid AND gi.courseid = 53) GROUP BY gg.userid HAVING matching_items = 0; ------------------------------------------------------ ERROR: column "matching_items" does not exist LINE 6: HAVING matching_items = 0; ^ ********** Error ********** ERROR: column "matching_items" does not exist SQL state: 42703 Character : 209
          Hide
          Jonathan Champ added a comment -

          Yeah, that's what I was worried about... the only thing I would know to try is to move the SUM back to the HAVING clause now that we think that the IF was the issue.

          SELECT gg.userid
          FROM mdl_grade_grades gg 
          JOIN mdl_grade_items gi 
          ON (gi.id = gg.itemid AND gi.courseid = 53) 
          GROUP BY gg.userid 
          HAVING SUM(CASE WHEN gg.itemid = 145 THEN 1 ELSE 0 END) = 0;
          

          Which would be coded as

          +            $sql = "SELECT gg.userid
          +                      FROM {grade_grades} gg
          +                           JOIN {grade_items} gi
          +                           ON (gi.id = gg.itemid AND gi.courseid = ?)
          +                     GROUP BY gg.userid
          +                     HAVING SUM(CASE WHEN gg.itemid = ? THEN 1 ELSE 0 END) = 0";
          
          Show
          Jonathan Champ added a comment - Yeah, that's what I was worried about... the only thing I would know to try is to move the SUM back to the HAVING clause now that we think that the IF was the issue. SELECT gg.userid FROM mdl_grade_grades gg JOIN mdl_grade_items gi ON (gi.id = gg.itemid AND gi.courseid = 53) GROUP BY gg.userid HAVING SUM(CASE WHEN gg.itemid = 145 THEN 1 ELSE 0 END) = 0; Which would be coded as + $sql = "SELECT gg.userid + FROM {grade_grades} gg + JOIN {grade_items} gi + ON (gi.id = gg.itemid AND gi.courseid = ?) + GROUP BY gg.userid + HAVING SUM(CASE WHEN gg.itemid = ? THEN 1 ELSE 0 END) = 0";
          Hide
          Pau Ferrer Ocaña (crazyserver) added a comment -

          Thanks Jonathan! This last workarround worked for me in postgresql!

          Show
          Pau Ferrer Ocaña (crazyserver) added a comment - Thanks Jonathan! This last workarround worked for me in postgresql!
          Hide
          Andrew Davis added a comment -

          Sorry for the delay. Putting this up for peer review again.

          Show
          Andrew Davis added a comment - Sorry for the delay. Putting this up for peer review again.
          Hide
          David Monllaó added a comment -

          Hi Andrew,

          For what I've tested (thanks to Aparup for Mssql) the query is compatible with the 4 databases engines.

          Show
          David Monllaó added a comment - Hi Andrew, For what I've tested (thanks to Aparup for Mssql) the query is compatible with the 4 databases engines.
          Hide
          Andrew Davis added a comment -

          Rebased. Submitting for integration.

          Show
          Andrew Davis added a comment - Rebased. Submitting for integration.
          Hide
          Dan Poltawski 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
          Dan Poltawski 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 -

          Hi guys,

          No doubt this query is an improvement over the current approach.
          Given it has been well tested we are happy with that.

          However I've discussed this issue at length with Eloy this morning and we've one concern.
          More in regard to the handling of this in general than the solution here.
          It seems to use that using the grade_grades table as the source when identifying holes still risks there being situations where holes won't be plugged.
          We're keen to know what would happen in the following situations:

          1. What if the course has only 1 grade-item?
          2. What if there are N grade_items but all them are missing userid X ?

          I'll leave this in review presently and see what you have to say about those.
          Perhaps neither are an issue in which case this will land, or perhaps there is something uncovered there that will represent a new issue.
          Anyway will wait for the reply.

          Many thanks
          Sam

          Show
          Sam Hemelryk added a comment - Hi guys, No doubt this query is an improvement over the current approach. Given it has been well tested we are happy with that. However I've discussed this issue at length with Eloy this morning and we've one concern. More in regard to the handling of this in general than the solution here. It seems to use that using the grade_grades table as the source when identifying holes still risks there being situations where holes won't be plugged. We're keen to know what would happen in the following situations: What if the course has only 1 grade-item? What if there are N grade_items but all them are missing userid X ? I'll leave this in review presently and see what you have to say about those. Perhaps neither are an issue in which case this will land, or perhaps there is something uncovered there that will represent a new issue. Anyway will wait for the reply. Many thanks Sam
          Hide
          Jonathan Champ added a comment -

          The original scope of this was to make a poorly performing query perform better. The query itself may not be necessary at all. If it is necessary, knowing why would help define a superior implementation of the overall logic.

          Why does a grade_grade need to exist for each item for each user? Surely this makes some queries simpler that rely on such a constraint, but generates database entries that may never be relevant. Perhaps some grade methods rely on having an entry to determine which "lowest grade to drop".

          If there is only one grade_item or the user never participates, I would expect them to fail the class. In general, the risk of have holes is likely more related to average scores, averages with lowest or highest dropped. Typical sums should not be affected as much.

          A better solution might be to join the list of graded users. Not sure what the SQL WHERE would be in that case, whether or not you want to include non-enrolled students. This would at least prevent extra database entries being generated for user without a role_assignment.

          Ultimately, this patch makes the performance noticeably better on this query and I believe it should land. Going forward, a new issue (one that indicates a relationship with this issue) should be created to identify the purpose of this code and tackle the problem of modifying the system to offer this functionality in a smarter way.

          Hope that helps!

          Show
          Jonathan Champ added a comment - The original scope of this was to make a poorly performing query perform better. The query itself may not be necessary at all. If it is necessary, knowing why would help define a superior implementation of the overall logic. Why does a grade_grade need to exist for each item for each user? Surely this makes some queries simpler that rely on such a constraint, but generates database entries that may never be relevant. Perhaps some grade methods rely on having an entry to determine which "lowest grade to drop". If there is only one grade_item or the user never participates, I would expect them to fail the class. In general, the risk of have holes is likely more related to average scores, averages with lowest or highest dropped. Typical sums should not be affected as much. A better solution might be to join the list of graded users. Not sure what the SQL WHERE would be in that case, whether or not you want to include non-enrolled students. This would at least prevent extra database entries being generated for user without a role_assignment. Ultimately, this patch makes the performance noticeably better on this query and I believe it should land. Going forward, a new issue (one that indicates a relationship with this issue) should be created to identify the purpose of this code and tackle the problem of modifying the system to offer this functionality in a smarter way. Hope that helps!
          Hide
          Andrew Davis added a comment - - edited

          Sam for #1 that you raised, what if there is only 1 grade item, there is this code above...

          if (!$this->is_calculated()) {
              return false;
          }
          

          I don' claim to be 100% sure of why this safety net exists however it only comes into play once we encounter a calculated grade item. For this code to even execute while having a single grade item you would need to have the course contain nothing but a single calculated grade item. Actually the course itself has a grade item so even in any empty course its possible the user would already have a row in grade_grade although I haven't tested that.

          The unit tests in lib/grade/tests/grade_item_tests.php could certainly be enhanced to cover this and any other edge cases we're concerned about. My preference would be to see this integrated and I'll do some further investigating and unit test writing in another issue.

          Show
          Andrew Davis added a comment - - edited Sam for #1 that you raised, what if there is only 1 grade item, there is this code above... if (!$ this ->is_calculated()) { return false ; } I don' claim to be 100% sure of why this safety net exists however it only comes into play once we encounter a calculated grade item. For this code to even execute while having a single grade item you would need to have the course contain nothing but a single calculated grade item. Actually the course itself has a grade item so even in any empty course its possible the user would already have a row in grade_grade although I haven't tested that. The unit tests in lib/grade/tests/grade_item_tests.php could certainly be enhanced to cover this and any other edge cases we're concerned about. My preference would be to see this integrated and I'll do some further investigating and unit test writing in another issue.
          Hide
          Sam Hemelryk added a comment -

          Thanks for clearing that up, this has been integrated now

          Show
          Sam Hemelryk added a comment - Thanks for clearing that up, this has been integrated now
          Hide
          Rajesh Taneja added a comment -

          Thanks Andrew,

          works as expected.

          Show
          Rajesh Taneja added a comment - Thanks Andrew, works as expected.
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Changes are now upstream, thanks for your collaboration!

          If you are going to have any celebration next days, enjoy with your gang, if not, too!

          Ciao

          Show
          Eloy Lafuente (stronk7) added a comment - Changes are now upstream, thanks for your collaboration! If you are going to have any celebration next days, enjoy with your gang, if not, too! Ciao

            People

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

              Dates

              • Created:
                Updated:
                Resolved: