Moodle
  1. Moodle
  2. MDL-29350

Duplicating activities causes duplicate rows in groupings_groups table

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Critical Critical
    • Resolution: Fixed
    • Affects Version/s: 2.1.1, 2.1.2, 2.2
    • Fix Version/s: 2.0.6, 2.1.3
    • Component/s: Backup, Course
    • Labels:
      None
    • Testing Instructions:
      Hide

      To test the duplication process:

      • Create an empty course.
      • Create one grouping in the course, then create one group in this grouping.
      • Create an activity (say, an assignment), set it to "Visible groups", and restrict it to the grouping.
      • Click the "Duplicate" button and confirm.
      • Now verify that the following query returns an empty result:

      SELECT COUNT(id) AS idcnt, MIN(id) AS firstid, groupingid, groupid FROM mdl_groupings_groups GROUP BY groupingid, groupid HAVING idcnt>1;

      To test the database cleanup:

      • Produce the bad DB entries caused by this bug (see "replication instructions" in the description)
      • Run the upgrade
      • Verify that the following query returns an empty result:

      SELECT COUNT(id) AS idcnt, MIN(id) AS firstid, groupingid, groupid FROM mdl_groupings_groups GROUP BY groupingid, groupid HAVING idcnt>1;

      Show
      To test the duplication process: Create an empty course. Create one grouping in the course, then create one group in this grouping. Create an activity (say, an assignment), set it to "Visible groups", and restrict it to the grouping. Click the "Duplicate" button and confirm. Now verify that the following query returns an empty result: SELECT COUNT(id) AS idcnt, MIN(id) AS firstid, groupingid, groupid FROM mdl_groupings_groups GROUP BY groupingid, groupid HAVING idcnt>1; To test the database cleanup: Produce the bad DB entries caused by this bug (see "replication instructions" in the description) Run the upgrade Verify that the following query returns an empty result: SELECT COUNT(id) AS idcnt, MIN(id) AS firstid, groupingid, groupid FROM mdl_groupings_groups GROUP BY groupingid, groupid HAVING idcnt>1;
    • Affected Branches:
      MOODLE_21_STABLE, MOODLE_22_STABLE
    • Fixed Branches:
      MOODLE_20_STABLE, MOODLE_21_STABLE
    • Pull from Repository:
    • Pull Master Branch:
      MDL-29350-master-2
    • Rank:
      18878

      Description

      When duplicating activities in a course (using the "Duplicate" icon), duplicate records may be inserted into the groupings_groups table.

      To reproduce:

      • Create an empty course.
      • Create one grouping in the course, then create one group in this grouping.
      • Create an activity (say, an assignment), set it to "Visible groups", and restrict it to the grouping.
      • Click the "Duplicate" button and confirm.
      • Now run the following query manually on the DB:

      SELECT COUNT(id) AS idcnt, MIN(id) AS firstid, groupingid, groupid FROM mdl_groupings_groups GROUP BY groupingid, groupid HAVING idcnt>1;

      This will now show (at least) one line with idcnt=2.

      Any further duplication will double the number of rows. On my production system, there's one groupingid-groupid pair which has 16384 table entries. Since many parts of the code assume that (groupingid, groupid) is unique, this leads to various strange effects in different places of Moodle, for example in the screens that assign groups to groupings.

      The reason for this is in the backup/restore code in restore_groups_structure_step::process_grouping_group() . This "restores" a groupings_groups entry even if it already exists.

      Patch to follow.

        Issue Links

          Activity

          Hide
          Henning Bostelmann added a comment -

          Linked a fix for master. This removes the bug, and contains cleanup code for the existing data.

          Maybe a unique index should be added to the groupings_groups table?

          Also increasing priority to "Critical" since this bug leads to data corruption.

          Show
          Henning Bostelmann added a comment - Linked a fix for master. This removes the bug, and contains cleanup code for the existing data. Maybe a unique index should be added to the groupings_groups table? Also increasing priority to "Critical" since this bug leads to data corruption.
          Hide
          Sam Hemelryk added a comment -

          Wow good spotting Henning, this is definitely one for Eloy (our backup/restore genius).
          I've made him peer-reviewer of this issue, Eloy you'll want to check this out

          Show
          Sam Hemelryk added a comment - Wow good spotting Henning, this is definitely one for Eloy (our backup/restore genius). I've made him peer-reviewer of this issue, Eloy you'll want to check this out
          Hide
          Patrick Mulrooney added a comment - - edited

          I can verify this happened to us as well. The 'groupings_groups' table had grown to 1,000,000+ entries (only 300 of which were unique) causing a major performance loss.

          The fix given prevented duplication. Thanks!

          Show
          Patrick Mulrooney added a comment - - edited I can verify this happened to us as well. The 'groupings_groups' table had grown to 1,000,000+ entries (only 300 of which were unique) causing a major performance loss. The fix given prevented duplication. Thanks!
          Hide
          Christopher Butera added a comment - - edited

          I'm experiencing this as well. I added a unique index to the groupings table and this seemed to prevent duplication, but unfortunately it prints a database error when duplicating assignments.
          UNIQUE KEY `groupingdup` (`groupingid`,`groupid`)

          Show
          Christopher Butera added a comment - - edited I'm experiencing this as well. I added a unique index to the groupings table and this seemed to prevent duplication, but unfortunately it prints a database error when duplicating assignments. UNIQUE KEY `groupingdup` (`groupingid`,`groupid`)
          Hide
          Christopher Butera added a comment -

          The fix worked for me as well. Let's get this committed please.

          Show
          Christopher Butera added a comment - The fix worked for me as well. Let's get this committed please.
          Hide
          Andrew Nicols added a comment -

          We're hitting this too.

          Show
          Andrew Nicols added a comment - We're hitting this too.
          Hide
          Andrew Nicols added a comment -

          Grabbing this from Eloy because he's not had a chance to look at it yet

          Show
          Andrew Nicols added a comment - Grabbing this from Eloy because he's not had a chance to look at it yet
          Hide
          Andrew Nicols added a comment -

          I've tested activity duplication on MOODLE_21_STABLE, and master.
          Also confirmed that backup/restore doesn't break with this commit.
          The database update seems to do the trick for Postgres too.

          I'd prefer it if the restore_stepslib.php change was a bit smaller. Rather than storing the groupingid and groupid in a separate variable and then applying it to both $data and $params, it would read better as:

          $params = array();
          $params['groupingid'] = $data->groupingid;
          $params['groupid']    = $data->groupid;
          

          This could also reduce the risk of something breaking in the future.

          Henning: Can you provide pull branches for the stable branches too? I don't think that 2.0 is affected as there's no way of duplicating an activity within a course and whole course backup/restore doesn't suffer this issue.

          Thanks

          Show
          Andrew Nicols added a comment - I've tested activity duplication on MOODLE_21_STABLE, and master. Also confirmed that backup/restore doesn't break with this commit. The database update seems to do the trick for Postgres too. I'd prefer it if the restore_stepslib.php change was a bit smaller. Rather than storing the groupingid and groupid in a separate variable and then applying it to both $data and $params, it would read better as: $params = array(); $params['groupingid'] = $data->groupingid; $params['groupid'] = $data->groupid; This could also reduce the risk of something breaking in the future. Henning: Can you provide pull branches for the stable branches too? I don't think that 2.0 is affected as there's no way of duplicating an activity within a course and whole course backup/restore doesn't suffer this issue. Thanks
          Hide
          Andrew Nicols added a comment -

          Oh, I also noticed some trailing whitespace in both commits which should probably disappear

          Show
          Andrew Nicols added a comment - Oh, I also noticed some trailing whitespace in both commits which should probably disappear
          Hide
          Andrew Nicols added a comment -

          There are two commits to each of these. I'm afraid I haven't worked out the magic Fu! to get gitweb to show both commits.

          Show
          Andrew Nicols added a comment - There are two commits to each of these. I'm afraid I haven't worked out the magic Fu! to get gitweb to show both commits.
          Hide
          Dan Poltawski added a comment -

          This looks good, thanks everyone.

          I think it will be better for these two commits to be merged into one though. They are based on the same fixing the same bug, although I understand the rationale for 2.0, especially with version number changes but they are both the same issue.

          Show
          Dan Poltawski added a comment - This looks good, thanks everyone. I think it will be better for these two commits to be merged into one though. They are based on the same fixing the same bug, although I understand the rationale for 2.0, especially with version number changes but they are both the same issue.
          Hide
          Andrew Nicols added a comment -

          I've merged the commits and updated the database upgrade for the latest weekly releases.

          Show
          Andrew Nicols added a comment - I've merged the commits and updated the database upgrade for the latest weekly releases.
          Hide
          Eloy Lafuente (stronk7) added a comment -

          I'm going to integrate this, looks perfect, how did I miss to look for dupes there, grrr.

          Anyway, given the numbers from Patrick above (10^6 dupes for only 300 real records, what a ratio!) I'm going to add one commit:

          • switching from get_records() to get_recordset() to avoid memory-bound problems
          • some calls to upgrade_set_timeout() each 1000 iterations so the script won't timeout.

          Ciao

          Show
          Eloy Lafuente (stronk7) added a comment - I'm going to integrate this, looks perfect, how did I miss to look for dupes there, grrr. Anyway, given the numbers from Patrick above (10^6 dupes for only 300 real records, what a ratio!) I'm going to add one commit: switching from get_records() to get_recordset() to avoid memory-bound problems some calls to upgrade_set_timeout() each 1000 iterations so the script won't timeout. Ciao
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Integrated, thanks!

          Show
          Eloy Lafuente (stronk7) added a comment - Integrated, thanks!
          Hide
          Eloy Lafuente (stronk7) added a comment -

          I've tested the query against the 4 DBs.

          Then performed various duplicate and import executions of activities having some grouping (with groups) assigned, nothing became duplicated, as expected.

          Finally, I've run the upgrade in one site having dupes, with debug on and the queries seemed ok, and were ok, there were no more dupes after the upgrade.

          So passed, thanks!

          Show
          Eloy Lafuente (stronk7) added a comment - I've tested the query against the 4 DBs. Then performed various duplicate and import executions of activities having some grouping (with groups) assigned, nothing became duplicated, as expected. Finally, I've run the upgrade in one site having dupes, with debug on and the queries seemed ok, and were ok, there were no more dupes after the upgrade. So passed, thanks!
          Hide
          Eloy Lafuente (stronk7) added a comment -

          U P S T R E A M I Z E D !

          Thanks! Closing...

          Show
          Eloy Lafuente (stronk7) added a comment - U P S T R E A M I Z E D ! Thanks! Closing...
          Hide
          Sam Marshall added a comment -

          For info, anyone wants to fix this quickly in database, here's a Postgres query that works (I think) to delete the duplicate groups. (The one in the code works in a php loop so not easy to do manually.) This one will not work in mysql on account of suckage.

          DELETE FROM mdl_groupings_groups WHERE id IN(
          SELECT id FROM mdl_groupings_groups g
          JOIN (SELECT MIN(id) AS firstid, groupingid, groupid FROM mdl_groupings_groups GROUP BY groupingid, groupid HAVING count(id)>1) dups
          ON dups.groupingid = g.groupingid AND dups.groupid = g.groupid AND g.id > dups.firstid)
          
          Show
          Sam Marshall added a comment - For info, anyone wants to fix this quickly in database, here's a Postgres query that works (I think) to delete the duplicate groups. (The one in the code works in a php loop so not easy to do manually.) This one will not work in mysql on account of suckage. DELETE FROM mdl_groupings_groups WHERE id IN( SELECT id FROM mdl_groupings_groups g JOIN (SELECT MIN(id) AS firstid, groupingid, groupid FROM mdl_groupings_groups GROUP BY groupingid, groupid HAVING count(id)>1) dups ON dups.groupingid = g.groupingid AND dups.groupid = g.groupid AND g.id > dups.firstid)

            People

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

              Dates

              • Created:
                Updated:
                Resolved: