Moodle
  1. Moodle
  2. MDL-29620

postgres id sequence for workshop activity is not preserved while upgrading from 1.9 to 2.0

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 2.0.5, 2.1.2, 2.2
    • Fix Version/s: 2.0.6, 2.1.3
    • Component/s: Workshop
    • Labels:
    • Environment:
    • Database:
      PostgreSQL
    • Testing Instructions:
      Hide

      1. On PostgreSQL, create new Moodle 1.9
      2. Create a course
      3. Create some Workshop instances (one should be enough, two or three should not hurt)
      4. Upgrade the site to the patched 2.0 or 2.1 or 2.2 Moodle
      5. TEST: make sure you can add another workshop instance

      Optional advance testing:

      • test other upgrade paths like 1.9 > non-patched 2.0 > (patched 2.0 or patched 2.1) etc
      • test on other databases: MySQL should not be affected by this issue but upgrade test would be nice, too. MSSQL and Oracle are probably affected by this issue, testing there would be nice to have on them, too.
      Show
      1. On PostgreSQL, create new Moodle 1.9 2. Create a course 3. Create some Workshop instances (one should be enough, two or three should not hurt) 4. Upgrade the site to the patched 2.0 or 2.1 or 2.2 Moodle 5. TEST: make sure you can add another workshop instance Optional advance testing: test other upgrade paths like 1.9 > non-patched 2.0 > (patched 2.0 or patched 2.1) etc test on other databases: MySQL should not be affected by this issue but upgrade test would be nice, too. MSSQL and Oracle are probably affected by this issue, testing there would be nice to have on them, too.
    • Workaround:
      Hide

      Setting the lastval attribute of the mdl_workshop_id_seq to the maximum value in the id column in mdl_workshop bypasses the problem:

      SELECT SETVAL( 'mdl_workshop_id_seq', (SELECT MAX( id ) + 1 AS last_value FROM mdl_workshop ), false );

      Show
      Setting the lastval attribute of the mdl_workshop_id_seq to the maximum value in the id column in mdl_workshop bypasses the problem: SELECT SETVAL( 'mdl_workshop_id_seq', (SELECT MAX( id ) + 1 AS last_value FROM mdl_workshop ), false );
    • Affected Branches:
      MOODLE_20_STABLE, MOODLE_21_STABLE, MOODLE_22_STABLE
    • Fixed Branches:
      MOODLE_20_STABLE, MOODLE_21_STABLE
    • Pull from Repository:
    • Pull Master Branch:
      MDL-29620-workshop-sequence
    • Rank:
      19133

      Description

      What is not working
      -----------------------
      Upgrading a moodle 1.9.13 installation to 2.0.4 does not keep the values for the mdl_workshop_id_seq sequence (moodle instance is running on a postgresql database). This results in errors due to violations of the UNIQUE constraints for the primary key mdl_workshop.id when workshop instances are created after the upgrade has finished

      Expected behaviour
      -----------------------
      The values (esp. last_value) of the sequence for mdl_workshop_id_seq should be kept in order to ensure smooth and continous operation across updates

      Notes / Remarks
      ------------------------

      • Throughout this bugreport, I assume $CFG->prefix = 'mdl_' for the sake of breviety. Please adapt if necessary to the value used at your instances
      • Interestingly, the error seems to occur only while updating from 1.9.x -> 2.0.x. Updating the 2.0.4 instance to 2.1.1 left the sequence in the correct state

        Activity

        Hide
        David Mudrak added a comment -

        Hmm interesting. I was actually developing and testing the upgrade procedure on PostgreSQL. I'll try to reproduce.

        Show
        David Mudrak added a comment - Hmm interesting. I was actually developing and testing the upgrade procedure on PostgreSQL. I'll try to reproduce.
        Hide
        David Mudrak added a comment -

        Got it. Working on patch.

        Show
        David Mudrak added a comment - Got it. Working on patch.
        Hide
        David Mudrak added a comment -

        Right. Petr Škoda gave a perfect hint and the fix is actually very easy. But I need to discuss the best way how to implement it into the workshop upgrade path.

        Synposis

        During the upgrade from 1.9 to 2.x, workshop calls $DB->import_record() to copy records from

        {workshop_old}

        to the new

        {workshop}

        while keeping the original ids. The bug is that I forgot to reset the sequence (the counter that looks after id autoincrement) after that transfer. So the records were transferred correctly but the serial id sequence was not updated. This leads to the reported behaviour that Postgres is trying to assign id=1 to the next workshop instance.

        Solution

        We must call reset_sequence('workshop'), for example as show in the following patch for MOODLE_20_STABLE:

        diff --git a/mod/workshop/db/upgrade.php b/mod/workshop/db/upgrade.php
        index df614c9..55359a8 100644
        --- a/mod/workshop/db/upgrade.php
        +++ b/mod/workshop/db/upgrade.php
        @@ -308,5 +308,10 @@ function xmldb_workshop_upgrade($oldversion) {
                 upgrade_mod_savepoint(true, 2011021100, 'workshop');
             }
         
        +    if ($oldversion < 2011030401) {
        +        $dbman->reset_sequence('workshop');
        +        upgrade_mod_savepoint(true, 2011030401, 'workshop');
        +    }
        +
             return true;
         }
        diff --git a/mod/workshop/version.php b/mod/workshop/version.php
        index 54bdc72..84aa880 100644
        --- a/mod/workshop/version.php
        +++ b/mod/workshop/version.php
        @@ -29,6 +29,6 @@
         
         defined('MOODLE_INTERNAL') || die();
         
        -$module->version  = 2011030400;
        +$module->version  = 2011030401;
         $module->requires = 2011020900;  // Requires this Moodle version
         //$module->cron     = 60;
        
        

        Discussion

        However I'd like to discuss the right approach with Eloy, Sam and Petr. I can see two ways of doing this:

        • put the reset_sequence() call into workshop_upgrade_module_instances() so that it would affect only sites that are going to be upgraded in the future. This leaves all eventual already-upgraded sites in a pretty bad state as no new workshop can't be added (and maybe they did not spot it yet)
        • or call the reset_sequence() as additional upgrade step at the end of upgrade.php. This would work for both future and already-passed upgrades but some cons. I did not find a way how to check (via Moodle cross-db layer) the current sequence state. So if the most recent workshop instance was deleted, the future call of this reset_sequence() would cause the deleted id re-used. I am not sure whether it is a big problem or not (afaik MySQL forgets such id when the server is restarted anyway, for example). As we must support all allowed combination of upgrade paths, we must expect this reset would happen several times (as the same code must land in 2.0, 2.1 and probably 2.2).

        Anyway, I can confirm that the suggested patch works and resetting the sequence is the way to go. The question is how to call it now.

        Show
        David Mudrak added a comment - Right. Petr Škoda gave a perfect hint and the fix is actually very easy. But I need to discuss the best way how to implement it into the workshop upgrade path. Synposis During the upgrade from 1.9 to 2.x, workshop calls $DB->import_record() to copy records from {workshop_old} to the new {workshop} while keeping the original ids. The bug is that I forgot to reset the sequence (the counter that looks after id autoincrement) after that transfer. So the records were transferred correctly but the serial id sequence was not updated. This leads to the reported behaviour that Postgres is trying to assign id=1 to the next workshop instance. Solution We must call reset_sequence('workshop'), for example as show in the following patch for MOODLE_20_STABLE: diff --git a/mod/workshop/db/upgrade.php b/mod/workshop/db/upgrade.php index df614c9..55359a8 100644 --- a/mod/workshop/db/upgrade.php +++ b/mod/workshop/db/upgrade.php @@ -308,5 +308,10 @@ function xmldb_workshop_upgrade($oldversion) { upgrade_mod_savepoint( true , 2011021100, 'workshop'); } + if ($oldversion < 2011030401) { + $dbman->reset_sequence('workshop'); + upgrade_mod_savepoint( true , 2011030401, 'workshop'); + } + return true ; } diff --git a/mod/workshop/version.php b/mod/workshop/version.php index 54bdc72..84aa880 100644 --- a/mod/workshop/version.php +++ b/mod/workshop/version.php @@ -29,6 +29,6 @@ defined('MOODLE_INTERNAL') || die(); -$module->version = 2011030400; +$module->version = 2011030401; $module->requires = 2011020900; // Requires this Moodle version //$module->cron = 60; Discussion However I'd like to discuss the right approach with Eloy, Sam and Petr. I can see two ways of doing this: put the reset_sequence() call into workshop_upgrade_module_instances() so that it would affect only sites that are going to be upgraded in the future. This leaves all eventual already-upgraded sites in a pretty bad state as no new workshop can't be added (and maybe they did not spot it yet) or call the reset_sequence() as additional upgrade step at the end of upgrade.php. This would work for both future and already-passed upgrades but some cons. I did not find a way how to check (via Moodle cross-db layer) the current sequence state. So if the most recent workshop instance was deleted, the future call of this reset_sequence() would cause the deleted id re-used. I am not sure whether it is a big problem or not (afaik MySQL forgets such id when the server is restarted anyway, for example). As we must support all allowed combination of upgrade paths, we must expect this reset would happen several times (as the same code must land in 2.0, 2.1 and probably 2.2). Anyway, I can confirm that the suggested patch works and resetting the sequence is the way to go. The question is how to call it now.
        Hide
        David Mudrak added a comment -

        Peer review requested (an experienced eye of some DB and upgrade guru is warmly welcome).

        Show
        David Mudrak added a comment - Peer review requested (an experienced eye of some DB and upgrade guru is warmly welcome).
        Hide
        Eloy Lafuente (stronk7) added a comment -

        I think this needs to be applied:

        • On all 2.x branches (and master of course).
        • Both in the original step where the import happened and extra new step dated now.
        • Exclusively in the workshop upgrade script. Anyone wanting a fix will need to upgrade.

        Also, I think it's safe to reset_sequence() multiple times in the same request, just if you can check it's covered by tests...

        Ciao

        Show
        Eloy Lafuente (stronk7) added a comment - I think this needs to be applied: On all 2.x branches (and master of course). Both in the original step where the import happened and extra new step dated now. Exclusively in the workshop upgrade script. Anyone wanting a fix will need to upgrade. Also, I think it's safe to reset_sequence() multiple times in the same request, just if you can check it's covered by tests... Ciao
        Hide
        Rajesh Taneja added a comment -

        I am putting it back for someone else to peer review this, as I am not so conversant with workshop module.
        Although, patch looks Good to me.

        Show
        Rajesh Taneja added a comment - I am putting it back for someone else to peer review this, as I am not so conversant with workshop module. Although, patch looks Good to me.
        Hide
        David Mudrak added a comment -

        Submitting a patch for integration. The patch fixes the original upgrade step and adds another upgrade step in Workshop.

        Credit goes to Martin Schwinzerl for the report, careful analysis of the issue and suggested solution.

        Show
        David Mudrak added a comment - Submitting a patch for integration. The patch fixes the original upgrade step and adds another upgrade step in Workshop. Credit goes to Martin Schwinzerl for the report, careful analysis of the issue and suggested solution.
        Hide
        Aparup Banerjee added a comment -

        thanks! this has been integrated!

        Show
        Aparup Banerjee added a comment - thanks! this has been integrated!
        Hide
        Sam Hemelryk added a comment -

        Thanks David, all appears to work perfectly.
        Cheers
        Sam

        Show
        Sam Hemelryk added a comment - Thanks David, all appears to work perfectly. Cheers Sam
        Hide
        Eloy Lafuente (stronk7) added a comment -

        Yes, you got this finally upstream, just in time for Moodle 2.2beta. Congrats and thanks!

        Ciao

        Show
        Eloy Lafuente (stronk7) added a comment - Yes, you got this finally upstream, just in time for Moodle 2.2beta. Congrats and thanks! Ciao

          People

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

            Dates

            • Created:
              Updated:
              Resolved: