MDL-36941 core_message: improved performance of helper::get_messages()

Improved the query to use the 'convhash' field as well as adding an index. Also fixed issue where 'timeread' was hardcoded as 0.
mpetrowi · Mar 23, 2018 · e159b53 · e159b53
1 parent d0d1e97
commit e159b53
Show file tree

Hide file tree

Showing 4 changed files with 53 additions and 23 deletions.
diff --git a/lib/db/install.xml b/lib/db/install.xml
@@ -608,6 +608,9 @@
         <KEY NAME="useridfrom" TYPE="foreign" FIELDS="useridfrom" REFTABLE="user" REFFIELDS="id"/>
         <KEY NAME="conversationid" TYPE="foreign" FIELDS="conversationid" REFTABLE="message_conversations" REFFIELDS="id"/>
       </KEYS>
+      <INDEXES>
+        <INDEX NAME="conversationid_timecreated" UNIQUE="false" FIELDS="conversationid, timecreated"/>
+      </INDEXES>
     </TABLE>
     <TABLE NAME="message_conversations" COMMENT="Stores all message conversations">
       <FIELDS>

diff --git a/lib/db/upgrade.php b/lib/db/upgrade.php
@@ -2176,5 +2176,19 @@ function xmldb_main_upgrade($oldversion) {
         upgrade_main_savepoint(true, 2018032200.06);
     }
 
+    if ($oldversion < 2018032200.07) {
+        // Define table 'messages' to add an index to.
+        $table = new xmldb_table('messages');
+
+        // Conditionally launch add index.
+        $index = new xmldb_index('conversationid_timecreated', XMLDB_INDEX_NOTUNIQUE, array('conversationid, timecreated'));
+        if (!$dbman->index_exists($table, $index)) {
+            $dbman->add_index($table, $index);
+        }
+
+        // Main savepoint reached.
+        upgrade_main_savepoint(true, 2018032200.07);
+    }
+
     return true;
 }
diff --git a/message/classes/helper.php b/message/classes/helper.php
@@ -51,39 +51,47 @@ public static function get_messages($userid, $otheruserid, $timedeleted = 0, $li
                                         $sort = 'timecreated ASC', $timefrom = 0, $timeto = 0) {
         global $DB;
 
-        $sql = "SELECT m.id, m.useridfrom, mdm.userid as useridto, m.subject, m.fullmessage, m.fullmessagehtml,
-                       m.fullmessageformat, m.smallmessage, m.timecreated, 0 as timeread
-                  FROM {messages} m
-            INNER JOIN {message_conversations} md
-                    ON md.id = m.conversationid
-            INNER JOIN {message_conversation_members} mdm
-                    ON mdm.conversationid = md.id";
-        $params = [];
+        $hash = self::get_conversation_hash([$userid, $otheruserid]);
+
+        $sql = "SELECT m.id, m.useridfrom, m.subject, m.fullmessage, m.fullmessagehtml,
+                       m.fullmessageformat, m.smallmessage, m.timecreated, muaread.timecreated AS timeread
+                  FROM {message_conversations} mc
+            INNER JOIN {messages} m
+                    ON m.conversationid = mc.id
+             LEFT JOIN {message_user_actions} muaread
+                    ON (muaread.messageid = m.id
+                   AND muaread.userid = :userid1
+                   AND muaread.action = :readaction)";
+        $params = ['userid1' => $userid, 'readaction' => api::MESSAGE_ACTION_READ, 'convhash' => $hash];
 
         if (empty($timedeleted)) {
             $sql .= " LEFT JOIN {message_user_actions} mua
-                             ON (mua.messageid = m.id AND mua.userid = ? AND mua.action = ? AND mua.timecreated is NOT NULL)";
-            $params[] = $userid;
-            $params[] = api::MESSAGE_ACTION_DELETED;
+                             ON (mua.messageid = m.id
+                            AND mua.userid = :userid2
+                            AND mua.action = :deleteaction
+                            AND mua.timecreated is NOT NULL)";
         } else {
             $sql .= " INNER JOIN {message_user_actions} mua
-                              ON (mua.messageid = m.id AND mua.userid = ? AND mua.action = ? AND mua.timecreated = ?)";
-            $params[] = $userid;
-            $params[] = api::MESSAGE_ACTION_DELETED;
-            $params[] = $timedeleted;
+                              ON (mua.messageid = m.id
+                             AND mua.userid = :userid2
+                             AND mua.action = :deleteaction
+                             AND mua.timecreated = :timedeleted)";
+            $params['timedeleted'] = $timedeleted;
         }
 
-        $sql .= " WHERE (m.useridfrom = ? AND mdm.userid = ? OR m.useridfrom = ? AND mdm.userid = ?)";
-        $params = array_merge($params, [$userid, $otheruserid, $otheruserid, $userid]);
+        $params['userid2'] = $userid;
+        $params['deleteaction'] = api::MESSAGE_ACTION_DELETED;
+
+        $sql .= " WHERE mc.convhash = :convhash";
 
         if (!empty($timefrom)) {
-            $sql .= " AND m.timecreated >= ?";
-            $params[] = $timefrom;
+            $sql .= " AND m.timecreated >= :timefrom";
+            $params['timefrom'] = $timefrom;
         }
 
         if (!empty($timeto)) {
-            $sql .= " AND m.timecreated <= ?";
-            $params[] = $timeto;
+            $sql .= " AND m.timecreated <= :timeto";
+            $params['timeto'] = $timeto;
         }
 
         if (empty($timedeleted)) {
@@ -92,7 +100,12 @@ public static function get_messages($userid, $otheruserid, $timedeleted = 0, $li
 
         $sql .= " ORDER BY m.$sort";
 
-        return $DB->get_records_sql($sql, $params, $limitfrom, $limitnum);
+        $messages = $DB->get_records_sql($sql, $params, $limitfrom, $limitnum);
+        foreach ($messages as &$message) {
+            $message->useridto = ($message->useridfrom == $userid) ? $otheruserid : $userid;
+        }
+
+        return $messages;
     }
 
     /**

diff --git a/version.php b/version.php
@@ -29,7 +29,7 @@
 
 defined('MOODLE_INTERNAL') || die();
 
-$version  = 2018032200.06;              // YYYYMMDD      = weekly release date of this DEV branch.
+$version  = 2018032200.07;              // YYYYMMDD      = weekly release date of this DEV branch.
                                         //         RR    = release increments - 00 in DEV branches.
                                         //           .XX = incremental changes.