Uploaded image for project: 'Moodle'
  1. Moodle
  2. MDL-30515

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

    Details

    • Type: Bug
    • Status: Closed
    • Priority: 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
            jfilip 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
            jfilip 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
            jfilip 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
            jfilip 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
            jfilip 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
            jfilip 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
            jfilip 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
            jfilip 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
            jfilip Justin Filip added a comment -

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

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

            Thanks for following through on this.

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

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

            Show
            dougiamas Martin Dougiamas added a comment - This can break upgrades in some cases, so hope it can be put in 2.2.1
            Hide
            dmonllao 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
            dmonllao 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
            dmonllao David Monllaó added a comment -

            Patch tested with mysql, postgres, oracle and mssql

            Show
            dmonllao David Monllaó added a comment - Patch tested with mysql, postgres, oracle and mssql
            Hide
            ankit_frenz 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_frenz 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
            nebgor 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
            nebgor 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
            dmonllao David Monllaó added a comment -

            Thanks Aparup, pull branches rebased

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

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

            Show
            samhemelryk Sam Hemelryk added a comment - Thanks, this has been integrated now. Moodle 2.2 only.
            Hide
            salvetore 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
            salvetore 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
            poltawski 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
            poltawski 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
            skodak Petr Skoda added a comment -

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

            Show
            skodak Petr Skoda added a comment - Hi, this crated a regression for PostgreSQL - see MDL-34840 .
            Hide
            dmonllao 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
            dmonllao 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:
                  Fix Release Date:
                  10/Sep/12