Moodle
  1. Moodle
  2. MDL-30797

Missing Unique Key/Index in groupings_groups (groupingid, groupid)

    Details

    • Database:
      Any
    • Testing Instructions:
      Hide

      Set up

      1. Create a course.
      2. Create some groups and create some groupings.
      3. Add groups to groupings.
      4. Back up the course.

      Restore into this course

      1. Restore the course making sure to "Merge the backup course into this course" (Destination section of restore).
      2. Complete the restore and open up your database.
      3. Check the groupings_groups table and make sure that there are no entries with the same combination of grouping id and grouid.

      Restore as a new course

      1. Restore the course making sure to "Restore as a new course" (Destination section of restore).
      2. Complete the restore and open up your database.
      3. Check the groupings_groups table and make sure that there are no entries with the same combination of grouping id and grouid.
      Show
      Set up Create a course. Create some groups and create some groupings. Add groups to groupings. Back up the course. Restore into this course Restore the course making sure to "Merge the backup course into this course" (Destination section of restore). Complete the restore and open up your database. Check the groupings_groups table and make sure that there are no entries with the same combination of grouping id and grouid . Restore as a new course Restore the course making sure to "Restore as a new course" (Destination section of restore). Complete the restore and open up your database. Check the groupings_groups table and make sure that there are no entries with the same combination of grouping id and grouid .
    • Affected Branches:
      MOODLE_21_STABLE, MOODLE_22_STABLE, MOODLE_23_STABLE
    • Fixed Branches:
      MOODLE_23_STABLE, MOODLE_24_STABLE
    • Pull from Repository:
    • Pull Master Branch:
      wip-MDL-30797-master

      Description

      There shouldn't be more than one row in groupings_groups with specific groupingid, groupid

        Gliffy Diagrams

          Activity

          Hide
          Michael de Raadt added a comment -

          Hi, Asaf.

          How are you proposing to create such entries?

          Are you doing this through the interface? If so, what steps are you taking to achieve this?

          Or are you manipulating the database directly?

          Show
          Michael de Raadt added a comment - Hi, Asaf. How are you proposing to create such entries? Are you doing this through the interface? If so, what steps are you taking to achieve this? Or are you manipulating the database directly?
          Hide
          Asaf Ohaion added a comment -

          It doesn't matter how the entries are created,
          duplicate entries might be created by Moodle because of some bug,
          in any case, the database system shouldn't allow this,
          Uniqueness should be enforced by the DB system

          Show
          Asaf Ohaion added a comment - It doesn't matter how the entries are created, duplicate entries might be created by Moodle because of some bug, in any case, the database system shouldn't allow this, Uniqueness should be enforced by the DB system
          Hide
          Michael de Raadt added a comment -

          Sorry for the delay in getting back to you.

          I'm still not sure exactly what you are suggesting here.

          Are you saying there is not a unique key requirement enforced in the DB structure and you believe there should be?

          Show
          Michael de Raadt added a comment - Sorry for the delay in getting back to you. I'm still not sure exactly what you are suggesting here. Are you saying there is not a unique key requirement enforced in the DB structure and you believe there should be?
          Hide
          Tony Levi added a comment -

          Hi,

          Just noting we've hit this in at least two production sites, quite badly. It causes all memory to be consumed and massive swapping (effectively an outage).

          There is definitely some Moodle code causing duplicates, the unique index should be added to prevent such bugs causing havoc.

          Show
          Tony Levi added a comment - Hi, Just noting we've hit this in at least two production sites, quite badly. It causes all memory to be consumed and massive swapping (effectively an outage). There is definitely some Moodle code causing duplicates, the unique index should be added to prevent such bugs causing havoc.
          Hide
          Adam Olley added a comment -

          Hi Michael,

          I believe that's exactly what Asaf is suggesting. I've just come across a bunch of duplicate groupings_groups entries in a couple of our clients databases. These are clients without any customizations around groups and groupings, so somewhere, somehow, in core it's allowing these duplicates to be created. But just like how course completion didn't have a unique index on course/user pairs which was causing access to courses to be completely blocked off, this can prevent access to group management pages if too many duplicates exist for a given pair.

          Show
          Adam Olley added a comment - Hi Michael, I believe that's exactly what Asaf is suggesting. I've just come across a bunch of duplicate groupings_groups entries in a couple of our clients databases. These are clients without any customizations around groups and groupings, so somewhere, somehow, in core it's allowing these duplicates to be created. But just like how course completion didn't have a unique index on course/user pairs which was causing access to courses to be completely blocked off, this can prevent access to group management pages if too many duplicates exist for a given pair.
          Hide
          Adam Olley added a comment -

          Upgrade code should just be as simple as:
          */ begin transation
          */ remove duplicate entries
          */ add unique index on groupingid,groupid
          */ commit transaction

          Show
          Adam Olley added a comment - Upgrade code should just be as simple as: */ begin transation */ remove duplicate entries */ add unique index on groupingid,groupid */ commit transaction
          Hide
          Adam Olley added a comment -

          I've found the root cause of this. Import/Restore.

          The restore steps imports groups and groupings and the mappings between them DIRECTLY using insert_record. It should be making the appropriate call off to groups_assign_grouping($groupingid, $groupid), that func checks to make sure the record doesn't already exist first since it's meant to be unique.

          Everytime you import a course that has group mappings, you'll notice that you get duplicates in the mdl_groupings_groups table.

          I'll edit this issue with a github url to this part of the fix soon

          Show
          Adam Olley added a comment - I've found the root cause of this. Import/Restore. The restore steps imports groups and groupings and the mappings between them DIRECTLY using insert_record. It should be making the appropriate call off to groups_assign_grouping($groupingid, $groupid), that func checks to make sure the record doesn't already exist first since it's meant to be unique. Everytime you import a course that has group mappings, you'll notice that you get duplicates in the mdl_groupings_groups table. I'll edit this issue with a github url to this part of the fix soon
          Hide
          Adam Olley added a comment - - edited

          Looks like the duplicate causing bug in import/restore was fixed in 2.1.3 (MDL-29350) (I was looking at 2.1.2 code at the time).

          However, I find it odd the fix that went in still ignored the correct way to be mapping a group to a grouping, and that's via the Group API!

          See suggested change for that as follows:
          https://github.com/aolley/moodle/compare/moodle:master...aolley:MDL-30797

          The code just becomes more and more unmanageable if areas don't delegate to the correct APIs when it makes sense to do so. It makes any future changes to tables, like say, enforcing a unique index where there should be one, tricky.

          Show
          Adam Olley added a comment - - edited Looks like the duplicate causing bug in import/restore was fixed in 2.1.3 ( MDL-29350 ) (I was looking at 2.1.2 code at the time). However, I find it odd the fix that went in still ignored the correct way to be mapping a group to a grouping, and that's via the Group API! See suggested change for that as follows: https://github.com/aolley/moodle/compare/moodle:master...aolley:MDL-30797 The code just becomes more and more unmanageable if areas don't delegate to the correct APIs when it makes sense to do so. It makes any future changes to tables, like say, enforcing a unique index where there should be one, tricky.
          Hide
          Michael de Raadt added a comment -

          It looks like this issue has been neglected for some time.

          I've promoted the issue so it will hopefully get some attention soon.

          Show
          Michael de Raadt added a comment - It looks like this issue has been neglected for some time. I've promoted the issue so it will hopefully get some attention soon.
          Hide
          Adrian Greeve added a comment -

          The issue in the title has been fixed and code was included to clean up any duplicate entries that might have existed.

          This patch is just to tidy up the existing code so that it uses the appropriate API in the groups lib.

          Show
          Adrian Greeve added a comment - The issue in the title has been fixed and code was included to clean up any duplicate entries that might have existed. This patch is just to tidy up the existing code so that it uses the appropriate API in the groups lib.
          Hide
          Frédéric Massart added a comment -

          Thanks for your work on this guys, here are three comments for you Adrian:

          • I noticed that the diff provided by Adam is really similar to your solution, you should perhaps credits those lines to him, and then add the casting to object that you were right to add.
          • In the original code, the timeadded field was likely to be restored, now that we are using the core code, this value is not restored at all. I don't think this is highly important, but there is a difference between the original logic and the one proposed.
          • Should this be backported?

          Cheers,
          Fred

          Show
          Frédéric Massart added a comment - Thanks for your work on this guys, here are three comments for you Adrian: I noticed that the diff provided by Adam is really similar to your solution, you should perhaps credits those lines to him, and then add the casting to object that you were right to add. In the original code, the timeadded field was likely to be restored, now that we are using the core code, this value is not restored at all. I don't think this is highly important, but there is a difference between the original logic and the one proposed. Should this be backported? Cheers, Fred
          Hide
          Adrian Greeve added a comment -

          Hi Fred,

          • Fixed this up so that Adam's commit is included.
          • I created a separate commit for an addition to the groups_assign_grouping() function so that it will take 'time added'.
          • If we include the altered function then I would propose that this not be backported.

          Could you have another look for me please Fred?

          Show
          Adrian Greeve added a comment - Hi Fred, Fixed this up so that Adam's commit is included. I created a separate commit for an addition to the groups_assign_grouping() function so that it will take 'time added'. If we include the altered function then I would propose that this not be backported. Could you have another look for me please Fred?
          Hide
          Frédéric Massart added a comment -

          Looks good to me Adrian, I just noticed that there is a typo in the PHP Docs for $timeadded ('into' instead of 'int'). Also, I don't think there is a need for 2 different commits, they could probably be merged, but that's not important. And another thing, that is more of my personal coding style, I'd make sure that the value of $timeadded is not null instead of checking if it's true and then cast it to int, but that's just me and it's not blocking your patch at all.

          Thanks

          Show
          Frédéric Massart added a comment - Looks good to me Adrian, I just noticed that there is a typo in the PHP Docs for $timeadded ('into' instead of 'int'). Also, I don't think there is a need for 2 different commits, they could probably be merged, but that's not important. And another thing, that is more of my personal coding style, I'd make sure that the value of $timeadded is not null instead of checking if it's true and then cast it to int, but that's just me and it's not blocking your patch at all. Thanks
          Hide
          Adrian Greeve added a comment -

          Hi Fred,

          I made the alterations that you suggested.

          Submitting for integration review.

          Show
          Adrian Greeve added a comment - Hi Fred, I made the alterations that you suggested. Submitting for integration review.
          Hide
          Eloy Lafuente (stronk7) 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
          Eloy Lafuente (stronk7) 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
          Adrian Greeve added a comment -

          Rebased and ready to go.

          Show
          Adrian Greeve added a comment - Rebased and ready to go.
          Hide
          Dan Poltawski added a comment -

          Hi Adrian, as discussed in chat, seems like this could be backported as the existing behviour of function not changed.

          Show
          Dan Poltawski added a comment - Hi Adrian, as discussed in chat, seems like this could be backported as the existing behviour of function not changed.
          Hide
          Adrian Greeve added a comment -

          Extra branches have been added.

          Thanks.

          Show
          Adrian Greeve added a comment - Extra branches have been added. Thanks.
          Hide
          Dan Poltawski added a comment -

          Integrated to master, 24 and 23.

          Thanks Adrian/Adam

          Show
          Dan Poltawski added a comment - Integrated to master, 24 and 23. Thanks Adrian/Adam
          Hide
          Andrew Davis added a comment -

          Works as described. Passing.

          Show
          Andrew Davis added a comment - Works as described. Passing.
          Hide
          Dan Poltawski added a comment -

          Hurray! We did it! Thanks to all the reporters, testers, user and watchers for a bumper week of Moodling!

          Show
          Dan Poltawski added a comment - Hurray! We did it! Thanks to all the reporters, testers, user and watchers for a bumper week of Moodling!

            People

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

              Dates

              • Created:
                Updated:
                Resolved: