Moodle
  1. Moodle
  2. MDL-30515

Check for non-unique values before adding unique indexes during Moodle 2.x upgrade

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Critical Critical
    • Resolution: Fixed
    • Affects Version/s: 2.0, 2.1
    • Fix Version/s: 2.2.5
    • Component/s: Database SQL/XMLDB
    • Environment:
      Probably shouldn't matter but I'm testing on Debian GNU/Linux with MySQL 5.1 and PostgreSQL 9.1
    • Database:
      Any
    • Testing Instructions:
      Hide

      Steps

      1. setup a new Moodle 1.9 site or use an existing one
      2. forcibly insert duplicate records in the mdl_grade_letters table (duplicate values being the same value for contextid, lowerboundary, and letter in two or more records)
      3. perform an upgrade to Moodle 2.2 with the patch applied

      Expected outcome

      The upgrade should complete without any problems.

      Problem outcome

      The upgrade fails when trying to add the unique index on the mdl_grade_letters table.

      Show
      Steps setup a new Moodle 1.9 site or use an existing one forcibly insert duplicate records in the mdl_grade_letters table (duplicate values being the same value for contextid , lowerboundary , and letter in two or more records) perform an upgrade to Moodle 2.2 with the patch applied Expected outcome The upgrade should complete without any problems. Problem outcome The upgrade fails when trying to add the unique index on the mdl_grade_letters table.
    • Affected Branches:
      MOODLE_20_STABLE, MOODLE_21_STABLE
    • Fixed Branches:
      MOODLE_22_STABLE
    • Pull from Repository:

      Description

      During an upgrade from Moodle 1.9 to Moodle 2.x a unique index is added to the mdl_grade_letters table. I have seen more than one case where this table contains duplicate records that prevent this index from being added and thus halt the upgrade, preventing it from continuing until the duplicates are dealt with.

        Gliffy Diagrams

          Issue Links

            Activity

            Hide
            Justin Filip added a comment - - edited

            The unique index was added as part of MDL-11888 and duplicates are a problem despite the comments in there.

            Show
            Justin Filip added a comment - - edited The unique index was added as part of MDL-11888 and duplicates are a problem despite the comments in there.
            Hide
            Justin Filip added a comment - - edited

            The code that is trying to insert duplicate records in the mdl_grade_letters table still exists in Moodle 2.x as reported by MDL-29598.

            Show
            Justin Filip added a comment - - edited The code that is trying to insert duplicate records in the mdl_grade_letters table still exists in Moodle 2.x as reported by MDL-29598 .
            Hide
            Justin Filip added a comment -

            I have code that will check for the existence of duplicates and remove them that can be added to the upgrade code before the unique index is created. Just putting said code into a Github branch and I will update this issue when that is ready.

            Show
            Justin Filip added a comment - I have code that will check for the existence of duplicates and remove them that can be added to the upgrade code before the unique index is created. Just putting said code into a Github branch and I will update this issue when that is ready.
            Hide
            Justin Filip added a comment -

            Attaching a CSV file of duplicate mdl_grade_letters values from a real production site.

            These values can be inserted into the mdl_grade_letters table for testing purposes.

            Show
            Justin Filip added a comment - Attaching a CSV file of duplicate mdl_grade_letters values from a real production site. These values can be inserted into the mdl_grade_letters table for testing purposes.
            Hide
            Justin Filip added a comment -

            Added Github pull branches and diff URLs for Moodle 2.0, 2.1, and 2.2 (master).

            Show
            Justin Filip added a comment - Added Github pull branches and diff URLs for Moodle 2.0, 2.1, and 2.2 (master).
            Hide
            Michael de Raadt added a comment -

            Thanks for following through on this.

            Show
            Michael de Raadt added a comment - Thanks for following through on this.
            Hide
            Martin Dougiamas added a comment -

            This can break upgrades in some cases, so hope it can be put in 2.2.1

            Show
            Martin Dougiamas added a comment - This can break upgrades in some cases, so hope it can be put in 2.2.1
            Hide
            David Monllaó added a comment - - edited

            Thanks for the patch Justin, I've changed the process to avoid the creation of a tmp table and just remove the duplicate entries leaving only the newest one.

            Submitting for peer review

            Show
            David Monllaó added a comment - - edited Thanks for the patch Justin, I've changed the process to avoid the creation of a tmp table and just remove the duplicate entries leaving only the newest one. Submitting for peer review
            Hide
            David Monllaó added a comment -

            Patch tested with mysql, postgres, oracle and mssql

            Show
            David Monllaó added a comment - Patch tested with mysql, postgres, oracle and mssql
            Hide
            Ankit Agarwal added a comment - - edited

            Hi guys,
            The patch looks good to me.
            The only thing I was worried about was reference to these entries being used in other tables,
            but after a bit of research it doesnt look like it is referenced from other tables.

            So my +1 for the patch.

            Thanks

            Show
            Ankit Agarwal added a comment - - edited Hi guys, The patch looks good to me. The only thing I was worried about was reference to these entries being used in other tables, but after a bit of research it doesnt look like it is referenced from other tables. So my +1 for the patch. Thanks
            Hide
            Aparup Banerjee added a comment -

            The main moodle.git repository has just been updated with latest weekly modifications. You may wish to rebase your PULL branches to simplify history and avoid any possible merge conflicts. This would also make integrator's life easier next week.

            TIA and ciao

            Show
            Aparup Banerjee added a comment - The main moodle.git repository has just been updated with latest weekly modifications. You may wish to rebase your PULL branches to simplify history and avoid any possible merge conflicts. This would also make integrator's life easier next week. TIA and ciao
            Hide
            David Monllaó added a comment -

            Thanks Aparup, pull branches rebased

            Show
            David Monllaó added a comment - Thanks Aparup, pull branches rebased
            Hide
            Sam Hemelryk added a comment -

            Thanks, this has been integrated now. Moodle 2.2 only.

            Show
            Sam Hemelryk added a comment - Thanks, this has been integrated now. Moodle 2.2 only.
            Hide
            Michael de Raadt added a comment -

            Test result: Success

            Without the change, the update failed. With the change, the update succeeded.

            The duplicate grade letter was consolidated into one entry.

            Show
            Michael de Raadt added a comment - Test result: Success Without the change, the update failed. With the change, the update succeeded. The duplicate grade letter was consolidated into one entry.
            Hide
            Dan Poltawski added a comment -

            asko, Дзякуй, ধন্যবাদ, Благодаря, Gràcies, 感谢, 謝謝, Hvala, Díky, Tak, Bedankt, Tänan, متشکریم, Salamat, Kiitokset, Merci, Grazas, Danke, Ευχαριστώ, આભાર, תודה, धन्यवाद, Köszönjük, Takk fyrir, Terima Kasih, Grazie, ありがとうございます, Рахмет, សូមអរគុណ, 감사합니다, gratiās, Pateicamies, Ačiū, Благодарам, Tēnā koa, Kia Ora Rawa Atu, आभारी आहोत, Талархал, Takk, Dziękuję, Obrigado, Mulţumesc, Engraziel, Спасибо, Fa'afetai, Хвала, Hvala, ස්තූතියි, Vďaka, Hvala, Mahadsanid, Thanks, Gracias, Tack, Salamat, நன்றி, నెనరులు, ขอบคุณค่ะ!

            Your work has made it into this weeks Moodle release! There are no gold medals available this week - but millions around the world will benefit. Thank you!

            Show
            Dan Poltawski added a comment - asko, Дзякуй, ধন্যবাদ, Благодаря, Gràcies, 感谢, 謝謝, Hvala, Díky, Tak, Bedankt, Tänan, متشکریم, Salamat, Kiitokset, Merci, Grazas, Danke, Ευχαριστώ, આભાર, תודה, धन्यवाद, Köszönjük, Takk fyrir, Terima Kasih, Grazie, ありがとうございます, Рахмет, សូមអរគុណ, 감사합니다, gratiās, Pateicamies, Ačiū, Благодарам, Tēnā koa, Kia Ora Rawa Atu, आभारी आहोत, Талархал, Takk, Dziękuję, Obrigado, Mulţumesc, Engraziel, Спасибо, Fa'afetai, Хвала, Hvala, ස්තූතියි, Vďaka, Hvala, Mahadsanid, Thanks, Gracias, Tack, Salamat, நன்றி, నెనరులు, ขอบคุณค่ะ! Your work has made it into this weeks Moodle release! There are no gold medals available this week - but millions around the world will benefit. Thank you!
            Hide
            Petr Skoda added a comment -

            Hi, this crated a regression for PostgreSQL - see MDL-34840.

            Show
            Petr Skoda added a comment - Hi, this crated a regression for PostgreSQL - see MDL-34840 .
            Hide
            David Monllaó added a comment -

            Hi, wondering about how can I skip that I suppose it was copy & pasting the query from the sql consoles... Sorry about that

            Show
            David Monllaó added a comment - Hi, wondering about how can I skip that I suppose it was copy & pasting the query from the sql consoles... Sorry about that

              People

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

                Dates

                • Created:
                  Updated:
                  Resolved: