Moodle
  1. Moodle
  2. MDL-38228

Major Performance Problem with "Moving Modules" upgrade step

    Details

    • Testing Instructions:
      Hide

      Much of this testing is a duplicate of MDL-37939 as we are just altering the SQL query in that upgrade step.

      Install a fresh 2.3 version just prior to 2012062504.08

      1. Create a new course
      2. Create 5-6 course modules in each of 3 sections.
      3. Move course modules between sections. .

      4. They will appear to move, but they will be broken sequence data
      Confirm that with SQL queries;

      SELECT id, section, sequence from mdl_course_sections where course = ? order by section asc;
      SELECT id, section from mdl_course_modules where course = ?;

      Refresh the page and make some more changes:
      Confirm that although the drag/drop appears to work, when you actually refresh the page it doesn't persist across a refresh

      5. Now apply the patch and upgrade
      confirm that the CMs didn't appear to move from where they were before the upgrade
      confirm that the DB looks correct now (no duplicates in sequence data)
      move some CMs around sections
      refresh
      confirm that the section sequence data is correct
      confirm that the cm.section is correct

      On 2.4 and 2.5

      1. Take a course with a fair few CMs
      2. Apply the upgrade
      3. Confirm that the CMs didn't appear to move from where they were before the upgrade
      4. Run the SQL to confirm sequences are correct
      SELECT id, section, sequence from mdl_course_sections where course = ? order by section asc;
      SELECT id, section from mdl_course_modules where course = ?;

      Breaking&fixing test:

      By editing the DB directly create situations when fields course_modules.section and course_sections.sequence (inside the same course) do not match, for example:

      • Module points to the wrong section
      • Module points to the non-existing section (i.e. section 0)
      • Module is present is several section sequences
      • Module is present several times in its section sequence
      • Module is not present in any section sequences

      Re-run the upgrade script and make sure it is fixed. Note that in 2.6 you will need to actually view the course to complete fixing.

      Hint, to re-run the upgrade script you can create a file testupgrade.php :

      <?php
      include "config.php";
      require_once($CFG->libdir.'/db/upgradelib.php'); 
      upgrade_course_modules_sequences();
      purge_all_caches();
      echo "Done.";
      

      Show
      Much of this testing is a duplicate of MDL-37939 as we are just altering the SQL query in that upgrade step. Install a fresh 2.3 version just prior to 2012062504.08 1. Create a new course 2. Create 5-6 course modules in each of 3 sections. 3. Move course modules between sections. . 4. They will appear to move, but they will be broken sequence data Confirm that with SQL queries; SELECT id, section, sequence from mdl_course_sections where course = ? order by section asc; SELECT id, section from mdl_course_modules where course = ?; Refresh the page and make some more changes: Confirm that although the drag/drop appears to work, when you actually refresh the page it doesn't persist across a refresh 5. Now apply the patch and upgrade confirm that the CMs didn't appear to move from where they were before the upgrade confirm that the DB looks correct now (no duplicates in sequence data) move some CMs around sections refresh confirm that the section sequence data is correct confirm that the cm.section is correct On 2.4 and 2.5 1. Take a course with a fair few CMs 2. Apply the upgrade 3. Confirm that the CMs didn't appear to move from where they were before the upgrade 4. Run the SQL to confirm sequences are correct SELECT id, section, sequence from mdl_course_sections where course = ? order by section asc; SELECT id, section from mdl_course_modules where course = ?; Breaking&fixing test: By editing the DB directly create situations when fields course_modules.section and course_sections.sequence (inside the same course) do not match, for example: Module points to the wrong section Module points to the non-existing section (i.e. section 0) Module is present is several section sequences Module is present several times in its section sequence Module is not present in any section sequences Re-run the upgrade script and make sure it is fixed. Note that in 2.6 you will need to actually view the course to complete fixing. Hint, to re-run the upgrade script you can create a file testupgrade.php : <?php include "config.php"; require_once($CFG->libdir.'/db/upgradelib.php'); upgrade_course_modules_sequences(); purge_all_caches(); echo "Done.";
    • Affected Branches:
      MOODLE_23_STABLE, MOODLE_24_STABLE, MOODLE_25_STABLE, MOODLE_26_STABLE
    • Fixed Branches:
      MOODLE_24_STABLE, MOODLE_25_STABLE, MOODLE_26_STABLE
    • Pull Master Branch:
      wip-MDL-38228-master
    • Story Points (Obsolete):
      20

      Description

      The upgrade script portion of the fix for MDL-37939 is inefficient and causes a considerable performance problem for upgrading large sites. The process queries course modules for each course section individually. That's a pretty big deal for Moodlerooms, where our largest site has nearly 445,000 course sections, and an average of over 6,000 course sections across 1,200 sites.

      The root of the problem is finding the course modules which don't exist in the sequence field of the related course section. The following query will get those course section records for you.

      SELECT cs.*
      FROM mdl_course_sections cs
      INNER JOIN mdl_course_modules cm
        ON (cm.course = cs.course AND cm.section = cs.id)
      WHERE CONCAT(',', cs.sequence, ',') NOT LIKE CONCAT('%,', cm.id, ',%')
      

      The following returns an average of 33 course sections for affected sites (~80%), with a maximum of 772 course sections.

      Note: FIND_IN_SET is an operator that only works for MySQL and PG, but the query can be tweaked to support any RDBMS.

      Additonally: for 2.5 and 2.6 branches the new upgrade script has to be run again because of another bug. In version 2.4 we can just replace the old script with a faster one.

        Gliffy Diagrams

          Issue Links

            Activity

            Hide
            Kris Stokking added a comment -

            I strongly believe this is a blocker, but I'm only escalating to critical in case there's something I missed. I'm running on fumes after last night's quarterly dev meeting.

            Regardless, please expedite before MDL-37939 ships in the next stable release.

            Show
            Kris Stokking added a comment - I strongly believe this is a blocker, but I'm only escalating to critical in case there's something I missed. I'm running on fumes after last night's quarterly dev meeting. Regardless, please expedite before MDL-37939 ships in the next stable release.
            Hide
            Andrew Nicols added a comment -

            Passing to Dan as discussed

            Show
            Andrew Nicols added a comment - Passing to Dan as discussed
            Hide
            Dan Poltawski added a comment -

            Rather than FIND_IN_SET, could we do a LIKE?

            Show
            Dan Poltawski added a comment - Rather than FIND_IN_SET, could we do a LIKE?
            Hide
            Damyon Wiese added a comment - - edited

            select s.id, s.sequence from mdl_course_modules as m JOIN mdl_course_sections as s on m.section != s.id where concat(',', s.sequence, ',') like concat('%,', m.id, ',%');

            Show
            Damyon Wiese added a comment - - edited select s.id, s.sequence from mdl_course_modules as m JOIN mdl_course_sections as s on m.section != s.id where concat(',', s.sequence, ',') like concat('%,', m.id, ',%');
            Hide
            Damyon Wiese added a comment -

            I tweaked the upgrade query while fixing the regression on the linked issue.

            Show
            Damyon Wiese added a comment - I tweaked the upgrade query while fixing the regression on the linked issue.
            Hide
            Eloy Lafuente (stronk7) added a comment - - edited

            Yup, I think concat + like is the only cross-db possibility right now. An alternative would be regexp support in DBs but we haven't it enabled/supported for all them.

            An alternative would be to use sql_position() instead of LIKE with left wildchars. It may behave better for some databases (it uses internally POSITION()/INSTR()).

            Ciao

            Show
            Eloy Lafuente (stronk7) added a comment - - edited Yup, I think concat + like is the only cross-db possibility right now. An alternative would be regexp support in DBs but we haven't it enabled/supported for all them. An alternative would be to use sql_position() instead of LIKE with left wildchars. It may behave better for some databases (it uses internally POSITION()/INSTR()). Ciao
            Hide
            Kris Stokking added a comment -

            Fixing a typo in my query - cm.section should join against cs.id, not cs.section.

            Show
            Kris Stokking added a comment - Fixing a typo in my query - cm.section should join against cs.id, not cs.section.
            Hide
            Dan Poltawski added a comment -

            Assigning to Damyon as he is addressing it in MDL-37865.

            Show
            Dan Poltawski added a comment - Assigning to Damyon as he is addressing it in MDL-37865 .
            Hide
            Kris Stokking added a comment -

            Dan - I don't think MDL-37865 is the right ticket. I'm guessing it should have been MDL-38173, although I don't see a fix for this issue in that patch. Damyon, can you clarify?

            Show
            Kris Stokking added a comment - Dan - I don't think MDL-37865 is the right ticket. I'm guessing it should have been MDL-38173 , although I don't see a fix for this issue in that patch. Damyon, can you clarify?
            Hide
            Damyon Wiese added a comment -

            Hi Kris,

            For the first of those 2 fixes I don't think you can safely apply the LIKE clause without missing some corrupted modules.

            It is safer for the second upgrade step because the corruption is well defined.

            I know this is a slow upgrade step - but this is better than having unknown bad data hanging around.

            Show
            Damyon Wiese added a comment - Hi Kris, For the first of those 2 fixes I don't think you can safely apply the LIKE clause without missing some corrupted modules. It is safer for the second upgrade step because the corruption is well defined. I know this is a slow upgrade step - but this is better than having unknown bad data hanging around.
            Hide
            Kris Stokking added a comment -

            Damyon - apologies for missing your comment. When I looked into this previously I was looking at the following commit: https://github.com/moodle/moodle/commit/bf78144a45f9eb0971081c3730a67807b9292c44. However, you mentioned 2 fixes. Is there another?

            I know this is a slow upgrade step - but this is better than having unknown bad data hanging around.

            I would say that depends on your perspective. The upgrade process prevents usage, and this particular step of it could take an extremely long time on a large site. Since the script checks far more courses than are affected, a site administrator will have to wait an excessive amount of time to allow users re-entry into the LMS. In an ideal world, cleanup scripts such as this could (optionally) be moved outside of the upgrade process so that partners and large institutions with technical expertise could run it afterwards.

            Show
            Kris Stokking added a comment - Damyon - apologies for missing your comment. When I looked into this previously I was looking at the following commit: https://github.com/moodle/moodle/commit/bf78144a45f9eb0971081c3730a67807b9292c44 . However, you mentioned 2 fixes. Is there another? I know this is a slow upgrade step - but this is better than having unknown bad data hanging around. I would say that depends on your perspective. The upgrade process prevents usage, and this particular step of it could take an extremely long time on a large site. Since the script checks far more courses than are affected, a site administrator will have to wait an excessive amount of time to allow users re-entry into the LMS. In an ideal world, cleanup scripts such as this could (optionally) be moved outside of the upgrade process so that partners and large institutions with technical expertise could run it afterwards.
            Hide
            Damyon Wiese added a comment -

            Hi Kris,

            That's fine for diligent partners - but there are far more users who would blindly install updates and miss all those extra steps and so would get random, extremely hard to debug errors and 6 months later we would find ourselves in the same boat cleaning up bad data.

            Show
            Damyon Wiese added a comment - Hi Kris, That's fine for diligent partners - but there are far more users who would blindly install updates and miss all those extra steps and so would get random, extremely hard to debug errors and 6 months later we would find ourselves in the same boat cleaning up bad data.
            Hide
            Kris Stokking added a comment -

            Right, that's why I said optionally moved. We don't want to add a burden to non-technical users, but that doesn't mean we can't give experienced sys admins better capabilities for improving the upgrade experience. However, that's a feature request for a different day...

            I still don't see the 2 parts to the upgrade process. I only see the one, which I linked in the commit above, and that could definitely be improved by the SQL suggested above. How would the LIKE clause miss corrupted modules?

            Show
            Kris Stokking added a comment - Right, that's why I said optionally moved. We don't want to add a burden to non-technical users, but that doesn't mean we can't give experienced sys admins better capabilities for improving the upgrade experience. However, that's a feature request for a different day... I still don't see the 2 parts to the upgrade process. I only see the one, which I linked in the commit above, and that could definitely be improved by the SQL suggested above. How would the LIKE clause miss corrupted modules?
            Hide
            Tim Hunt added a comment -

            Since someone else thought this looked like a blocker, as well as me, I am upgrading it.

            There are suggestions above about improving the effiency of the SQL. Now 2.5 is out of the way, will someone be working on this again?

            Show
            Tim Hunt added a comment - Since someone else thought this looked like a blocker, as well as me, I am upgrading it. There are suggestions above about improving the effiency of the SQL. Now 2.5 is out of the way, will someone be working on this again?
            Hide
            Tim Hunt added a comment -

            I think this is a blocker, becuase on our sever, with 200,000 course sections, this setp of the upgrade is taking ~70 mins.

            Show
            Tim Hunt added a comment - I think this is a blocker, becuase on our sever, with 200,000 course sections, this setp of the upgrade is taking ~70 mins.
            Hide
            Sam Marshall added a comment -

            Note: We are going to work around this by commenting it out and possibly doing something similar manually after the upgrade, so this will shortly stop being a problem for us. But I agree that it definitely should be a blocker; this upgrade step shouldn't have got into a release.

            Show
            Sam Marshall added a comment - Note: We are going to work around this by commenting it out and possibly doing something similar manually after the upgrade, so this will shortly stop being a problem for us. But I agree that it definitely should be a blocker; this upgrade step shouldn't have got into a release.
            Hide
            Russell Smith added a comment - - edited

            So to try and understand the result set we are looking for;

            1. Look in all the course_sections
            2. Find any section that has a module ID, that's not in the sequence.

            Is that correct?

            I would say we don't need a query that returns 100% correct match. You just need something that constrains the results to 'close' enough. As long as you don't exclude too many records. It's fine if you have a few extras as the checking that's already in the code can run to confirm whether we need to update the sequence or not. I no longer have a database with any records of this type in it. So when I run a query to test, I get 0 records. On the plus side that query would result in a very fast upgrade in this section.

            I'm not sure how to generalize the || to all databases, but I believe the following catches the cases you want. Is anybody able to review it?

            SELECT cs.id, cs.sequence, cm.id
              FROM mdl_course_sections cs
              INNER JOIN mdl_course_modules cm
                ON (cm.course = cs.course AND cm.section = cs.id) 
             
               WHERE NOT (
                 -- Find a valid match for this module.  If one is not found, include it in the list
             
                 -- Item is in the middle of the sequence
                 cs.sequence LIKE '%,' || cm.id ||',%'
                 OR
                 -- Item is at the start of the sequence
                 cs.sequence LIKE cm.id ||',%'
                 OR 
                 -- Item is the only items in the sequence, add '' to type convert.
                 cs.sequence = cm.id || ''
                 OR 
                 -- Item is at the end of the sequence
                 cs.sequence LIKE '%,' || cm.id
              )
            ;
            

            On to run this query on environment, it takes 2s in PostgreSQL.

            mdl_course_sections: 130782
            mdl_course_modules: 529251

            Show
            Russell Smith added a comment - - edited So to try and understand the result set we are looking for; 1. Look in all the course_sections 2. Find any section that has a module ID, that's not in the sequence. Is that correct? I would say we don't need a query that returns 100% correct match. You just need something that constrains the results to 'close' enough. As long as you don't exclude too many records. It's fine if you have a few extras as the checking that's already in the code can run to confirm whether we need to update the sequence or not. I no longer have a database with any records of this type in it. So when I run a query to test, I get 0 records. On the plus side that query would result in a very fast upgrade in this section. I'm not sure how to generalize the || to all databases, but I believe the following catches the cases you want. Is anybody able to review it? SELECT cs.id, cs.sequence, cm.id FROM mdl_course_sections cs INNER JOIN mdl_course_modules cm ON (cm.course = cs.course AND cm.section = cs.id)   WHERE NOT ( -- Find a valid match for this module. If one is not found, include it in the list   -- Item is in the middle of the sequence cs.sequence LIKE '%,' || cm.id ||',%' OR -- Item is at the start of the sequence cs.sequence LIKE cm.id ||',%' OR -- Item is the only items in the sequence, add '' to type convert. cs.sequence = cm.id || '' OR -- Item is at the end of the sequence cs.sequence LIKE '%,' || cm.id ) ; On to run this query on environment, it takes 2s in PostgreSQL. mdl_course_sections: 130782 mdl_course_modules: 529251
            Hide
            Kris Stokking added a comment -

            Hi Russell - I've updated the query in the main description so that it doesn't use the FIND_IN_SET operator and is cross-db compatible. It should be functional equivalent to yours. I'm hoping that Damyon Wiese can provide an example for this comment:

            For the first of those 2 fixes I don't think you can safely apply the LIKE clause without missing some corrupted modules.

            I'm happy to improve the SQL once I have a better understanding.

            Show
            Kris Stokking added a comment - Hi Russell - I've updated the query in the main description so that it doesn't use the FIND_IN_SET operator and is cross-db compatible. It should be functional equivalent to yours. I'm hoping that Damyon Wiese can provide an example for this comment: For the first of those 2 fixes I don't think you can safely apply the LIKE clause without missing some corrupted modules. I'm happy to improve the SQL once I have a better understanding.
            Hide
            Russell Smith added a comment -

            My understanding of his comments is that MDL-37939 and then MDL-38173 are the 'first' and 'second' part he is referring to.

            I spent a lot of time studying MDL-37939 when it was first released and also again now. I would hesitate to rework the entire patch, I would expect the above query would just replace the following;

                if ($oldversion < 2012062504.08) {
                    // Retrieve the list of course_sections as a recordset to save memory
                    $coursesections = $DB->get_recordset('course_sections', null, 'course, id', 'id, course, sequence'); 
            
            

            with

                if ($oldversion < 2012062504.08) {
                    // Retrieve the list of course_sections as a recordset to save memory
                    $coursesections = $DB->get_recordset_sql("SELECT DISTINCT cs.id, cs.course, cs.sequence 
                                                              FROM mdl_course_sections cs
                                                              INNER JOIN mdl_course_modules cm
                                                                ON (cm.course = cs.course AND cm.section = cs.id)
                                                              WHERE CONCAT(',', cs.sequence, ',') NOT LIKE CONCAT('%,', cm.id, ',%')
                                                              ORDER BY cs.course, cs.id");
            

            That way we are just selecting course sections that are affected and still using the existing logic to correct those sequences. It may not provide 100% of the speed improvement but I expect it to provide 90%+ as we are reducing the input set by a large fraction.

            My study of the code and comments on MDL-37939 say the original code only adds missing items to the sequence. I agree with Kris that the updated SQL will indeed find all cases where a cm.id is not in a particular sequence.

            So we are both just waiting for final confirmation from Damyon Wiese.

            Show
            Russell Smith added a comment - My understanding of his comments is that MDL-37939 and then MDL-38173 are the 'first' and 'second' part he is referring to. I spent a lot of time studying MDL-37939 when it was first released and also again now. I would hesitate to rework the entire patch, I would expect the above query would just replace the following; if ($oldversion < 2012062504.08) { // Retrieve the list of course_sections as a recordset to save memory $coursesections = $DB->get_recordset('course_sections', null, 'course, id', 'id, course, sequence'); with if ($oldversion < 2012062504.08) { // Retrieve the list of course_sections as a recordset to save memory $coursesections = $DB->get_recordset_sql("SELECT DISTINCT cs.id, cs.course, cs.sequence FROM mdl_course_sections cs INNER JOIN mdl_course_modules cm ON (cm.course = cs.course AND cm.section = cs.id) WHERE CONCAT(',', cs.sequence, ',') NOT LIKE CONCAT('%,', cm.id, ',%') ORDER BY cs.course, cs.id"); That way we are just selecting course sections that are affected and still using the existing logic to correct those sequences. It may not provide 100% of the speed improvement but I expect it to provide 90%+ as we are reducing the input set by a large fraction. My study of the code and comments on MDL-37939 say the original code only adds missing items to the sequence. I agree with Kris that the updated SQL will indeed find all cases where a cm.id is not in a particular sequence. So we are both just waiting for final confirmation from Damyon Wiese.
            Hide
            Damyon Wiese added a comment -

            This issue was assigned to me automatically, however I will not be able to work on this issue in the immediate future. In order to create a truer sense of the state of this issue and to allow other developers to have chance to become involved, I am removing myself as the assignee of this issue.

            For more information, see http://docs.moodle.org/dev/Changes_to_issue_assignment

            Show
            Damyon Wiese added a comment - This issue was assigned to me automatically, however I will not be able to work on this issue in the immediate future. In order to create a truer sense of the state of this issue and to allow other developers to have chance to become involved, I am removing myself as the assignee of this issue. For more information, see http://docs.moodle.org/dev/Changes_to_issue_assignment
            Hide
            Russell Smith added a comment -

            Well Kris, looks like we are best just to write the patch and submit it for peer review. I'm busy for the next couple of days, I'll pick it up next week if you haven't had a chance to make the patch.

            Show
            Russell Smith added a comment - Well Kris, looks like we are best just to write the patch and submit it for peer review. I'm busy for the next couple of days, I'll pick it up next week if you haven't had a chance to make the patch.
            Hide
            Russell Smith added a comment -

            Well Kris, looks like we are best just to write the patch and submit it for peer review. I'm busy for the next couple of days, I'll pick it up next week if you haven't had a chance to make the patch.

            Show
            Russell Smith added a comment - Well Kris, looks like we are best just to write the patch and submit it for peer review. I'm busy for the next couple of days, I'll pick it up next week if you haven't had a chance to make the patch.
            Hide
            Russell Smith added a comment -

            Further information relating to the size of the database used to run queries

            A already clean working DB from 2.3.4+ was used to run the queries.

            mdl_course_sections 133,579
            mdl_course_modules 554,297

            New query Time: 1375.813 ms
            Results: 0 as it's a clean environment.

            On a clean environment this will produce an instant upgrade.
            On dirty environments, you will see only the exact number of broken sections processed.

            The original PHP code in the upgrade section uses a similar, but different mechanism to calculate if a particular section has been modified. The new SQL provides the addition of a course restriction which did not previously exist. This is safe as the original corruption didn't affect courses.
            In the environment above which experienced a few hundred corruptions, course was never affected. My reviews of the original code agree with the fact the course will be correct in all cases we are trying to correct.

            Show
            Russell Smith added a comment - Further information relating to the size of the database used to run queries A already clean working DB from 2.3.4+ was used to run the queries. mdl_course_sections 133,579 mdl_course_modules 554,297 New query Time: 1375.813 ms Results: 0 as it's a clean environment. On a clean environment this will produce an instant upgrade. On dirty environments, you will see only the exact number of broken sections processed. The original PHP code in the upgrade section uses a similar, but different mechanism to calculate if a particular section has been modified. The new SQL provides the addition of a course restriction which did not previously exist. This is safe as the original corruption didn't affect courses. In the environment above which experienced a few hundred corruptions, course was never affected. My reviews of the original code agree with the fact the course will be correct in all cases we are trying to correct.
            Hide
            Russell Smith added a comment -

            Sadly I only had a couple of hours to work on this today. I'm not sure how long before I'll be able to pick it up again.

            All of the code is available however I have not completed the upgrade testing. Only php linted the upgrade file and tested the SQL in an SQL editor to confirm the correct results.

            Anybody who is able to do the next bit, please pick the up and progress it further. When I get time to work on it further I will, however if you want this faster, please feel free to chip in.

            Show
            Russell Smith added a comment - Sadly I only had a couple of hours to work on this today. I'm not sure how long before I'll be able to pick it up again. All of the code is available however I have not completed the upgrade testing. Only php linted the upgrade file and tested the SQL in an SQL editor to confirm the correct results. Anybody who is able to do the next bit, please pick the up and progress it further. When I get time to work on it further I will, however if you want this faster, please feel free to chip in.
            Hide
            Petr Skoda added a comment - - edited

            Hello, please fix the whitespace in your SQL statements - the SELECT, FROM, JOIN, WHERE words should be all right aligned (see http://docs.moodle.org/dev/DB_layer_2.0_examples).

            Also please do not use INNER word there.

            Maybe we could set longer timeout before running the query, I suppose the concats might take a while to execute, right?

            Show
            Petr Skoda added a comment - - edited Hello, please fix the whitespace in your SQL statements - the SELECT, FROM, JOIN, WHERE words should be all right aligned (see http://docs.moodle.org/dev/DB_layer_2.0_examples ). Also please do not use INNER word there. Maybe we could set longer timeout before running the query, I suppose the concats might take a while to execute, right?
            Hide
            Russell Smith added a comment -

            I got this far, I may as well see if I can get it finished.

            Show
            Russell Smith added a comment - I got this far, I may as well see if I can get it finished.
            Hide
            Petr Skoda added a comment -

            one more thing, the SQL statements are usually in double quotes "", it helps with SQL syntax highlighting in some IDEs

            Show
            Petr Skoda added a comment - one more thing, the SQL statements are usually in double quotes "", it helps with SQL syntax highlighting in some IDEs
            Hide
            Russell Smith added a comment -

            I've yet to complete the last comment about ' vs ".

            However running the query on mysql only took 14 seconds on my untuned system with heavily corrupted data due to the column order issue between postgresql and mysql when restoring. That is with the same dataset that PostgreSQL had. So I do not believe performance to be an issue.

            Redoing the data to ensure the summary field had the correct data resulted in an execution time of 23 seconds so I don't believe up-ing the upgrade time is required.

            I hope to have time tomorrow to correct the outstanding review item before sending for official peer review.

            Show
            Russell Smith added a comment - I've yet to complete the last comment about ' vs ". However running the query on mysql only took 14 seconds on my untuned system with heavily corrupted data due to the column order issue between postgresql and mysql when restoring. That is with the same dataset that PostgreSQL had. So I do not believe performance to be an issue. Redoing the data to ensure the summary field had the correct data resulted in an execution time of 23 seconds so I don't believe up-ing the upgrade time is required. I hope to have time tomorrow to correct the outstanding review item before sending for official peer review.
            Hide
            Petr Skoda added a comment -

            great, thanks for the info, it is exactly what we need

            Show
            Petr Skoda added a comment - great, thanks for the info, it is exactly what we need
            Hide
            Russell Smith added a comment -

            Okay, I've made the SQL changes as requested. I think this is ready for formal peer review.

            I've assigned Petr as Peer Reviewer, he seems to be doing the review already. If you don't wish to peer review, please unassign yourself.

            Show
            Russell Smith added a comment - Okay, I've made the SQL changes as requested. I think this is ready for formal peer review. I've assigned Petr as Peer Reviewer, he seems to be doing the review already. If you don't wish to peer review, please unassign yourself.
            Hide
            Kris Stokking added a comment -

            My apologies for dropping off of this ticket, huge thanks to Russell and Petr for carrying the torch! I ran the suggested fix across ~1,300 sites and the worst offender was only 131 seconds while the average was just around 1 second. I'm in the process of upgrade testing for our upcoming release (hence the radio silence), and I should be able to ensure that the affected data is properly identified.

            Show
            Kris Stokking added a comment - My apologies for dropping off of this ticket, huge thanks to Russell and Petr for carrying the torch! I ran the suggested fix across ~1,300 sites and the worst offender was only 131 seconds while the average was just around 1 second. I'm in the process of upgrade testing for our upcoming release (hence the radio silence), and I should be able to ensure that the affected data is properly identified.
            Hide
            Petr Skoda added a comment -

            Kris: Thanks for the info.

            Russell:
            1/ WHERE ".$sequenceconcat." NOT LIKE ".$moduleconcat." should not use the quotes
            2/ the linked diffs show a bit more commits
            3/ you should probably squash the commits to improve the git history

            Show
            Petr Skoda added a comment - Kris: Thanks for the info. Russell: 1/ WHERE ".$sequenceconcat." NOT LIKE ".$moduleconcat." should not use the quotes 2/ the linked diffs show a bit more commits 3/ you should probably squash the commits to improve the git history
            Hide
            Russell Smith added a comment -

            Okay, 1-3 all done. Hopefully this is the last go around!

            Show
            Russell Smith added a comment - Okay, 1-3 all done. Hopefully this is the last go around!
            Hide
            Petr Skoda added a comment -

            looks ok, submitting for integration, big thanks!

            Show
            Petr Skoda added a comment - looks ok, submitting for integration, big thanks!
            Hide
            Dan Poltawski added a comment -

            Hi Everyone,

            The number of regressions/fixes in this chain of issues really emphasises how nuanced this issue is, so to respond to the comments about the performance of this upgrade step, it was tricky and multiple times in this chain of issue we ended up upgrade steps which caused corruption or didn't quite fix the underlying issue. In this context, having the upgrade step be slow but conservative was a safe way forward. I'd much rather having slow upgrades then massive corruption among Moodle installs.

            But of course, if we can improve it thats great, and thanks a lot for taking this on Russell!

            I'm really trying to get my head back into this group of issues to understand them and ensure we're not missing anything, could you explain why you've added wildcard operators around moduleconcat match? Is there a reason why we should ignore partial matches?

            thanks,
            Dan

            Show
            Dan Poltawski added a comment - Hi Everyone, The number of regressions/fixes in this chain of issues really emphasises how nuanced this issue is, so to respond to the comments about the performance of this upgrade step, it was tricky and multiple times in this chain of issue we ended up upgrade steps which caused corruption or didn't quite fix the underlying issue. In this context, having the upgrade step be slow but conservative was a safe way forward. I'd much rather having slow upgrades then massive corruption among Moodle installs. But of course, if we can improve it thats great, and thanks a lot for taking this on Russell! I'm really trying to get my head back into this group of issues to understand them and ensure we're not missing anything, could you explain why you've added wildcard operators around moduleconcat match? Is there a reason why we should ignore partial matches? thanks, Dan
            Hide
            Russell Smith added a comment -

            The complexity is one of the reasons I took this up. We spent hours investigating, understanding and resolving this issue in our environment. So I felt qualified to work on it. I agree getting your head around it is very complicated. The long thread above shows that to be true.

            The issue with slow is that it's not just a bit slow. 24 hours+ for a minor patch upgrade isn't really acceptable. That is the performance Kris is seeing on some large environments. Even in they are 100% correct. So I'd vote that it needs some improvement. As I think I mentioned earlier in the comments, I've only adjusted the SQL query to limit the possibility of regression from applying the fix. Playing with the code below would require significantly more testing. So that was avoided even though we won't end up with 100% performance gain.

            When we are matching we need to ensure we only match a full sequence id. Without ,'s around the cm.id you'll get partial matches;
            cm.id 1 would match sequences with a "1" at any point in the text. We need to make sure it only matches if it's the full value.

            Example:

            sequence should be: 1,10,65,105
            sequence currently is: 1,65,105

            Now we run the query without extra comma's on cm.id and we search for '10' to see if it's in the sequence. The invalid sequence contains '10' as part of '105'. So the row would not be returned. This is even though the sequence is corrupt and missing elements.

            Under the additional comma's use, we are going to look for ',10,' and that will not match 105.

            As the query is a NOT LIKE query, partial extra matches will reduce the output set, not increase it. So you can only end up missing options you should have processed.

            Now we've established why you add the comma's to cm.id; why do we add them to cs.sequence. That is because if the first or last item in the sequence is being searched, it doesn't have 2 commas. So in the example sequences, we make them; ,1,10,65,105, to ensure you can search for cm.id with comma's on either side and still get the correct results.

            There are a number of other ways to write this query in various different databases. However the one presented is the one that is portable. It's not necessarily the easiest to understand.

            A final note in this comment; The comments above the SQL are important to understand. As the PHP code only looks for missing items in the sequence, no other type of corruption. So they query also looks for no other type of corruption.

            So ,105, will not match ,10, however it will match 10.

            What we are looking for in this query is; Modules that are in the course_modules tables, but do not appear in the course_section sequence. They are out of sync. The PHP code below only searches for 'additions' and never looks for removals. So in all cases we are only interested in complete matches of cm.id numbers. Partial matches are feeding invalid data into the query. The result of that would be that we would match 10 -> 105 and not process the fact that cm.id 10 is missing from the sequence when it really should be in there.

            Show
            Russell Smith added a comment - The complexity is one of the reasons I took this up. We spent hours investigating, understanding and resolving this issue in our environment. So I felt qualified to work on it. I agree getting your head around it is very complicated. The long thread above shows that to be true. The issue with slow is that it's not just a bit slow. 24 hours+ for a minor patch upgrade isn't really acceptable. That is the performance Kris is seeing on some large environments. Even in they are 100% correct. So I'd vote that it needs some improvement. As I think I mentioned earlier in the comments, I've only adjusted the SQL query to limit the possibility of regression from applying the fix. Playing with the code below would require significantly more testing. So that was avoided even though we won't end up with 100% performance gain. When we are matching we need to ensure we only match a full sequence id. Without ,'s around the cm.id you'll get partial matches; cm.id 1 would match sequences with a "1" at any point in the text. We need to make sure it only matches if it's the full value. Example: sequence should be: 1,10,65,105 sequence currently is: 1,65,105 Now we run the query without extra comma's on cm.id and we search for '10' to see if it's in the sequence. The invalid sequence contains '10' as part of '105'. So the row would not be returned. This is even though the sequence is corrupt and missing elements. Under the additional comma's use, we are going to look for ',10,' and that will not match 105. As the query is a NOT LIKE query, partial extra matches will reduce the output set, not increase it. So you can only end up missing options you should have processed. Now we've established why you add the comma's to cm.id; why do we add them to cs.sequence. That is because if the first or last item in the sequence is being searched, it doesn't have 2 commas. So in the example sequences, we make them; ,1,10,65,105, to ensure you can search for cm.id with comma's on either side and still get the correct results. There are a number of other ways to write this query in various different databases. However the one presented is the one that is portable. It's not necessarily the easiest to understand. A final note in this comment; The comments above the SQL are important to understand. As the PHP code only looks for missing items in the sequence, no other type of corruption. So they query also looks for no other type of corruption. So ,105, will not match ,10, however it will match 10. What we are looking for in this query is; Modules that are in the course_modules tables, but do not appear in the course_section sequence. They are out of sync. The PHP code below only searches for 'additions' and never looks for removals. So in all cases we are only interested in complete matches of cm.id numbers. Partial matches are feeding invalid data into the query. The result of that would be that we would match 10 -> 105 and not process the fact that cm.id 10 is missing from the sequence when it really should be in there.
            Hide
            Dan Poltawski added a comment -

            Hi Russel,

            Thanks a lot for explaining in detail, in fact I realised after you've explained that I was misreading this incorrectly, thinking it was a self-constructed sequence, but in fact its just one item in the sequence.

            However, I have just noticed that course_sections.sequence is a TEXT field. We can't use DISTINCT on text fields as this is not support by MSSQL or Oracle: http://docs.moodle.org/dev/XMLDB_Modifying_DML_functions#DISTINCT_clause_on_TEXT.2FBINARY

            I've thought about the genereal approach to the logic, but I can't think of a problem with it. So if we can overcome this somehow I think this will be a good solution. (Although, many eyes etc)

            Show
            Dan Poltawski added a comment - Hi Russel, Thanks a lot for explaining in detail, in fact I realised after you've explained that I was misreading this incorrectly, thinking it was a self-constructed sequence, but in fact its just one item in the sequence. However, I have just noticed that course_sections.sequence is a TEXT field. We can't use DISTINCT on text fields as this is not support by MSSQL or Oracle: http://docs.moodle.org/dev/XMLDB_Modifying_DML_functions#DISTINCT_clause_on_TEXT.2FBINARY I've thought about the genereal approach to the logic, but I can't think of a problem with it. So if we can overcome this somehow I think this will be a good solution. (Although, many eyes etc)
            Hide
            CiBoT added a comment -

            Moving this reopened issue out from current integration. Please, re-submit it for integration once ready.

            Show
            CiBoT added a comment - Moving this reopened issue out from current integration. Please, re-submit it for integration once ready.
            Hide
            Kris Stokking added a comment -

            Good catch Dan. I think we can remove the DISTINCT clause and the query will work just fine, it will just process more records than it should. That number should still be far less than processing for all course sections. Or we could even check for distinction within Moodle as the result set is not terribly complex.

            The upgrade code leaves a ton of room for optimization that I don't think has ever really been looked at because A) the current code works and B) most institutions aren't upgrading hundreds of sites at a time like Moodlerooms is. Another route I thought of was to defer cleanup and possibly even optimization type tasks until after the main upgrade process has completed, which would reduce the total time to upgrade. This type setup would obviously need to be configurable, and would be turned off by default for backwards compatibility, but would be a huge boon to large institutions and partners. It would also mitigate some of the problems with long running upgrade tasks. I'll need to come up with something more formal, but since we have so many great eyes on this ticket I thought I would share with the group to get initial feedback.

            On a related note, I ran into a bit of a snag with upgrade testing that I haven't been able to sort out yet. I should have results by Wednesday.

            Show
            Kris Stokking added a comment - Good catch Dan. I think we can remove the DISTINCT clause and the query will work just fine, it will just process more records than it should. That number should still be far less than processing for all course sections. Or we could even check for distinction within Moodle as the result set is not terribly complex. The upgrade code leaves a ton of room for optimization that I don't think has ever really been looked at because A) the current code works and B) most institutions aren't upgrading hundreds of sites at a time like Moodlerooms is. Another route I thought of was to defer cleanup and possibly even optimization type tasks until after the main upgrade process has completed, which would reduce the total time to upgrade. This type setup would obviously need to be configurable, and would be turned off by default for backwards compatibility, but would be a huge boon to large institutions and partners. It would also mitigate some of the problems with long running upgrade tasks. I'll need to come up with something more formal, but since we have so many great eyes on this ticket I thought I would share with the group to get initial feedback. On a related note, I ran into a bit of a snag with upgrade testing that I haven't been able to sort out yet. I should have results by Wednesday.
            Hide
            Russell Smith added a comment -

            Okay, after reading about ways of getting around distinct, I believe using get_records_sql without the distinct is what we want.

            We are looking for course_sections, which id is selected, which is unique. So any duplicates will be overwritten in the query.

            I reran the query on MySQL with out DISTINCT on my hardware, with data volumes as before and got 36.89 seconds for an empty result set.

            Rebased branches are now available using get_records_sql, which will automatically remove the duplicates unless I'm reading that code incorrectly.

            Can somebody peer review/whatever is needed to get this back into this week or next week's integration cycle. Next week might be better if Kris is able to give more test results on Wednesday. I'm sure Dan will feel better about applying it with more test results

            Show
            Russell Smith added a comment - Okay, after reading about ways of getting around distinct, I believe using get_records_sql without the distinct is what we want. We are looking for course_sections, which id is selected, which is unique. So any duplicates will be overwritten in the query. I reran the query on MySQL with out DISTINCT on my hardware, with data volumes as before and got 36.89 seconds for an empty result set. Rebased branches are now available using get_records_sql, which will automatically remove the duplicates unless I'm reading that code incorrectly. Can somebody peer review/whatever is needed to get this back into this week or next week's integration cycle. Next week might be better if Kris is able to give more test results on Wednesday. I'm sure Dan will feel better about applying it with more test results
            Hide
            Russell Smith added a comment -

            Dan P says using get_records_sql is not acceptable, so back to the updated query which will require further performance testing. I'll aim to have it up in a couple of days.

            Show
            Russell Smith added a comment - Dan P says using get_records_sql is not acceptable, so back to the updated query which will require further performance testing. I'll aim to have it up in a couple of days.
            Hide
            Russell Smith added a comment -

            I've not worked on this for 2 weeks, I'm on holidays at the moment and it's unlikely I will return to it soon. Please feel free to pick it up if you are able.

            Show
            Russell Smith added a comment - I've not worked on this for 2 weeks, I'm on holidays at the moment and it's unlikely I will return to it soon. Please feel free to pick it up if you are able.
            Hide
            Kris Stokking added a comment -

            Apologies Russell - I'm waiting for the test environment to get setup correctly, and it's taken longer to do so than I had originally planned. It has not fallen off my radar.

            Show
            Kris Stokking added a comment - Apologies Russell - I'm waiting for the test environment to get setup correctly, and it's taken longer to do so than I had originally planned. It has not fallen off my radar.
            Hide
            Ankit Agarwal added a comment -

            I am taking this issue over, as it doesn't seem Russell will be able to work on it in near future.

            Show
            Ankit Agarwal added a comment - I am taking this issue over, as it doesn't seem Russell will be able to work on it in near future.
            Hide
            Ankit Agarwal added a comment - - edited

            Hi,
            I took up the patch that Russell wrote and came up with some tests for it, to verify if it is considering all possible cases.
            https://github.com/ankitagarwal/moodle/compare/moodle:master...ankitagarwal:MDL-38228-master

            Out of 20 broken sequences that I generated in my tests, the SQL was able to recognise only 5. The SQL considers only the case of "missing cmids" in the sequence. If we consider this is the only possible corruption, the patch works. However, there can be repeated ids, incorrect ids etc as well (MDL-37939). which are completely ignored by the SQL, which were fixed by the previous code. I have added a second commit that modifies the sql to identify sequences with repeated cmids. Am not sure that, there is a cross DB way of identifying all possible corrupt records in a single SQL statement.

            Marina has been working on MDL-37028 , which fixes this issue on the fly. Also there is a proposal of having a cli tool for this. I suggest we leave this upgrade path as it is, since slow upgrade is better than upgrade that leaves broken data behind. If massive sites are really worried about this, they can manually comment this out and use the proposed cli tool of MDL-37028 as and when they need it.

            Thanks

            Show
            Ankit Agarwal added a comment - - edited Hi, I took up the patch that Russell wrote and came up with some tests for it, to verify if it is considering all possible cases. https://github.com/ankitagarwal/moodle/compare/moodle:master...ankitagarwal:MDL-38228-master Out of 20 broken sequences that I generated in my tests, the SQL was able to recognise only 5. The SQL considers only the case of "missing cmids" in the sequence. If we consider this is the only possible corruption, the patch works. However, there can be repeated ids, incorrect ids etc as well ( MDL-37939 ). which are completely ignored by the SQL, which were fixed by the previous code. I have added a second commit that modifies the sql to identify sequences with repeated cmids. Am not sure that, there is a cross DB way of identifying all possible corrupt records in a single SQL statement. Marina has been working on MDL-37028 , which fixes this issue on the fly. Also there is a proposal of having a cli tool for this. I suggest we leave this upgrade path as it is, since slow upgrade is better than upgrade that leaves broken data behind. If massive sites are really worried about this, they can manually comment this out and use the proposed cli tool of MDL-37028 as and when they need it. Thanks
            Hide
            Marina Glancy added a comment -

            After MDL-37028 is integrated this upgrade script may just be replaced with clearing course cache (setting

            {course}

            .modinfo to null)

            Show
            Marina Glancy added a comment - After MDL-37028 is integrated this upgrade script may just be replaced with clearing course cache (setting {course} .modinfo to null)
            Hide
            Damyon Wiese added a comment -

            This is not just fixing the caches - so I don't think it is that easy Marina. It is fixing the course_modules tables and course_sections tables.

            Show
            Damyon Wiese added a comment - This is not just fixing the caches - so I don't think it is that easy Marina. It is fixing the course_modules tables and course_sections tables.
            Hide
            Marina Glancy added a comment -

            In MDL-37028 I check and fix these tables during re-generation of the course cache

            Show
            Marina Glancy added a comment - In MDL-37028 I check and fix these tables during re-generation of the course cache
            Hide
            Kris Stokking added a comment -

            Ankit - as far as I can tell, you've tested a bunch of use cases that wouldn't have been caused by the same bug that this issue is trying to address. Regardless:

            1. I am very much in favor of moving the code into an admin tool for data integrity checks - in fact, I would like to see more cleanup scripts moved out of the upgrade process (optionally) and into admin tools, unless they absolutely need to be fixed as part of the upgrade. This significantly reduces the overall downtime required for upgrades.

            2. The admin tool suffers from similar scalability problems that this script does. We really should be considering large data sets. It would be far better to identify problematic records with a (relatively) fixed set of optimized database queries than to pull all possible data and inspect it within PHP.

            Show
            Kris Stokking added a comment - Ankit - as far as I can tell, you've tested a bunch of use cases that wouldn't have been caused by the same bug that this issue is trying to address. Regardless: 1. I am very much in favor of moving the code into an admin tool for data integrity checks - in fact, I would like to see more cleanup scripts moved out of the upgrade process (optionally) and into admin tools, unless they absolutely need to be fixed as part of the upgrade. This significantly reduces the overall downtime required for upgrades. 2. The admin tool suffers from similar scalability problems that this script does. We really should be considering large data sets. It would be far better to identify problematic records with a (relatively) fixed set of optimized database queries than to pull all possible data and inspect it within PHP.
            Hide
            moodle.com added a comment -

            Assigning this to Marina as she was working on MDL-37028.

            Show
            moodle.com added a comment - Assigning this to Marina as she was working on MDL-37028 .
            Hide
            Marina Glancy added a comment -

            Hehe, both scripts - the one suggested in MDL-37939 and this one can break the DB completely.

            Try the following:

            1. Create a normal course with a course module in it
            2. Manually set in DB course_module.section=0 for this module
            3. Even if the course cache is rebuilt after that (for example edit course settings), the module is still displayed.
            4. Run upgrade script - module disappeared
            • it is not displayed on the course view page
            • it is not displayed on overview page of this activity type (for example /mod/quiz/index.php?id=COURSEID)
            • if you type URL /mod/quiz/view.php?id=COURSEMODULEID or /course/modedit.php?update=COURSEMODULEID it results with an error because function require_login() also calls get_fast_modinfo() and would not set page cm if it does not exist in $modinfo->cms.
            • even worse, /course/mod.php?delete=COURSEMODULEID would not delete it either

            P.S. Try this on 2.5 or below, on master this situation is automatically fixed by course integrity check added in MDL-37028

            Show
            Marina Glancy added a comment - Hehe, both scripts - the one suggested in MDL-37939 and this one can break the DB completely. Try the following: Create a normal course with a course module in it Manually set in DB course_module.section=0 for this module Even if the course cache is rebuilt after that (for example edit course settings), the module is still displayed. Run upgrade script - module disappeared it is not displayed on the course view page it is not displayed on overview page of this activity type (for example /mod/quiz/index.php?id=COURSEID) if you type URL /mod/quiz/view.php?id=COURSEMODULEID or /course/modedit.php?update=COURSEMODULEID it results with an error because function require_login() also calls get_fast_modinfo() and would not set page cm if it does not exist in $modinfo->cms. even worse, /course/mod.php?delete=COURSEMODULEID would not delete it either P.S. Try this on 2.5 or below, on master this situation is automatically fixed by course integrity check added in MDL-37028
            Hide
            Marina Glancy added a comment -

            Ok, ready for peer review whoever feels brave.
            I will create branches for 2.3-2.4 the same as 2.5 after the review.

            Since the previous upgrade script unfortunately broke stuff instead of fixing I had to add a new upgrade script instead of replacing the old one (but commented out the previous one).

            Also added a configuration variable that prevents from running the same script over when you upgrade to 2.4 and then to 2.5.

            Please note that script for 2.3-2.5 is different from 2.6 because in 2.6 we a) don't need to reset course cache b) don't worry about duplicated references in course sections as they are fixed on-the-fly

            If everything is OK in the DB there will be 1 query in 2.6 and 2 queries in 2.3-2.5. The queries are quite big, they analyse the whole tables course_modules and course_sections but at least it's all done at the same time.

            Show
            Marina Glancy added a comment - Ok, ready for peer review whoever feels brave. I will create branches for 2.3-2.4 the same as 2.5 after the review. Since the previous upgrade script unfortunately broke stuff instead of fixing I had to add a new upgrade script instead of replacing the old one (but commented out the previous one). Also added a configuration variable that prevents from running the same script over when you upgrade to 2.4 and then to 2.5. Please note that script for 2.3-2.5 is different from 2.6 because in 2.6 we a) don't need to reset course cache b) don't worry about duplicated references in course sections as they are fixed on-the-fly If everything is OK in the DB there will be 1 query in 2.6 and 2 queries in 2.3-2.5. The queries are quite big, they analyse the whole tables course_modules and course_sections but at least it's all done at the same time.
            Hide
            Tim Hunt added a comment -

            I would add another bit to the testing instructions: "Create a site with 10,000 courses (use David Monllao's scripts I guess). ... more set-up as necessary ... . Do the upgrade through your web browser. Ensure it does not time out."

            Actually, for the OU, the upgrade to 2.4 broke with just 2,000 courses.

            But, since this is a performance issue, surely we should test the performance aspect.

            Show
            Tim Hunt added a comment - I would add another bit to the testing instructions: "Create a site with 10,000 courses (use David Monllao's scripts I guess). ... more set-up as necessary ... . Do the upgrade through your web browser. Ensure it does not time out." Actually, for the OU, the upgrade to 2.4 broke with just 2,000 courses. But, since this is a performance issue, surely we should test the performance aspect.
            Hide
            Marina Glancy added a comment - - edited

            that's a good idea Tim. Actually more important is the total number of modules (the bigger the better) and sections-per-course (also the bigger the better).

            BTW you are welcome to test the SQL query if you have a big DB (just the query that finds the broken modules, not the whole script fixing them).

            <?php
            include "config.php";
             
            $starttime = microtime(true);
            $sequenceconcat = $DB->sql_concat("','", "s.sequence", "','");
            $moduleconcat = $DB->sql_concat("'%,'", "m.id", "',%'");
            $sql = "SELECT m.id, m.course, m.section, s.sequence
                FROM {course_modules} m LEFT OUTER JOIN {course_sections} s
                ON m.course = s.course and m.section = s.id
                WHERE s.sequence IS NULL OR ($sequenceconcat NOT LIKE $moduleconcat)
                ORDER BY m.course";
            $rows = $DB->get_records_sql($sql);
            $endtime = microtime(true);
            printf("Query 1 took %.4f s and returned %d rows<br>", $endtime-$starttime, count($rows));
             
             
            $starttime = microtime(true);
            $sequenceconcat = $DB->sql_concat("','", "s.sequence", "','");
            $moduleconcat = $DB->sql_concat("'%,'", "m.id", "',%'");
            $moduleconcatdup1 = $DB->sql_concat("'%,'", "m.id", "','", "m.id", "',%'");
            $moduleconcatdup2 = $DB->sql_concat("'%,'", "m.id", "',%,'", "m.id", "',%'");
            $sql = "SELECT m.id, m.course, m.section, s.id AS sectionid, s.sequence
                FROM {course_modules} m JOIN {course_sections} s
                ON m.course = s.course AND
                (
                    (m.section <> s.id AND $sequenceconcat LIKE $moduleconcat)
                    OR
                    (m.section = s.id AND $sequenceconcat LIKE $moduleconcatdup1)
                    OR
                    (m.section = s.id AND $sequenceconcat LIKE $moduleconcatdup2)
                )";
            $rows = $DB->get_records_sql($sql);
            $endtime = microtime(true);
            printf("Query 2 took %.4f s and returned %d rows<br>", $endtime-$starttime, count($rows));
            

            Show
            Marina Glancy added a comment - - edited that's a good idea Tim. Actually more important is the total number of modules (the bigger the better) and sections-per-course (also the bigger the better). BTW you are welcome to test the SQL query if you have a big DB (just the query that finds the broken modules, not the whole script fixing them). <?php include "config.php";   $starttime = microtime(true); $sequenceconcat = $DB->sql_concat("','", "s.sequence", "','"); $moduleconcat = $DB->sql_concat("'%,'", "m.id", "',%'"); $sql = "SELECT m.id, m.course, m.section, s.sequence FROM {course_modules} m LEFT OUTER JOIN {course_sections} s ON m.course = s.course and m.section = s.id WHERE s.sequence IS NULL OR ($sequenceconcat NOT LIKE $moduleconcat) ORDER BY m.course"; $rows = $DB->get_records_sql($sql); $endtime = microtime(true); printf("Query 1 took %.4f s and returned %d rows<br>", $endtime-$starttime, count($rows));     $starttime = microtime(true); $sequenceconcat = $DB->sql_concat("','", "s.sequence", "','"); $moduleconcat = $DB->sql_concat("'%,'", "m.id", "',%'"); $moduleconcatdup1 = $DB->sql_concat("'%,'", "m.id", "','", "m.id", "',%'"); $moduleconcatdup2 = $DB->sql_concat("'%,'", "m.id", "',%,'", "m.id", "',%'"); $sql = "SELECT m.id, m.course, m.section, s.id AS sectionid, s.sequence FROM {course_modules} m JOIN {course_sections} s ON m.course = s.course AND ( (m.section <> s.id AND $sequenceconcat LIKE $moduleconcat) OR (m.section = s.id AND $sequenceconcat LIKE $moduleconcatdup1) OR (m.section = s.id AND $sequenceconcat LIKE $moduleconcatdup2) )"; $rows = $DB->get_records_sql($sql); $endtime = microtime(true); printf("Query 2 took %.4f s and returned %d rows<br>", $endtime-$starttime, count($rows));
            Hide
            Tim Hunt added a comment -

            Good idea. I will try to run the query against a copy of our live DB today.

            And, we do have some courses with ridiculously large numbers of sections. (Thank you subpage ).

            Show
            Tim Hunt added a comment - Good idea. I will try to run the query against a copy of our live DB today. And, we do have some courses with ridiculously large numbers of sections. (Thank you subpage ).
            Hide
            Tim Hunt added a comment -

            SELECT m.id, m.course, m.section, s.sequence
                FROM mdl_course_modules m LEFT OUTER JOIN mdl_course_sections s
                ON m.course = s.course and m.section = s.id
                WHERE s.sequence IS NULL OR (',' || s.sequence || ',' NOT LIKE '%,' || m.id || ',%')
                ORDER BY m.course
            

            (We are on postgres) took 711 ms. 279 rows returned.

            SELECT m.id, m.course, m.section, s.id AS sectionid, s.sequence
                FROM mdl_course_modules m JOIN mdl_course_sections s
                ON m.course = s.course AND
                (
                    (m.section <> s.id AND ',' || s.sequence || ',' LIKE '%,' || m.id || ',%')
                    OR
                    (m.section = s.id AND ',' || s.sequence || ',' LIKE '%,' || m.id || ',' || m.id || ',%')
                    OR
                    (m.section = s.id AND ',' || s.sequence || ',' LIKE '%,' || m.id || ',%,' || m.id || ',%')
                )
            

            45,271 ms, (~45 seconds) 2 rows returned.

            Is that high enough to be a worry, you are withing a factor of 10 of a browser time-out?

            mdl_course_modules: 299,383 rows.
            mdl_course_sections: 222,703 rows.

            The explain on the slow query is

            Merge Join  (cost=145.53..3686020.54 rows=192143 width=45)
              Merge Cond: (s.course = m.course)
              Join Filter: (((m.section <> s.id) AND (((','::text || s.sequence) || ','::text) ~~ (('%,'::text || (m.id)::text) || ',%'::text))) OR ((m.section = s.id) AND (((','::text || s.sequence) || ','::text) ~~ (((('%,'::text || (m.id)::text) || ','::text) || (m.id)::text) || ',%'::text))) OR ((m.section = s.id) AND (((','::text || s.sequence) || ','::text) ~~ (((('%,'::text || (m.id)::text) || ',%,'::text) || (m.id)::text) || ',%'::text))))
              ->  Index Scan using mdl_coursect_cousec_uix on mdl_course_sections s  (cost=0.00..15827.94 rows=222703 width=29)
              ->  Materialize  (cost=0.00..19693.22 rows=299383 width=24)
                    ->  Index Scan using mdl_courmodu_cou_ix on mdl_course_modules m  (cost=0.00..18944.76 rows=299383 width=24)
            

            Show
            Tim Hunt added a comment - SELECT m.id, m.course, m. section , s. sequence FROM mdl_course_modules m LEFT OUTER JOIN mdl_course_sections s ON m.course = s.course and m. section = s.id WHERE s. sequence IS NULL OR ( ',' || s. sequence || ',' NOT LIKE '%,' || m.id || ',%' ) ORDER BY m.course (We are on postgres) took 711 ms. 279 rows returned. SELECT m.id, m.course, m. section , s.id AS sectionid, s. sequence FROM mdl_course_modules m JOIN mdl_course_sections s ON m.course = s.course AND ( (m. section <> s.id AND ',' || s. sequence || ',' LIKE '%,' || m.id || ',%' ) OR (m. section = s.id AND ',' || s. sequence || ',' LIKE '%,' || m.id || ',' || m.id || ',%' ) OR (m. section = s.id AND ',' || s. sequence || ',' LIKE '%,' || m.id || ',%,' || m.id || ',%' ) ) 45,271 ms, (~45 seconds) 2 rows returned. Is that high enough to be a worry, you are withing a factor of 10 of a browser time-out? mdl_course_modules: 299,383 rows. mdl_course_sections: 222,703 rows. The explain on the slow query is Merge Join (cost=145.53..3686020.54 rows=192143 width=45) Merge Cond: (s.course = m.course) Join Filter: (((m.section <> s.id) AND (((','::text || s.sequence) || ','::text) ~~ (('%,'::text || (m.id)::text) || ',%'::text))) OR ((m.section = s.id) AND (((','::text || s.sequence) || ','::text) ~~ (((('%,'::text || (m.id)::text) || ','::text) || (m.id)::text) || ',%'::text))) OR ((m.section = s.id) AND (((','::text || s.sequence) || ','::text) ~~ (((('%,'::text || (m.id)::text) || ',%,'::text) || (m.id)::text) || ',%'::text)))) -> Index Scan using mdl_coursect_cousec_uix on mdl_course_sections s (cost=0.00..15827.94 rows=222703 width=29) -> Materialize (cost=0.00..19693.22 rows=299383 width=24) -> Index Scan using mdl_courmodu_cou_ix on mdl_course_modules m (cost=0.00..18944.76 rows=299383 width=24)
            Hide
            Kris Stokking added a comment -

            MySQL 5.5:

            SELECT m.id, m.course, m.section, s.sequence
                FROM mdl_course_modules m LEFT OUTER JOIN mdl_course_sections s
                ON m.course = s.course and m.section = s.id
                WHERE s.sequence IS NULL OR (CONCAT(',', s.sequence, ',') NOT LIKE CONCAT('%,', m.id, ',%'))
                ORDER BY m.course
            

            407 rows, 2.22 seconds

            SELECT m.id, m.course, m.section, s.id AS sectionid, s.sequence
                FROM mdl_course_modules m JOIN mdl_course_sections s
                ON m.course = s.course AND
                (
                    (m.section <> s.id AND CONCAT(',', s.sequence, ',') LIKE CONCAT('%,', m.id, ',%'))
                    OR
                    (m.section = s.id AND CONCAT(',', s.sequence, ',') LIKE CONCAT('%,', m.id, ',', m.id, ',%'))
                    OR
                    (m.section = s.id AND CONCAT(',', s.sequence, ',') LIKE CONCAT('%,', m.id, ',%,', m.id, ',%'))
                )
            

            8 rows, 43.7 seconds

            course_modules: 677,379
            course_sections: 488,894
            mdl_course_modules m JOIN mdl_course_sections s (ON m.course = s.course): 12,102,962 - This number should be a better reflection of the execution time for query #2.

            Show
            Kris Stokking added a comment - MySQL 5.5: SELECT m.id, m.course, m. section , s. sequence FROM mdl_course_modules m LEFT OUTER JOIN mdl_course_sections s ON m.course = s.course and m. section = s.id WHERE s. sequence IS NULL OR (CONCAT( ',' , s. sequence , ',' ) NOT LIKE CONCAT( '%,' , m.id, ',%' )) ORDER BY m.course 407 rows, 2.22 seconds SELECT m.id, m.course, m. section , s.id AS sectionid, s. sequence FROM mdl_course_modules m JOIN mdl_course_sections s ON m.course = s.course AND ( (m. section <> s.id AND CONCAT( ',' , s. sequence , ',' ) LIKE CONCAT( '%,' , m.id, ',%' )) OR (m. section = s.id AND CONCAT( ',' , s. sequence , ',' ) LIKE CONCAT( '%,' , m.id, ',' , m.id, ',%' )) OR (m. section = s.id AND CONCAT( ',' , s. sequence , ',' ) LIKE CONCAT( '%,' , m.id, ',%,' , m.id, ',%' )) ) 8 rows, 43.7 seconds course_modules: 677,379 course_sections: 488,894 mdl_course_modules m JOIN mdl_course_sections s (ON m.course = s.course): 12,102,962 - This number should be a better reflection of the execution time for query #2.
            Hide
            Marina Glancy added a comment -

            Tim, Kris, thanks a lot for testing. IMHO those results are not bad considering that we go through two big tables and analyse text fields. What do you think yourself?
            Tim, regarding browser timeout - I would expect admins of big databases to run upgrade in CLI mode but even in the browser mode the timeout is removed in the script during upgrade process.

            At the same time I can see that both your databases have quite a lot of errors in module/section interlinking - both those queries should return 0 records on healthy database. This is another reason why this upgrade step is so important.

            There is a script in 2.6 if you want yourself to do a complete check with reporting of what and where is wrong:
            https://github.com/moodle/moodle/blob/master/admin/cli/fix_course_sequence.php
            it requires the function course_integrity_check() that was added in 2.6 but you can easily copy it into the CLI script to use with stable versions of Moodle.
            https://github.com/moodle/moodle/blob/master/course/lib.php#L856

            Structure of tables course_modules and course_sections has not changed for ages so there is nothing version-specific there. If you find that this script can be useful for previous versions, please comment on MDL-37028

            The script will not make any modifications to DB unless you specify option --fix

            Show
            Marina Glancy added a comment - Tim, Kris, thanks a lot for testing. IMHO those results are not bad considering that we go through two big tables and analyse text fields. What do you think yourself? Tim, regarding browser timeout - I would expect admins of big databases to run upgrade in CLI mode but even in the browser mode the timeout is removed in the script during upgrade process. At the same time I can see that both your databases have quite a lot of errors in module/section interlinking - both those queries should return 0 records on healthy database. This is another reason why this upgrade step is so important. There is a script in 2.6 if you want yourself to do a complete check with reporting of what and where is wrong: https://github.com/moodle/moodle/blob/master/admin/cli/fix_course_sequence.php it requires the function course_integrity_check() that was added in 2.6 but you can easily copy it into the CLI script to use with stable versions of Moodle. https://github.com/moodle/moodle/blob/master/course/lib.php#L856 Structure of tables course_modules and course_sections has not changed for ages so there is nothing version-specific there. If you find that this script can be useful for previous versions, please comment on MDL-37028 The script will not make any modifications to DB unless you specify option --fix
            Hide
            Tim Hunt added a comment -

            "I would expect admins of big databases to run upgrade in CLI"

            So would I, but surprisingly, for various reasons the OU is still upgrading through the web browser. Still, that need not stop you here. You have clearly achieved huge speed-ups, and it is probably now fast enough. Nice work.

            Show
            Tim Hunt added a comment - "I would expect admins of big databases to run upgrade in CLI" So would I, but surprisingly, for various reasons the OU is still upgrading through the web browser. Still, that need not stop you here. You have clearly achieved huge speed-ups, and it is probably now fast enough. Nice work.
            Hide
            Kris Stokking added a comment -

            Marina: The patch is looking good and I think it will work just fine for our purposes. I like how we theoretically have the option to apply $CFG->movingmoduleupgradescriptwasrun = 1 to unaffected sites ahead of time, minimizing the time to upgrade on larger sites. But we likely won't need to because it runs pretty darn fast regardless. I also like how we now have a diagnostic script for further reporting/fixing as needed - thanks very much for your work!

            Tim: Are there any beneficial reasons for running the upgrade through the web browser? I ask because I was under the same assumption as Marina.

            Future thoughts:

            • We should push to allow execution of other upgrade components outside of the main upgrade process when concurrent usage is permitted. What Tim did with the QE upgrade is a great example - the main upgrade process applied the schema changes and other critical parts, while the conversion of data could be deferred until after the main upgrade process. The upgrade process shouldn't be used as a crutch for forcing the execution of one-time scripts - there are good reasons for doing it for small sites that lack hosting experience or ability, but it comes at the cost of adding downtime for the upgrade to complete. I'm not sure this is something that can be enforced by the code, but it could be done during code reviews.
            • I wonder if it might be a good idea to push admins of large sites to run the upgrade process through CLI, assuming that any benefits to running it via web can be implemented in both places. It could be something as simple as warning admins that their site contains a high number of course modules (usually a good indicator), and it's recommended that they run the upgrade via CLI.
            Show
            Kris Stokking added a comment - Marina: The patch is looking good and I think it will work just fine for our purposes. I like how we theoretically have the option to apply $CFG->movingmoduleupgradescriptwasrun = 1 to unaffected sites ahead of time, minimizing the time to upgrade on larger sites. But we likely won't need to because it runs pretty darn fast regardless. I also like how we now have a diagnostic script for further reporting/fixing as needed - thanks very much for your work! Tim: Are there any beneficial reasons for running the upgrade through the web browser? I ask because I was under the same assumption as Marina. Future thoughts: We should push to allow execution of other upgrade components outside of the main upgrade process when concurrent usage is permitted. What Tim did with the QE upgrade is a great example - the main upgrade process applied the schema changes and other critical parts, while the conversion of data could be deferred until after the main upgrade process. The upgrade process shouldn't be used as a crutch for forcing the execution of one-time scripts - there are good reasons for doing it for small sites that lack hosting experience or ability, but it comes at the cost of adding downtime for the upgrade to complete. I'm not sure this is something that can be enforced by the code, but it could be done during code reviews. I wonder if it might be a good idea to push admins of large sites to run the upgrade process through CLI, assuming that any benefits to running it via web can be implemented in both places. It could be something as simple as warning admins that their site contains a high number of course modules (usually a good indicator), and it's recommended that they run the upgrade via CLI.
            Hide
            Tim Hunt added a comment -

            The only advantage of upgrade through the web browser is that you don't need CLI access to the server. This is a very small advantage, and all big sites (and as many other people as possible) should use CLI upgrade.

            In a big institution where the servers are managed by one team, and the Moodle code by another, however .... But, as I said before, that should not stop progress on this patch.

            Show
            Tim Hunt added a comment - The only advantage of upgrade through the web browser is that you don't need CLI access to the server. This is a very small advantage, and all big sites (and as many other people as possible) should use CLI upgrade. In a big institution where the servers are managed by one team, and the Moodle code by another, however .... But, as I said before, that should not stop progress on this patch.
            Hide
            Dan Poltawski added a comment - - edited

            Hi Marina,

            Gonna need some more time to process this, but thought i'd quickly comment about the LIKE's that shuld be $DB->sql_like()

            Also, perhaps we could move this into upgradelib function just for cleanliness.

            Show
            Dan Poltawski added a comment - - edited Hi Marina, Gonna need some more time to process this, but thought i'd quickly comment about the LIKE's that shuld be $DB->sql_like() Also, perhaps we could move this into upgradelib function just for cleanliness.
            Hide
            Marina Glancy added a comment -

            Thanks Dan, I will correct LIKE's and move to upgradelib shortly

            Show
            Marina Glancy added a comment - Thanks Dan, I will correct LIKE's and move to upgradelib shortly
            Hide
            Michael de Raadt added a comment -

            Carrying over to the next sprint

            Show
            Michael de Raadt added a comment - Carrying over to the next sprint
            Hide
            moodle.com added a comment -

            Removing this issue from the current sprint as Marina is not currently working as part of the BACKEND team. She may continue working on the issue independently.

            Show
            moodle.com added a comment - Removing this issue from the current sprint as Marina is not currently working as part of the BACKEND team. She may continue working on the issue independently.
            Hide
            Marina Glancy added a comment -

            grrr, we just figured out that MDL-38386 was accidentally not integrated in 2.5, and therefore did not appear in 2.6.

            Show
            Marina Glancy added a comment - grrr, we just figured out that MDL-38386 was accidentally not integrated in 2.5, and therefore did not appear in 2.6.
            Hide
            Marina Glancy added a comment - - edited

            Ok, trying to restore the history of this issue. I think this is the most unlucky issue ever.

            MDL-37430 made visibleold=1 for resources created in hidden section. By doing that it introduced a bug when moving modules between sections resulted in corrupted module/section relations. Integrated in 2.3, 2.4 and 2.5(dev)

            MDL-37939 fixed the bug with corrupting module/section relations and introduced an upgrade script to fix already corrupted relations. There is a mistake in upgrade script that causes corruption to be even worse. Integrated in 2.3, 2.4 and 2.5(dev)

            MDL-38173 introduced new upgrade script that fixes the corruption created in upgrade script from 37939. The branch for versions 2.4 and 2.5 had a weird $oldversion condition that made the upgrade process skip this step in the most cases. Integrated in 2.3, 2.4 and 2.5(dev)

            MDL-38386 changed the condition on $oldversion in 2.4 and 2.5 branches so it runs always. Was accidentally not integrated in 2.5.

            MDL-38228 (this issue) initially was created to make the upgrade script form 37939 faster but it looks like it needs to do more now.

            My comment above https://tracker.moodle.org/browse/MDL-38228?focusedCommentId=244855&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-244855 about breaking DB by the 37939 upgrade script was actually fixed already but since the fix to the fix was forgotten in integration of 2.5 it does not appear in 2.5 and 2.6 (this is where I reproduced it).

            Show
            Marina Glancy added a comment - - edited Ok, trying to restore the history of this issue. I think this is the most unlucky issue ever. MDL-37430 made visibleold=1 for resources created in hidden section. By doing that it introduced a bug when moving modules between sections resulted in corrupted module/section relations. Integrated in 2.3, 2.4 and 2.5(dev) MDL-37939 fixed the bug with corrupting module/section relations and introduced an upgrade script to fix already corrupted relations. There is a mistake in upgrade script that causes corruption to be even worse. Integrated in 2.3, 2.4 and 2.5(dev) MDL-38173 introduced new upgrade script that fixes the corruption created in upgrade script from 37939. The branch for versions 2.4 and 2.5 had a weird $oldversion condition that made the upgrade process skip this step in the most cases. Integrated in 2.3, 2.4 and 2.5(dev) MDL-38386 changed the condition on $oldversion in 2.4 and 2.5 branches so it runs always. Was accidentally not integrated in 2.5. MDL-38228 (this issue) initially was created to make the upgrade script form 37939 faster but it looks like it needs to do more now. My comment above https://tracker.moodle.org/browse/MDL-38228?focusedCommentId=244855&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-244855 about breaking DB by the 37939 upgrade script was actually fixed already but since the fix to the fix was forgotten in integration of 2.5 it does not appear in 2.5 and 2.6 (this is where I reproduced it).
            Hide
            Marina Glancy added a comment -

            So now we have two upgrade script in /lib/db/upgrade.php, one from MDL-37939 and another from MDL-38173. My suggestion for solution:

            Version 2.4: comment out 37939 upgrade script, replace 38173 with the new faster script. Sites that already have bigger version do not need to repeat upgrade script

            Version 2.5: comment out both 37939 and 38173, add new upgrade script in the end that is fast and fixes everything. Since it is very likely that the second upgrade script never run even if sites have bigger version.

            Version 2.6: comment out both 37939 and 38173, add new upgrade script in the end that is fast and fixes something. The rest will be fixed on-the-fly for each course separately during the next course cache generation (course integrity check introduced in MDL-37028)

            I sort of feel like I'm not allowed to make any more mistakes here....

            Show
            Marina Glancy added a comment - So now we have two upgrade script in /lib/db/upgrade.php, one from MDL-37939 and another from MDL-38173 . My suggestion for solution: Version 2.4: comment out 37939 upgrade script, replace 38173 with the new faster script. Sites that already have bigger version do not need to repeat upgrade script Version 2.5: comment out both 37939 and 38173, add new upgrade script in the end that is fast and fixes everything. Since it is very likely that the second upgrade script never run even if sites have bigger version. Version 2.6: comment out both 37939 and 38173, add new upgrade script in the end that is fast and fixes something. The rest will be fixed on-the-fly for each course separately during the next course cache generation (course integrity check introduced in MDL-37028 ) I sort of feel like I'm not allowed to make any more mistakes here....
            Hide
            Marina Glancy added a comment -

            I did lots of testing and submitting for integration now.

            One interesting thing I've noticed:
            If two sections have the same module in their sequence, the module will be displayed in the second section but not in the place where it is in the sequence but as the first module in the section. On 2.4 and 2.5 if you try moving this module in this section or to another section, it will seem to be moving but on page refresh it will re-appear in the beginning of the section.

            After the upgrade script this module will appear in the middle of the section where it was supposed to be and not where it was displayed before. It was just too difficult to fix in upgrade script - I'd have to run much more DB queries

            Show
            Marina Glancy added a comment - I did lots of testing and submitting for integration now. One interesting thing I've noticed: If two sections have the same module in their sequence, the module will be displayed in the second section but not in the place where it is in the sequence but as the first module in the section. On 2.4 and 2.5 if you try moving this module in this section or to another section, it will seem to be moving but on page refresh it will re-appear in the beginning of the section. After the upgrade script this module will appear in the middle of the section where it was supposed to be and not where it was displayed before. It was just too difficult to fix in upgrade script - I'd have to run much more DB queries
            Hide
            Marina Glancy added a comment -

            sorry, accidentally clicked button in the wrong tab

            Show
            Marina Glancy added a comment - sorry, accidentally clicked button in the wrong tab
            Hide
            Damyon Wiese added a comment -

            Hi Marina,

            I was dreading wading into this chain again, but your solution looks 100% correct and well documented. Also your commentary on the chain was useful in my reading back all the old issues to see where it went wrong and verify the original corruption vs what may have crept in afterwards. I tested this on all branches with 1000+ corrupt modules and it fixes them perfectly everytime.

            Integrated to 24, 25 and master (triple checked that it was pushed).

            Thanks again!

            Show
            Damyon Wiese added a comment - Hi Marina, I was dreading wading into this chain again, but your solution looks 100% correct and well documented. Also your commentary on the chain was useful in my reading back all the old issues to see where it went wrong and verify the original corruption vs what may have crept in afterwards. I tested this on all branches with 1000+ corrupt modules and it fixes them perfectly everytime. Integrated to 24, 25 and master (triple checked that it was pushed). Thanks again!
            Hide
            Rajesh Taneja added a comment -

            Thanks for fixing this Marina,

            I tried all upgrades, edited DB as per instructions and all went as expected.
            All duplicate records were removed and everything seems to work as expected.

            Passing...

            Show
            Rajesh Taneja added a comment - Thanks for fixing this Marina, I tried all upgrades, edited DB as per instructions and all went as expected. All duplicate records were removed and everything seems to work as expected. Passing...
            Hide
            Damyon Wiese added a comment -

            Here lies 52 bugs.
            All fixed or swept under a rug.
            If they come back one day,
            To our dismay,
            We all will feel quite un-smug.

            Thanks for the reporting/fixing/testing on this issue. It has been sent upstream.

            Show
            Damyon Wiese added a comment - Here lies 52 bugs. All fixed or swept under a rug. If they come back one day, To our dismay, We all will feel quite un-smug. Thanks for the reporting/fixing/testing on this issue. It has been sent upstream.

              People

              • Votes:
                6 Vote for this issue
                Watchers:
                16 Start watching this issue

                Dates

                • Created:
                  Updated:
                  Resolved: