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 2.4 Branch:
      wip-MDL-30797-24
    • Pull Master Branch:
      wip-MDL-30797-master
    • Rank:
      33735

      Description

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

        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: