Uploaded image for project: '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

      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.

        Gliffy Diagrams

          Issue Links

            Activity

            Hide
            jrchamp Jonathan Champ added a comment -

            Added 2.x changesets.

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

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

            Show
            jrchamp Jonathan Champ added a comment - Increasing priority as the entire application ceased to function as a result of these slow queries.
            Hide
            pferre22 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
            pferre22 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
            jrchamp 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
            jrchamp 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
            pferre22 Pau Ferrer Ocaña (crazyserver) added a comment -

            Of course, both patchs are compatible!

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

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

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

            Thanks to you!

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

            Show
            pferre22 Pau Ferrer Ocaña (crazyserver) added a comment - Thanks to you! One more thing, our database is postgreSQL so it is also affected
            Hide
            poltawski 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
            poltawski 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
            poltawski Dan Poltawski added a comment -

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

            Note: I haven't reviewed this myself.

            Show
            poltawski Dan Poltawski added a comment - Grr. setting this to 'waiting for peer review' state. Note: I haven't reviewed this myself.
            Hide
            andyjdavis 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
            andyjdavis 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
            poltawski 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
            poltawski 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
            andyjdavis Andrew Davis added a comment -

            Assigning this me and the next stable sprint

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

            Thanks for taking it Andrew

            Show
            poltawski Dan Poltawski added a comment - Thanks for taking it Andrew
            Hide
            jrchamp 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
            jrchamp 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
            andyjdavis Andrew Davis added a comment -

            Adding testing instructions.

            Show
            andyjdavis Andrew Davis added a comment - Adding testing instructions.
            Show
            andyjdavis 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
            andyjdavis 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
            andyjdavis 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
            andyjdavis 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
            andyjdavis 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
            dmonllao 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
            dmonllao 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
            salvetore Michael de Raadt added a comment -

            Carrying over to the new sprint.

            Show
            salvetore Michael de Raadt added a comment - Carrying over to the new sprint.
            Hide
            jrchamp 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
            jrchamp 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
            andyjdavis 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
            andyjdavis 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
            jrchamp 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
            jrchamp 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
            dmonllao 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
            dmonllao 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
            jrchamp 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
            jrchamp 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
            pferre22 Pau Ferrer Ocaña (crazyserver) added a comment -

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

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

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

            Show
            andyjdavis Andrew Davis added a comment - Sorry for the delay. Putting this up for peer review again.
            Hide
            dmonllao 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
            dmonllao 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
            andyjdavis Andrew Davis added a comment -

            Rebased. Submitting for integration.

            Show
            andyjdavis Andrew Davis added a comment - Rebased. Submitting for integration.
            Hide
            poltawski 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
            poltawski 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
            samhemelryk 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
            samhemelryk 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
            jrchamp 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
            jrchamp 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
            andyjdavis 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
            andyjdavis 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
            samhemelryk Sam Hemelryk added a comment -

            Thanks for clearing that up, this has been integrated now

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

            Thanks Andrew,

            works as expected.

            Show
            rajeshtaneja Rajesh Taneja added a comment - Thanks Andrew, works as expected.
            Hide
            stronk7 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
            stronk7 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:
                2 Start watching this issue

                Dates

                • Created:
                  Updated:
                  Resolved:
                  Fix Release Date:
                  14/Jan/13