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 Master Branch:
      MDL-34933_master

      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)

        Gliffy Diagrams

          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: