Moodle
  1. Moodle
  2. MDL-34933

Performance Improvement for message_working table

    Details

    • Testing Instructions:
      Hide

      Testing is really to verify that the index gets created as necessary.

      Moodle upgrade:

      1. On a site that is a version prior to the version in the branch being tested, verify that no index exists on the "message_working" table for the field "unreadmessageid".
      2. Install a codebase containing this update on that site.
      3. Run the Moodle update.
      4. Verify that the index is created on the table "message_working" for the field "unreadmessageid".

      Moodle install:

      1. Run a new Moodle install using a codebase containing this code.
      2. Verify that an index was created in the "message_working" table using the field "unreadmessageid".
      Show
      Testing is really to verify that the index gets created as necessary. Moodle upgrade: On a site that is a version prior to the version in the branch being tested, verify that no index exists on the "message_working" table for the field "unreadmessageid". Install a codebase containing this update on that site. Run the Moodle update. Verify that the index is created on the table "message_working" for the field "unreadmessageid". Moodle install: Run a new Moodle install using a codebase containing this code. Verify that an index was created in the "message_working" table using the field "unreadmessageid".
    • Workaround:
      Hide

      n.a.

      Show
      n.a.
    • Affected Branches:
      MOODLE_22_STABLE, MOODLE_23_STABLE, MOODLE_24_STABLE, MOODLE_25_STABLE
    • Fixed Branches:
      MOODLE_23_STABLE, MOODLE_24_STABLE
    • Pull 2.4 Branch:
      MDL-34933_24_STABLE
    • Pull Master Branch:
      MDL-34933_master
    • Rank:
      43489

      Description

      Suggesting adding an index to the message_working.unreadmessageid field. When the tables start to grow, this query can take some time without the index. For example, for one site, it takes 1,891 ms to run the query.

      Here is the query:

      SELECT m.id, m.smallmessage, m.fullmessageformat, m.notification, u.firstname, u.lastname
      FROM mdl_message m
      JOIN mdl_message_working mw ON m.id=mw.unreadmessageid
      JOIN mdl_message_processors p ON mw.processorid=p.id
      JOIN mdl_user u ON m.useridfrom=u.id
      WHERE m.useridto = '?' AND p.name='?'
      

      Here is the trace:

      ...uery called at lib/dml/mysqli_native_moodle_database.php (808)
      ...le_database::get_records_sql called at lib/moodlelib.php (9911)
              in message_popup_window called at lib/pagelib.php (1230)
      in moodle_page::starting_output called at lib/pagelib.php (720)
      ...moodle_page::set_state called at lib/outputrenderers.php (632)
          in core_renderer::header called at mod/forum/view.php (113)
      

        Issue Links

          Activity

          Hide
          Mark Nielsen added a comment -

          Removed the suggestion for the index message_working.processorid, turns out that isn't necessary.

          Show
          Mark Nielsen added a comment - Removed the suggestion for the index message_working.processorid, turns out that isn't necessary.
          Hide
          Andrew Davis added a comment -

          This sounds like a good idea. If you're able to help out further by contributing code that would be greatly appreciated. Otherwise we'll get to it as soon as possible.

          Show
          Andrew Davis added a comment - This sounds like a good idea. If you're able to help out further by contributing code that would be greatly appreciated. Otherwise we'll get to it as soon as possible.
          Hide
          Mike Churchward added a comment - - edited

          Is the only thing that is needed here an update to the database files (/lib/db/install.xml and /lib/db/upgrade.php)?

          Show
          Mike Churchward added a comment - - edited Is the only thing that is needed here an update to the database files (/lib/db/install.xml and /lib/db/upgrade.php)?
          Hide
          Mark Nielsen added a comment -

          Hey Mike, yeah, only install.xml, upgrade.php and a version bump to get the index in there.

          Show
          Mark Nielsen added a comment - Hey Mike, yeah, only install.xml, upgrade.php and a version bump to get the index in there.
          Hide
          Mike Churchward added a comment -

          Dan Poltawski Hi Dan... We can easily provide the patches necessary to upgrade the Moodle database with the necessary index. But, is there a specific process to use in order to ensure we use the correct database version bump?

          Show
          Mike Churchward added a comment - Dan Poltawski Hi Dan... We can easily provide the patches necessary to upgrade the Moodle database with the necessary index. But, is there a specific process to use in order to ensure we use the correct database version bump?
          Hide
          Dan Poltawski added a comment -

          If you increment the version from master at the point which you do the patch, then we (integrators) are very used to dealing with the inevitable version conflicts when we merge it (we'll just increment it again).

          Show
          Dan Poltawski added a comment - If you increment the version from master at the point which you do the patch, then we (integrators) are very used to dealing with the inevitable version conflicts when we merge it (we'll just increment it again).
          Hide
          Mike Churchward added a comment -

          Posted fixes for 2.2. Should I post fixes for 2.3, 2.4 and 2.5 here, or to other issues?

          Show
          Mike Churchward added a comment - Posted fixes for 2.2. Should I post fixes for 2.3, 2.4 and 2.5 here, or to other issues?
          Hide
          Mike Churchward added a comment -

          Added it to all 2.x branches including master.

          Show
          Mike Churchward added a comment - Added it to all 2.x branches including master.
          Hide
          Mike Churchward added a comment -

          Andrew Davis I have included patches for all affected branches. Please review.

          Show
          Mike Churchward added a comment - Andrew Davis I have included patches for all affected branches. Please review.
          Hide
          Justin Filip added a comment -

          Peer review

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

          NOTE: Line numbers are relative to the Moodle 2.2 branch but the notes apply to each branch for this issue.

          Syntax

          Incorrect syntax for an inline comment: "Inline comments must use the "// " (2 slashes + whitespace) style, laid out neatly so that it fits among the code and lines up with it." – http://docs.moodle.org/dev/Coding_style#Inline_comments

          • lib/db/upgrade.php – line 7272 (see line 7277 for correct usage)

          Testing

          Missing testing instructions.

          Git

          "the commit message includes the tracker issue number and ideally the component (ideal format is MDL-xxxx Component: Commit message);" – http://docs.moodle.org/dev/Peer_reviewing_checklist#Git

          Show
          Justin Filip added a comment - Peer review [N] Syntax [-] Output [Y] Whitespace [-] Language [Y] Databases [N] Testing [-] Security [-] Documentation [N] Git [Y] Sanity check NOTE: Line numbers are relative to the Moodle 2.2 branch but the notes apply to each branch for this issue. Syntax Incorrect syntax for an inline comment: "Inline comments must use the "// " (2 slashes + whitespace) style, laid out neatly so that it fits among the code and lines up with it." – http://docs.moodle.org/dev/Coding_style#Inline_comments lib/db/upgrade.php – line 7272 (see line 7277 for correct usage) Testing Missing testing instructions. Git "the commit message includes the tracker issue number and ideally the component (ideal format is MDL-xxxx Component: Commit message);" – http://docs.moodle.org/dev/Peer_reviewing_checklist#Git In this case the component is core_message – http://docs.moodle.org/dev/Frankenstyle#Core_subsystems
          Hide
          Mike Churchward added a comment -

          Syntax issue fixed and committed.
          Git commit message fixed.

          Show
          Mike Churchward added a comment - Syntax issue fixed and committed. Git commit message fixed.
          Hide
          Mike Churchward added a comment -

          Test instructions included.

          Pleas re-peer review.

          Show
          Mike Churchward added a comment - Test instructions included. Pleas re-peer review.
          Hide
          Justin Filip added a comment -

          Thanks, this looks good to go now.

          Andrew Davis I don't have permission to send this up for integration, can you do that? Thanks.

          Show
          Justin Filip added a comment - Thanks, this looks good to go now. Andrew Davis I don't have permission to send this up for integration, can you do that? Thanks.
          Hide
          Andrew Davis added a comment -

          Hi. Sorry, one more change before this goes up for integration. You need to put an if around the index creation. Here is an example from elsewhere in lib/db/upgrade.php

          $table = new xmldb_table('role_assignments');
          $index = new xmldb_index('rolecontext', XMLDB_INDEX_NOTUNIQUE, array('roleid', 'contextid'));
          
          // Conditionally launch add index rolecontext
          if (!$dbman->index_exists($table, $index)) {
              $dbman->add_index($table, $index);
          }
          

          Some people, in particular developers, will have cause to set their version number back. If that occurs the upgrade needs to be able to rerun without error.

          Show
          Andrew Davis added a comment - Hi. Sorry, one more change before this goes up for integration. You need to put an if around the index creation. Here is an example from elsewhere in lib/db/upgrade.php $table = new xmldb_table('role_assignments'); $index = new xmldb_index('rolecontext', XMLDB_INDEX_NOTUNIQUE, array('roleid', 'contextid')); // Conditionally launch add index rolecontext if (!$dbman->index_exists($table, $index)) { $dbman->add_index($table, $index); } Some people, in particular developers, will have cause to set their version number back. If that occurs the upgrade needs to be able to rerun without error.
          Hide
          Mike Churchward added a comment -

          Andrew Davis conditional added to all branches.

          Show
          Mike Churchward added a comment - Andrew Davis conditional added to all branches.
          Hide
          Andrew Davis added a comment -

          Submitting for integration.

          Show
          Andrew Davis added a comment - Submitting for integration.
          Hide
          Damyon Wiese added a comment -

          Thanks Mike,

          Great patch, let the flood gates open!

          Integrated to 23, 24 and master.

          Show
          Damyon Wiese added a comment - Thanks Mike, Great patch, let the flood gates open! Integrated to 23, 24 and master.
          Hide
          Mike Churchward added a comment -

          Thanks. But why not integrate into 2.2 as well? Its still supported until 2.5 comes out, right?

          Show
          Mike Churchward added a comment - Thanks. But why not integrate into 2.2 as well? Its still supported until 2.5 comes out, right?
          Hide
          Dan Poltawski added a comment -

          Only for security fixes, Mike

          Show
          Dan Poltawski added a comment - Only for security fixes, Mike
          Hide
          Dan Poltawski added a comment -

          This issue remains untested without comment of progress, delaying the release.

          I'll take it then..

          Show
          Dan Poltawski added a comment - This issue remains untested without comment of progress, delaying the release. I'll take it then..
          Hide
          Dan Poltawski added a comment -

          Tested on 2.3, 2.4 and master. The upgrade/install creates index successfully.

          Show
          Dan Poltawski added a comment - Tested on 2.3, 2.4 and master. The upgrade/install creates index successfully.
          Hide
          Mike Churchward added a comment -

          Dan Poltawski - can you point me to the documentation that defines which versions and what are supported? Need to have that in my bookmarks...

          The problem with not applying this change to 2.2 is there is no way for us to unproblematically apply it to our codebase. Since a future security fix could also change the version number (necessary for this fix), we would always have conflicts. If it was just code, it would not likely ever have future merge issues.

          Show
          Mike Churchward added a comment - Dan Poltawski - can you point me to the documentation that defines which versions and what are supported? Need to have that in my bookmarks... The problem with not applying this change to 2.2 is there is no way for us to unproblematically apply it to our codebase. Since a future security fix could also change the version number (necessary for this fix), we would always have conflicts. If it was just code, it would not likely ever have future merge issues.
          Hide
          Dan Poltawski added a comment -

          http://docs.moodle.org/dev/Releases
          "Bug fixes for general core bugs in 2.2.x ended December 2012 (12 months).
          Bug fixes for serious security issues in 2.2.x will end June 2013 (18 months)."

          Show
          Dan Poltawski added a comment - http://docs.moodle.org/dev/Releases "Bug fixes for general core bugs in 2.2.x ended December 2012 (12 months). Bug fixes for serious security issues in 2.2.x will end June 2013 (18 months)."
          Hide
          Dan Poltawski added a comment -

          We increment the version number each weekly, and I don't think there were any db updates this week, so if you wanted to you could use this weeks 2.2 version number for the upgrade

          Show
          Dan Poltawski added a comment - We increment the version number each weekly, and I don't think there were any db updates this week, so if you wanted to you could use this weeks 2.2 version number for the upgrade
          Hide
          Mike Churchward added a comment -

          That would work. Thanks.

          Show
          Mike Churchward added a comment - That would work. Thanks.
          Hide
          Eloy Lafuente (stronk7) added a comment -

          I feel myself really alone tonight! So was time to push your fixes upstream!

          "Lest we forget. We will remember them."

          Thanks and ciao!

          Show
          Eloy Lafuente (stronk7) added a comment - I feel myself really alone tonight! So was time to push your fixes upstream! "Lest we forget. We will remember them." Thanks and ciao!

            People

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

              Dates

              • Created:
                Updated:
                Resolved: