From 0ce00c6d169159c5ba65f1fec8b4ebb6217b2bba Mon Sep 17 00:00:00 2001
From: Toby Zerner <toby.zerner@gmail.com>
Date: Sun, 12 Nov 2017 21:28:17 +1030
Subject: [PATCH] Performance: Refactor SQL that pins unread sticky posts to
 the top

Ordering by `is_sticky and (unread subquery) desc` removes MySQL's
ability to use an index for ordering by `last_time`, which triggers a
filesort across the whole discussions table which is BAD.

This commit uses a union to add all stickied discussions to the query.
The results of the unioned queries are then ordered by the `is_sticky
and (unread subquery)` criteria, so the filesort only takes place on
a maximum of limit * 2 rows. Big performance win when you get up to
thousands of discussions!
---
 .../Listener/PinStickiedDiscussionsToTop.php  | 58 ++++++++++---------
 1 file changed, 31 insertions(+), 27 deletions(-)

diff --git a/extensions/sticky/src/Listener/PinStickiedDiscussionsToTop.php b/extensions/sticky/src/Listener/PinStickiedDiscussionsToTop.php
index 4c284c36b..1f0f54e8d 100755
--- a/extensions/sticky/src/Listener/PinStickiedDiscussionsToTop.php
+++ b/extensions/sticky/src/Listener/PinStickiedDiscussionsToTop.php
@@ -25,7 +25,7 @@ class PinStickiedDiscussionsToTop
     public function subscribe(Dispatcher $events)
     {
         $events->listen(ConfigureDiscussionGambits::class, [$this, 'addStickyGambit']);
-        $events->listen(ConfigureDiscussionSearch::class, [$this, 'reorderSearch']);
+        $events->listen(ConfigureDiscussionSearch::class, [$this, 'reorderSearch'], -100);
     }
 
     /**
@@ -45,18 +45,15 @@ class PinStickiedDiscussionsToTop
             $search = $event->search;
             $query = $search->getQuery();
 
-            // TODO: ideally we might like to consider an event in core that is
-            // fired before the sort criteria is applied to the query (ie.
-            // immediately after gambits are applied) so that we can add the
-            // following order logic to the start without using array_unshift.
-
             if (! is_array($query->orders)) {
                 $query->orders = [];
             }
 
             // If we are viewing a specific tag, then pin all stickied
             // discussions to the top no matter what.
-            foreach ($search->getActiveGambits() as $gambit) {
+            $gambits = $search->getActiveGambits();
+
+            foreach ($gambits as $gambit) {
                 if ($gambit instanceof TagGambit) {
                     array_unshift($query->orders, ['column' => 'is_sticky', 'direction' => 'desc']);
 
@@ -64,28 +61,35 @@ class PinStickiedDiscussionsToTop
                 }
             }
 
-            // Otherwise, if we are viewing "all discussions" or similar, only
-            // pin stickied discussions to the top if they are unread. To do
-            // this we construct an order clause containing a subquery which
-            // determines whether or not the discussion is unread.
-            $subquery = $query->newQuery()
-                ->selectRaw(1)
-                ->from('users_discussions as sticky')
-                ->whereRaw('sticky.discussion_id = discussions.id')
-                ->where('sticky.user_id', '=', $search->getActor()->id)
-                ->where(function ($query) {
-                    $query->whereNull('sticky.read_number')
-                        ->orWhereRaw('discussions.last_post_number > sticky.read_number');
-                })
-                ->where('discussions.last_time', '>', $search->getActor()->read_time ?: 0);
+            // Otherwise, if we are viewing "all discussions", only pin stickied
+            // discussions to the top if they are unread. To do this in a
+            // performant way we create another query which will select all
+            // stickied discussions, marry them into the main query, and then
+            // reorder the unread ones up to the top.
+            if (empty($gambits)) {
+                $sticky = clone $query;
+                $sticky->where('is_sticky', true);
+                $sticky->limit = $sticky->offset = $sticky->orders = null;
 
-            array_unshift($query->orders, [
-                'type' => 'raw',
-                'sql' => "(is_sticky and exists ({$subquery->toSql()})) desc"
-            ]);
+                $query->union($sticky);
 
-            $orderBindings = $query->getRawBindings()['order'];
-            $query->setBindings(array_merge($subquery->getBindings(), $orderBindings), 'order');
+                $read = $query->newQuery()
+                    ->selectRaw(1)
+                    ->from('users_discussions as sticky')
+                    ->whereRaw('sticky.discussion_id = id')
+                    ->where('sticky.user_id', '=', $search->getActor()->id)
+                    ->whereRaw('sticky.read_number >= last_post_number');
+
+                // Add the bindings manually (rather than as the second
+                // argument in orderByRaw) for now due to a bug in Laravel which
+                // would add the bindings in the wrong order.
+                $query->orderByRaw('is_sticky and not exists ('.$read->toSql().') and last_time > ? desc')
+                    ->addBinding(array_merge($read->getBindings(), [$search->getActor()->read_time ?: 0]), 'union');
+
+                $query->unionOrders = array_merge($query->unionOrders, $query->orders);
+                $query->unionLimit = $query->limit;
+                $query->unionOffset = $query->offset;
+            }
         }
     }
 }