Moodle
  1. Moodle
  2. MDL-36493

improve perf of 2.3 mysql unsigned/text upgrade

    Details

    • Testing Instructions:
      Hide

      Please pay special attention to upgrade testing this. The change is only triggered from < 2.3 upgrades.

      1/ install moodle 22
      2/ upgrade to 2.3 and verify in database that all text columns are longtext and all ints are signed
      3/ install moodle 22
      4/ upgrade to master and verify in database that all text columns are longtext and all ints are signed

      Show
      Please pay special attention to upgrade testing this. The change is only triggered from < 2.3 upgrades. 1/ install moodle 22 2/ upgrade to 2.3 and verify in database that all text columns are longtext and all ints are signed 3/ install moodle 22 4/ upgrade to master and verify in database that all text columns are longtext and all ints are signed
    • Affected Branches:
      MOODLE_23_STABLE
    • Fixed Branches:
      MOODLE_23_STABLE, MOODLE_24_STABLE
    • Pull from Repository:
    • Pull 2.4 Branch:
      w50_MDL-36493_m24_mysql
    • Pull Master Branch:
      w50_MDL-36493_m25_mysql
    • Rank:
      45951

      Description

      We could probably use one query per table only...

        Issue Links

          Activity

          Hide
          Mark Nielsen added a comment -

          Hey Petr, I was checking out your changes and I'm a little confused on how the timeout is being handled. It seems like the timeout is being updated after each column in a single table is being checked and not when the actual modifications go through. Also, could we avoid the $DB->count_records($table, array()); when there are no changes? Might speed things up. (My assumption here is that the $DB->change_database_structure($sql); is the time hog)

          This looks like it will be a great improvement, thanks!

          Show
          Mark Nielsen added a comment - Hey Petr, I was checking out your changes and I'm a little confused on how the timeout is being handled. It seems like the timeout is being updated after each column in a single table is being checked and not when the actual modifications go through. Also, could we avoid the $DB->count_records($table, array()); when there are no changes? Might speed things up. (My assumption here is that the $DB->change_database_structure($sql); is the time hog) This looks like it will be a great improvement, thanks!
          Hide
          Petr Škoda added a comment -

          Hi, yes I agree that the code should be refactored a bit, I have added one more commit, thanks a lot.

          Show
          Petr Škoda added a comment - Hi, yes I agree that the code should be refactored a bit, I have added one more commit, thanks a lot.
          Hide
          Petr Škoda added a comment -

          I suppose this could be at least 3x faster on large installs.

          Show
          Petr Škoda added a comment - I suppose this could be at least 3x faster on large installs.
          Hide
          Martin Dougiamas added a comment - - edited

          This seems a bit late, but is it worth integrating into 2.5 now to let it be tested? Then backport to 2.3.x and 2.4.x later.

          How risky is this while thing really?

          Show
          Martin Dougiamas added a comment - - edited This seems a bit late, but is it worth integrating into 2.5 now to let it be tested? Then backport to 2.3.x and 2.4.x later. How risky is this while thing really?
          Hide
          Martin Dougiamas added a comment -

          Mark, Kris - is it possible for you to try this out on various sites and let us know if it's worth backporting for you?

          Show
          Martin Dougiamas added a comment - Mark, Kris - is it possible for you to try this out on various sites and let us know if it's worth backporting for you?
          Hide
          Kris Stokking added a comment -

          Hey Martin - In terms of the risk, I'd consider it quite low as the intent is to make the schema change more efficient by changing from one ALTER TABLE statement per column to one statement per table. The end result is the same, and it should make the upgrade of larger tables with multiple unsigned int/text fields much faster (for example, converting the log and grades history). Testing should be quite easy - we just need to validate that the schemas match after the upgrade both with and without the improvement. Metrics would be overkill in my opinion - it's guaranteed to perform at least as fast.

          This patch is definitely of interest to us, and it should be also be of interest to any site admin that hosts a Moodle site on a version pre-2.3. We'd be happy to help with testing effort if that will expedite it getting backported to 2.3.

          Show
          Kris Stokking added a comment - Hey Martin - In terms of the risk, I'd consider it quite low as the intent is to make the schema change more efficient by changing from one ALTER TABLE statement per column to one statement per table. The end result is the same, and it should make the upgrade of larger tables with multiple unsigned int/text fields much faster (for example, converting the log and grades history). Testing should be quite easy - we just need to validate that the schemas match after the upgrade both with and without the improvement. Metrics would be overkill in my opinion - it's guaranteed to perform at least as fast. This patch is definitely of interest to us, and it should be also be of interest to any site admin that hosts a Moodle site on a version pre-2.3. We'd be happy to help with testing effort if that will expedite it getting backported to 2.3.
          Hide
          Dan Poltawski added a comment -

          I've integrated this now, thanks Petr.

          Show
          Dan Poltawski added a comment - I've integrated this now, thanks Petr.
          Hide
          David Monllaó added a comment -

          It passes, tested upgrading from 22 to 23 and 24 and checking the DB with:

          SELECT * from information_schema.COLUMNS where TABLE_SCHEMA = 'databasename' AND DATA_TYPE LIKE '%int%' AND COLUMN_TYPE LIKE '%unsigned%'

          SELECT * from information_schema.COLUMNS where TABLE_SCHEMA = 'databasename' AND DATA_TYPE LIKE '%text%' AND DATA_TYPE != 'longtext'

          adodb_logsql is the only table with no longtext and no unsigned ints

          Show
          David Monllaó added a comment - It passes, tested upgrading from 22 to 23 and 24 and checking the DB with: SELECT * from information_schema.COLUMNS where TABLE_SCHEMA = 'databasename' AND DATA_TYPE LIKE '%int%' AND COLUMN_TYPE LIKE '%unsigned%' SELECT * from information_schema.COLUMNS where TABLE_SCHEMA = 'databasename' AND DATA_TYPE LIKE '%text%' AND DATA_TYPE != 'longtext' adodb_logsql is the only table with no longtext and no unsigned ints
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Changes are now upstream, thanks for your collaboration!

          If you are going to have any celebration next days, enjoy with your gang, if not, too!

          Ciao

          Show
          Eloy Lafuente (stronk7) added a comment - Changes are now upstream, thanks for your collaboration! If you are going to have any celebration next days, enjoy with your gang, if not, too! Ciao

            People

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

              Dates

              • Created:
                Updated:
                Resolved: