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

Problems with upgrade_grade_item_fix_sortorder()

    Details

    • Testing Instructions:
      Hide

      You will need to start out with versions dated Jan 13 or before (2.5.4, 2.6.1, non-current master).

      (Stolen from MDL-41062):
      Setup in old Moodle:

      1. Create COURSE1 with default settings
      2. Add an assignment 'Assign 1' with default settings
      3. Create COURSE2 with default settings
      4. Add an assignment 'Assign 2' with default settings
      5. VERIFY: Examine the database and ensure the grade items exist, and both have the same sort order in different courses. e.g:

        SELECT id, itemname, courseid, sortorder 
        FROM mdl_grade_items 
        WHERE itemname LIKE 'Assign%'
        ORDER BY sortorder, courseid;
         
         id | itemname | courseid | sortorder
        ----+----------+----------+-----------
          4 | Assign 1 |        2 |         2
          2 | Assign 2 |        3 |         2
        

      Create duplicates through import:

      1. Go to COURSE1
      2. Select import, choose COURSE2
      3. Choose ONLY to import ASSIGNCOURSE2
      4. VERIFY: Examine the database and ensure the grade items that are imported have created duplicates, there should be results fem the following query:

        SELECT courseid, sortorder
        FROM {grade_items}
        GROUP BY courseid, sortorder
        HAVING COUNT(id) > 1
        

      Upgrade to current version with this patch:

      1. VERIFY: Run the same query as above, and ensure there now are no results.
      2. Go to the Gradebook of COURSE1
      3. Go to Grades > Categories and items > Simple view and ensure that the grade items for the two assignments appear
      4. VERIFY: that you can resort the grade items and they behave in the way the interface demonstrates
      Show
      You will need to start out with versions dated Jan 13 or before (2.5.4, 2.6.1, non-current master). (Stolen from MDL-41062 ): Setup in old Moodle: Create COURSE1 with default settings Add an assignment 'Assign 1' with default settings Create COURSE2 with default settings Add an assignment 'Assign 2' with default settings VERIFY: Examine the database and ensure the grade items exist, and both have the same sort order in different courses. e.g: SELECT id, itemname, courseid, sortorder FROM mdl_grade_items WHERE itemname LIKE 'Assign%' ORDER BY sortorder, courseid;   id | itemname | courseid | sortorder ----+----------+----------+----------- 4 | Assign 1 | 2 | 2 2 | Assign 2 | 3 | 2 Create duplicates through import: Go to COURSE1 Select import, choose COURSE2 Choose ONLY to import ASSIGNCOURSE2 VERIFY: Examine the database and ensure the grade items that are imported have created duplicates, there should be results fem the following query: SELECT courseid, sortorder FROM {grade_items} GROUP BY courseid, sortorder HAVING COUNT(id) > 1 Upgrade to current version with this patch: VERIFY: Run the same query as above, and ensure there now are no results. Go to the Gradebook of COURSE1 Go to Grades > Categories and items > Simple view and ensure that the grade items for the two assignments appear VERIFY: that you can resort the grade items and they behave in the way the interface demonstrates
    • Affected Branches:
      MOODLE_25_STABLE, MOODLE_26_STABLE, MOODLE_27_STABLE
    • Fixed Branches:
      MOODLE_25_STABLE, MOODLE_26_STABLE
    • Pull Master Branch:

      Description

      This is a regression of MDL-41062 which landed recently.

      It seems that the main query of upgrade_grade_item_fix_sortorder() returns too many records, leading to the iteration later in code to perform way many updates (slow and advancing too much).

      More details, for your consideration in this comment.

      Ciao

        Gliffy Diagrams

          Issue Links

            Activity

            Hide
            stronk7 Eloy Lafuente (stronk7) added a comment -

            FYI, I've just tried it against a "extreme" site I had here. It contains 2-3 courses each one having 512, 1024 and 2048 activities. And they have a very high number of duplicates, because they were created by using backup and restore multiple times some months ago (2=>4=>8=>...=>1024=>2048 activities).

            SELECT count(*) FROM mdl_grade_items;
            +----------+
            | count(*) |
            +----------+
            |     4209 |
            +----------+

            And current query is returning > 10^6 records. >2h running CLI upgrade without ending (Web upgrade run out of time here)...

            Again, it's an extreme (and surely unreal) example, but from 4209 to 10^6 heavy updates (they have both inequalities and OR conditions, so indexes are doomed, basically), well, there is a difference.

            Ciao

            Show
            stronk7 Eloy Lafuente (stronk7) added a comment - FYI, I've just tried it against a "extreme" site I had here. It contains 2-3 courses each one having 512, 1024 and 2048 activities. And they have a very high number of duplicates, because they were created by using backup and restore multiple times some months ago (2=>4=>8=>...=>1024=>2048 activities). SELECT count(*) FROM mdl_grade_items; +----------+ | count(*) | +----------+ | 4209 | +----------+ And current query is returning > 10^6 records. >2h running CLI upgrade without ending (Web upgrade run out of time here)... Again, it's an extreme (and surely unreal) example, but from 4209 to 10^6 heavy updates (they have both inequalities and OR conditions, so indexes are doomed, basically), well, there is a difference. Ciao
            Hide
            emerrill Eric Merrill added a comment -

            For us this returns about 250,000 items, which we are pretty confident is way to high (we only have 133k grade items...). For now we have removed it from the upgrade code, and will need to run it separately when this problem is fixed.

            Show
            emerrill Eric Merrill added a comment - For us this returns about 250,000 items, which we are pretty confident is way to high (we only have 133k grade items...). For now we have removed it from the upgrade code, and will need to run it separately when this problem is fixed.
            Hide
            emerrill Eric Merrill added a comment -

            Just a real quick patch attempt, still need to look at the unit tests.

            Show
            emerrill Eric Merrill added a comment - Just a real quick patch attempt, still need to look at the unit tests.
            Show
            cibot CiBoT added a comment - Results for MDL-43946 Remote repository: https://github.com/merrill-oakland/moodle.git Remote branch MDL-43946 _m26 to be integrated into upstream MOODLE_26_STABLE Executed job http://integration.moodle.org/job/Precheck%20remote%20branch/1107 Details: http://integration.moodle.org/job/Precheck%20remote%20branch/1107/artifact/work/smurf.html Remote branch MDL-43946 to be integrated into upstream master Executed job http://integration.moodle.org/job/Precheck%20remote%20branch/1108 Details: http://integration.moodle.org/job/Precheck%20remote%20branch/1108/artifact/work/smurf.html
            Hide
            emerrill Eric Merrill added a comment -

            Unit tests pass. In our system, it takes the returned records from 250k to 10k.

            I'm not sure the best way to setup test instructions for this - should I just put to run unit tests? Re-run the tests in MDL-41062? Something else?

            Also, the same patch should probably happen for the code introduced in MDL-41062, but I'll do that once this one at least has a peer review.

            Show
            emerrill Eric Merrill added a comment - Unit tests pass. In our system, it takes the returned records from 250k to 10k. I'm not sure the best way to setup test instructions for this - should I just put to run unit tests? Re-run the tests in MDL-41062 ? Something else? Also, the same patch should probably happen for the code introduced in MDL-41062 , but I'll do that once this one at least has a peer review.
            Show
            cibot CiBoT added a comment - Results for MDL-43946 Remote repository: https://github.com/merrill-oakland/moodle.git Remote branch MDL-43946 _m26 to be integrated into upstream MOODLE_26_STABLE Executed job http://integration.moodle.org/job/Precheck%20remote%20branch/1109 Details: http://integration.moodle.org/job/Precheck%20remote%20branch/1109/artifact/work/smurf.html Remote branch MDL-43946 to be integrated into upstream master Executed job http://integration.moodle.org/job/Precheck%20remote%20branch/1110 Details: http://integration.moodle.org/job/Precheck%20remote%20branch/1110/artifact/work/smurf.html
            Hide
            stronk7 Eloy Lafuente (stronk7) added a comment - - edited

            Tiny comment: the original issue was also applied to 25_STABLE.

            About testing I'd borrow original ones, but forcing the tester to begin with v2.5.4, v2.6.1 and 3 weeks old master. And surely asking for at least 1 of them to be executed under every database flavor.

            Show
            stronk7 Eloy Lafuente (stronk7) added a comment - - edited Tiny comment: the original issue was also applied to 25_STABLE. About testing I'd borrow original ones, but forcing the tester to begin with v2.5.4, v2.6.1 and 3 weeks old master. And surely asking for at least 1 of them to be executed under every database flavor.
            Hide
            emerrill Eric Merrill added a comment -

            Thanks - updated. Should be ready for integration when pending another once-over.

            Show
            emerrill Eric Merrill added a comment - Thanks - updated. Should be ready for integration when pending another once-over.
            Hide
            stronk7 Eloy Lafuente (stronk7) added a comment -

            I think it's the simplest correct solution and should do the trick, so sending to integration, thanks!

            Show
            stronk7 Eloy Lafuente (stronk7) added a comment - I think it's the simplest correct solution and should do the trick, so sending to integration, thanks!
            Hide
            emerrill Eric Merrill added a comment -

            I've applied they same patch to the restore code over at: MDL-44046 (currently awaiting a peer review)

            Integrator: It may make sense to do these two together, as they are the same patch for the same problem, just in different parts of the code - it will probably be easiest for the same integrator to do them at the same time.

            Show
            emerrill Eric Merrill added a comment - I've applied they same patch to the restore code over at: MDL-44046 (currently awaiting a peer review) Integrator: It may make sense to do these two together, as they are the same patch for the same problem, just in different parts of the code - it will probably be easiest for the same integrator to do them at the same time.
            Hide
            cibot CiBoT added a comment -

            Moving this issue to current integration cycle, will be reviewed soon. Thanks for the hard work!

            Show
            cibot CiBoT added a comment - Moving this issue to current integration cycle, will be reviewed soon. Thanks for the hard work!
            Hide
            samhemelryk Sam Hemelryk added a comment -

            Thanks guys this has been integrated now.

            Show
            samhemelryk Sam Hemelryk added a comment - Thanks guys this has been integrated now.
            Hide
            markn Mark Nelson added a comment -

            Works as expected, thanks Eric.

            Show
            markn Mark Nelson added a comment - Works as expected, thanks Eric.
            Hide
            stronk7 Eloy Lafuente (stronk7) added a comment -

            Fetch your remotes, prune them,
            clean your integrated branches and say "Ahem".

            Rebase your ongoing stuff, keep conflicts away
            don't forget to test the code and we'll love you again.

            Thanks, closing!

            Show
            stronk7 Eloy Lafuente (stronk7) added a comment - Fetch your remotes, prune them, clean your integrated branches and say "Ahem". Rebase your ongoing stuff, keep conflicts away don't forget to test the code and we'll love you again. Thanks, closing!

              People

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

                Dates

                • Created:
                  Updated:
                  Resolved:
                  Fix Release Date:
                  10/Mar/14