Moodle
  1. Moodle
  2. MDL-34086 META: Increase robustness of upgrade from 1.9 to 2.2
  3. MDL-25948

Sites using 1.9.x, but originally installed with < 1.7, fail upgrading to 2.0 due to null fields in mdl_user table

    Details

    • Type: Sub-task Sub-task
    • Status: Closed
    • Priority: Blocker Blocker
    • Resolution: Fixed
    • Affects Version/s: 2.0, 2.0.1, 2.1.8, 2.2.5
    • Fix Version/s: 2.1.9, 2.2.6
    • Component/s: Installation
    • Environment:
      PostgreSQL 8.4.6 (I've no way to test it on MySQL)
    • Database:
      PostgreSQL
    • Testing Instructions:
      Hide

      1. Install Moodle 1.6 using Postgres.

      I had some issues here with getting Moodle's installer to generate the config.php as it could not connect to the database. I ended up having to create the file myself and specify the port. Eg.

      $CFG->dbtype = 'postgres7'; // mysql or postgres7 (for now)
      $CFG->dbhost = 'localhost'; // eg localhost or db.isp.com
      $CFG->dbname = 'name'; // database name, eg moodle
      $CFG->dbuser = 'user'; // your database username
      $CFG->dbpass = 'pass'; // your database password
      $CFG->dbport = '5432';
      $CFG->prefix = 'mdl_'; // Prefix to use for all table names

      $CFG->dbpersist = false; // Should database connections be reused?

      Remove all references of insertion to logs (eg. INSERT INTO prefix_log_display (module, action, mtable, field) VALUES ('forum', 'add', 'forum', 'name')) from lib/db/postgres7.sql and mod/forum/db/postgres7.sql before the upgrade as they will cause database errors. This is an issue in 1.6/1.7 so not reporting.

      2. Upgrade to 1.9, then to 2.1
      3. Ensure there are no null column errors. eg.

      Debug info: ERROR: column "lastip" contains null values.
      4. Compare the upgraded user table field attributes (default values, length etc) to those of a fresh install to ensure they are the same (note, they will be in the wrong order as the upgrade to 1.9 changes this (another issue?))
      5. Repeat, except for step 2 upgrade to 2.2

      Show
      1. Install Moodle 1.6 using Postgres. I had some issues here with getting Moodle's installer to generate the config.php as it could not connect to the database. I ended up having to create the file myself and specify the port. Eg. $CFG->dbtype = 'postgres7'; // mysql or postgres7 (for now) $CFG->dbhost = 'localhost'; // eg localhost or db.isp.com $CFG->dbname = 'name'; // database name, eg moodle $CFG->dbuser = 'user'; // your database username $CFG->dbpass = 'pass'; // your database password $CFG->dbport = '5432'; $CFG->prefix = 'mdl_'; // Prefix to use for all table names $CFG->dbpersist = false; // Should database connections be reused? Remove all references of insertion to logs (eg. INSERT INTO prefix_log_display (module, action, mtable, field) VALUES ('forum', 'add', 'forum', 'name')) from lib/db/postgres7.sql and mod/forum/db/postgres7.sql before the upgrade as they will cause database errors. This is an issue in 1.6/1.7 so not reporting. 2. Upgrade to 1.9, then to 2.1 3. Ensure there are no null column errors. eg. Debug info: ERROR: column "lastip" contains null values. 4. Compare the upgraded user table field attributes (default values, length etc) to those of a fresh install to ensure they are the same (note, they will be in the wrong order as the upgrade to 1.9 changes this (another issue?)) 5. Repeat, except for step 2 upgrade to 2.2
    • Affected Branches:
      MOODLE_20_STABLE, MOODLE_21_STABLE, MOODLE_22_STABLE
    • Fixed Branches:
      MOODLE_21_STABLE, MOODLE_22_STABLE
    • Pull from Repository:
    • Rank:
      15327

      Description

      In sites currently using 1.9.x, that have been originally installed with < 1.7 and then upgraded to 1.9 (even passing through 1.8), the mdl_user table have many NULLable columns without DEFAULTs.

      This will cause upgrading to 2.0.x failing due to db errors.

      -----------------------------------------------------------------
      I attached a simple SQL script that add DEFAULTs and sets NOT NULLs, as a freshly installed 1.9.x (please edit to change table prefix)

        Issue Links

          Activity

          Hide
          Lorenzo Nicora added a comment -

          I think a new db check should be implemented for 1.9, to fix problems related to old sites upgrading.
          This check should be similar to check_defaults, implemented in 2.0 only ( MDL-14854 ).

          But what need to be checked and fixed are NOT NULLs and DEFAULTs together.
          I implemented this check on 1.9.10 (attached zip), called check_notnull_defaults.

          The logic is the following:

          • If a column is defined by XMLDB as NOT NULL and the physical columns is NULL OR if the physical column has no DEFAULT and XMLDB defines any, fix the column:
            • SET DEFAULT according to XMLDB, if needed
            • UPDATE records containing NULL in this column with DEFAULT value
            • SET NOT NULL, if needed

          Please note that it never changes a NOT NULL column to NULL.
          It also doesn't check if defined and physical DEFAULTs matches.

          Currently only MySQL and PostgreSQL are supported, as I've no way to test on Oracle or MsSQL by now.

          Messages are not completely defined, to avoid modifying core xmldb message file.
          I had to change main_view action to be able to call it.

          Show
          Lorenzo Nicora added a comment - I think a new db check should be implemented for 1.9, to fix problems related to old sites upgrading. This check should be similar to check_defaults, implemented in 2.0 only ( MDL-14854 ). But what need to be checked and fixed are NOT NULLs and DEFAULTs together. I implemented this check on 1.9.10 (attached zip), called check_notnull_defaults. The logic is the following: If a column is defined by XMLDB as NOT NULL and the physical columns is NULL OR if the physical column has no DEFAULT and XMLDB defines any, fix the column: SET DEFAULT according to XMLDB, if needed UPDATE records containing NULL in this column with DEFAULT value SET NOT NULL, if needed Please note that it never changes a NOT NULL column to NULL. It also doesn't check if defined and physical DEFAULTs matches. Currently only MySQL and PostgreSQL are supported, as I've no way to test on Oracle or MsSQL by now. Messages are not completely defined, to avoid modifying core xmldb message file. I had to change main_view action to be able to call it.
          Hide
          Michael de Raadt added a comment -

          This has been duplicated now.

          I'm promoting this issue.

          Another patch has been contributed with the duplicate issue, which should be considered when resolving this issue.

          Show
          Michael de Raadt added a comment - This has been duplicated now. I'm promoting this issue. Another patch has been contributed with the duplicate issue, which should be considered when resolving this issue.
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Uhm, I've a bit amazed that both this report and all the related ones are about PostgreSQL.

          The key here is that when column is move from:

          nullable (with or without default)

          to

          not null (with default)

          the default, usually '' (empty string) should be applied to all the records being null (one update statement), and only then the alter table should be performed. I think that's the way the sql generators are doing for all DBs... perhaps there is one bug in the postgreSQL one.

          I'm going to try reproducing it here with some unit tests and review how the move above works under all DBs.

          Ciao

          Show
          Eloy Lafuente (stronk7) added a comment - Uhm, I've a bit amazed that both this report and all the related ones are about PostgreSQL. The key here is that when column is move from: nullable (with or without default) to not null (with default) the default, usually '' (empty string) should be applied to all the records being null (one update statement), and only then the alter table should be performed. I think that's the way the sql generators are doing for all DBs... perhaps there is one bug in the postgreSQL one. I'm going to try reproducing it here with some unit tests and review how the move above works under all DBs. Ciao
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Oki, we are going to add one new report to the editor, in charge of checking all the columns with incorrect specs and providing the SQL statements to fix them before upgrade.

          The report will be available under 1.9 (where it is really necessary) and also under al the 2.x versions, as an extra check available.

          Ciao

          Show
          Eloy Lafuente (stronk7) added a comment - Oki, we are going to add one new report to the editor, in charge of checking all the columns with incorrect specs and providing the SQL statements to fix them before upgrade. The report will be available under 1.9 (where it is really necessary) and also under al the 2.x versions, as an extra check available. Ciao
          Hide
          Aparup Banerjee added a comment -

          Hi Eloy, was there any code done here towards a report?

          Show
          Aparup Banerjee added a comment - Hi Eloy, was there any code done here towards a report?
          Hide
          Mark Nelson added a comment -

          I installed a 1.6 version of Moodle on PostgreSQL, upgrade to 1.9 and then to 2.0 and can confirm this is still an issue.

          Show
          Mark Nelson added a comment - I installed a 1.6 version of Moodle on PostgreSQL, upgrade to 1.9 and then to 2.0 and can confirm this is still an issue.
          Hide
          Mark Nelson added a comment - - edited

          I went through the upgrade twice and it only failed on two fields. I have created a patch and gone through the upgrade successfully.

          Edit:
          For some reason I thought you had to go from 1.9 -> 2.0 before you could upgrade further onwards. Just looked at release notes and noticed this is not the case. Sorry about all the emails!

          Show
          Mark Nelson added a comment - - edited I went through the upgrade twice and it only failed on two fields. I have created a patch and gone through the upgrade successfully. Edit: For some reason I thought you had to go from 1.9 -> 2.0 before you could upgrade further onwards. Just looked at release notes and noticed this is not the case. Sorry about all the emails!
          Hide
          Rajesh Taneja added a comment -

          Hello Mark,

          Patch looks good, although you might want to consider few things before pushing it further:

          1. You need to separate both update calls close to the problem area
          2. Comments should be precise and not so explanatory.
          3. Checked 1.6 code and seems we have alot of default values set to NULL, so you might want to consider them
          4. I think you need to change default values for old fields from NULL to '' as well
          5. Will be nice to update all fields, by bumping up the version

          FYI:
          Moodle19/lib/db/upgrade.php - Line 2991
          You might want to just update this for postgres as NULL was default in postgres only.
          Check Moodle17/lib/db/mysql.sql and postgres7.sql, and you can see postgres7.sql have

             idnumber varchar(64) default NULL,
             icq varchar(15) default NULL,
             skype varchar(50) default NULL,
             yahoo varchar(50) default NULL,
             aim varchar(50) default NULL,
             msn varchar(50) default NULL,
             phone1 varchar(20) default NULL,
             phone2 varchar(20) default NULL,
             institution varchar(40) default NULL,
             department varchar(30) default NULL,
             address varchar(70) default NULL,
             city varchar(20) default NULL,
             country char(2) default NULL,
             lastip varchar(15) default NULL,
             secret varchar(15) default NULL,
             picture integer default NULL,
             url varchar(255) default NULL,
          
          Show
          Rajesh Taneja added a comment - Hello Mark, Patch looks good, although you might want to consider few things before pushing it further: You need to separate both update calls close to the problem area Comments should be precise and not so explanatory. Checked 1.6 code and seems we have alot of default values set to NULL, so you might want to consider them I think you need to change default values for old fields from NULL to '' as well Will be nice to update all fields, by bumping up the version FYI: Moodle19/lib/db/upgrade.php - Line 2991 You might want to just update this for postgres as NULL was default in postgres only. Check Moodle17/lib/db/mysql.sql and postgres7.sql, and you can see postgres7.sql have idnumber varchar(64) default NULL, icq varchar(15) default NULL, skype varchar(50) default NULL, yahoo varchar(50) default NULL, aim varchar(50) default NULL, msn varchar(50) default NULL, phone1 varchar(20) default NULL, phone2 varchar(20) default NULL, institution varchar(40) default NULL, department varchar(30) default NULL, address varchar(70) default NULL, city varchar(20) default NULL, country char(2) default NULL, lastip varchar(15) default NULL, secret varchar(15) default NULL, picture integer default NULL, url varchar(255) default NULL,
          Hide
          Mark Nelson added a comment -

          Great peer review Raj. I agree with all your points. Please see updated diff.

          Show
          Mark Nelson added a comment - Great peer review Raj. I agree with all your points. Please see updated diff.
          Hide
          Mark Nelson added a comment - - edited

          Just a quick note, the upgrade from 1.6 -> 1.9 changes the order of the 'idnumber' field in the user table. Not really an issue.

          Show
          Mark Nelson added a comment - - edited Just a quick note, the upgrade from 1.6 -> 1.9 changes the order of the 'idnumber' field in the user table. Not really an issue.
          Hide
          Rajesh Taneja added a comment -

          [Y] Syntax
          [-] Output
          [Y] Whitespace
          [-] Language
          [Y] Databases
          [-] Testing
          [-] Security
          [-] Documentation
          [Y] Git
          [Y] Sanity check

          Thanks Mark, seems fine now. Feel free to push it for integration.

          Show
          Rajesh Taneja added a comment - [Y] Syntax [-] Output [Y] Whitespace [-] Language [Y] Databases [-] Testing [-] Security [-] Documentation [Y] Git [Y] Sanity check Thanks Mark, seems fine now. Feel free to push it for integration.
          Hide
          Dan Poltawski added a comment -

          The integration of this issue has been delayed to next week because the integration period is over (Monday, Tuesday) and testing must happen on Wednesday.
          This change to a more rigid timeframe on each integration/testing cycle aims to produce a better and clear separation and organization of tasks for everybody.
          This is a bulk-automated message, so if you want to blame somebody/thing/where, don't do it here (use git instead) :-D :-P
          Apologizes for the inconvenient, this will be integrated next week. Thanks for your collaboration & ciao

          Show
          Dan Poltawski added a comment - The integration of this issue has been delayed to next week because the integration period is over (Monday, Tuesday) and testing must happen on Wednesday. This change to a more rigid timeframe on each integration/testing cycle aims to produce a better and clear separation and organization of tasks for everybody. This is a bulk-automated message, so if you want to blame somebody/thing/where, don't do it here (use git instead) :-D :-P Apologizes for the inconvenient, this will be integrated next week. Thanks for your collaboration & ciao
          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
          Dan Poltawski added a comment -

          Hi Mark,

          If you remember we discussed in the office the idea of checking if the values in the DB were incorrect before dropping and recreating all the values.

          On some big sites the time to drop and recreate these indexes on the user table could be quite huge, so I think it is best to take this extra step of only doing these steps where the defaults are incorrect.

          Show
          Dan Poltawski added a comment - Hi Mark, If you remember we discussed in the office the idea of checking if the values in the DB were incorrect before dropping and recreating all the values. On some big sites the time to drop and recreate these indexes on the user table could be quite huge, so I think it is best to take this extra step of only doing these steps where the defaults are incorrect.
          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
          Mark Nelson added a comment - - edited

          Hi Dan, as discussed the XMLDB editor 'check defaults' tool reads the install.xml file and loads the structure from there, it then retrieves the structure in the database and makes a comparison. There is no simple API call to perform this check so would require some new logic which we decided not to do. I rebased my branches and am now resubmitting for integration.

          Show
          Mark Nelson added a comment - - edited Hi Dan, as discussed the XMLDB editor 'check defaults' tool reads the install.xml file and loads the structure from there, it then retrieves the structure in the database and makes a comparison. There is no simple API call to perform this check so would require some new logic which we decided not to do. I rebased my branches and am now resubmitting for integration.
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Hi Mark,

          I think I like the patch, just trying to imagine if could save some operations (update/index drop & recreation and alter table) if the column is already defined as not null in the database.

          If for example will, for sure, avoid the double processing of lastip and city (which update is now performed twice unconditionally). And potentially others too.

          So I'd suggest to do something like this:

          $columns = $DB->get_columns('user');
          

          And then look for:

          $columns[COLUMNNAME]->not_null and or $columns[COLUMNNAME]->has_default
          

          and then for each:

          • update
          • index drop and recreation
          • alter table

          decide based on that.

          In the other side... your code seems really safer because it will define the columns with the complete correct specs always (not null, default, length...) just in case there are other discrepancies in the schema. Are there?

          I think that's the question that will tell us if we can save some heavy changes (if not-null/default is the only discrepancy) or must run all them always (with your current code).

          So... are there other discrepancies apart from the not-null/default or no?

          Ciao

          Show
          Eloy Lafuente (stronk7) added a comment - Hi Mark, I think I like the patch, just trying to imagine if could save some operations (update/index drop & recreation and alter table) if the column is already defined as not null in the database. If for example will, for sure, avoid the double processing of lastip and city (which update is now performed twice unconditionally). And potentially others too. So I'd suggest to do something like this: $columns = $DB->get_columns('user'); And then look for: $columns[COLUMNNAME]->not_null and or $columns[COLUMNNAME]->has_default and then for each: update index drop and recreation alter table decide based on that. In the other side... your code seems really safer because it will define the columns with the complete correct specs always (not null, default, length...) just in case there are other discrepancies in the schema. Are there? I think that's the question that will tell us if we can save some heavy changes (if not-null/default is the only discrepancy) or must run all them always (with your current code). So... are there other discrepancies apart from the not-null/default or no? Ciao
          Hide
          Mark Nelson added a comment -

          Hi Eloy, looks like the only difference is the null values. I am going to create another commit (so I don't destroy the base logic) that adds some checks on the fields as you suggested.

          Show
          Mark Nelson added a comment - Hi Eloy, looks like the only difference is the null values. I am going to create another commit (so I don't destroy the base logic) that adds some checks on the fields as you suggested.
          Hide
          Mark Nelson added a comment -

          Hi Eloy, I made those changes. Can you please review? Thanks!

          Show
          Mark Nelson added a comment - Hi Eloy, I made those changes. Can you please review? Thanks!
          Hide
          Eloy Lafuente (stronk7) added a comment - - edited

          I've tested the 2 upgrades (to 21 and to 22) and everything looks perfect once finished. Just note that there are 2 missing indexes, but that's something that the "XMLDB Check indexes" report should be able to report, providing the needed SQL to fix them.

          So I'm going to integrate this right now (21 and 22 stables).

          Thanks!

          PS: No matter of this, I more and more think about the need to build the "Complete Comparison Check" within the XMLDB editor, similar to current BIGINT/Default/Indexes ones. It should be able to verify the whole DB schema reporting any discrepancy and providing the SQL to fix it. For sure, the ultimate solution for ANY problem. Feel free to create a new issue about that.

          Show
          Eloy Lafuente (stronk7) added a comment - - edited I've tested the 2 upgrades (to 21 and to 22) and everything looks perfect once finished. Just note that there are 2 missing indexes, but that's something that the "XMLDB Check indexes" report should be able to report, providing the needed SQL to fix them. So I'm going to integrate this right now (21 and 22 stables). Thanks! PS: No matter of this, I more and more think about the need to build the "Complete Comparison Check" within the XMLDB editor, similar to current BIGINT/Default/Indexes ones. It should be able to verify the whole DB schema reporting any discrepancy and providing the SQL to fix it. For sure, the ultimate solution for ANY problem. Feel free to create a new issue about that.
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Integrated (21 and 22), thanks!

          Show
          Eloy Lafuente (stronk7) added a comment - Integrated (21 and 22), thanks!
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Passed testing, as commented above.

          Show
          Eloy Lafuente (stronk7) added a comment - Passed testing, as commented above.
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Just in time for 2.1.9 and 2.2.6 releases, yay!

          Thanks, closing, ciao

          Show
          Eloy Lafuente (stronk7) added a comment - Just in time for 2.1.9 and 2.2.6 releases, yay! Thanks, closing, ciao
          Hide
          Dennis Meyer added a comment -

          I had to set default to '' (after update from 1.9 to 2.2/2.4):

          ALTER TABLE mdl_user ALTER COLUMN icq SET DEFAULT '';
          ALTER TABLE mdl_user ALTER COLUMN url SET DEFAULT '';
          ALTER TABLE mdl_user ALTER COLUMN secret SET DEFAULT '';
          ALTER TABLE mdl_user ALTER COLUMN address SET DEFAULT '';
          ALTER TABLE mdl_user ALTER COLUMN department SET DEFAULT '';
          ALTER TABLE mdl_user ALTER COLUMN institution SET DEFAULT '';
          ALTER TABLE mdl_user ALTER COLUMN phone2 SET DEFAULT '';
          ALTER TABLE mdl_user ALTER COLUMN phone1 SET DEFAULT '';
          ALTER TABLE mdl_user ALTER COLUMN msn SET DEFAULT '';
          ALTER TABLE mdl_user ALTER COLUMN aim SET DEFAULT '';
          ALTER TABLE mdl_user ALTER COLUMN yahoo SET DEFAULT '';
          ALTER TABLE mdl_user ALTER COLUMN skype SET DEFAULT '';

          Without those defaults no new user will be created, I guess :-/

          Show
          Dennis Meyer added a comment - I had to set default to '' (after update from 1.9 to 2.2/2.4): ALTER TABLE mdl_user ALTER COLUMN icq SET DEFAULT ''; ALTER TABLE mdl_user ALTER COLUMN url SET DEFAULT ''; ALTER TABLE mdl_user ALTER COLUMN secret SET DEFAULT ''; ALTER TABLE mdl_user ALTER COLUMN address SET DEFAULT ''; ALTER TABLE mdl_user ALTER COLUMN department SET DEFAULT ''; ALTER TABLE mdl_user ALTER COLUMN institution SET DEFAULT ''; ALTER TABLE mdl_user ALTER COLUMN phone2 SET DEFAULT ''; ALTER TABLE mdl_user ALTER COLUMN phone1 SET DEFAULT ''; ALTER TABLE mdl_user ALTER COLUMN msn SET DEFAULT ''; ALTER TABLE mdl_user ALTER COLUMN aim SET DEFAULT ''; ALTER TABLE mdl_user ALTER COLUMN yahoo SET DEFAULT ''; ALTER TABLE mdl_user ALTER COLUMN skype SET DEFAULT ''; Without those defaults no new user will be created, I guess :-/

            People

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

              Dates

              • Created:
                Updated:
                Resolved: