launchpad-reviewers team mailing list archive

[Colin Watson <mp+436194@xxxxxxxxxxxxxxxxxx>]

Colin Watson has proposed merging ~cjwatson/launchpad:optimize-person-sorting into launchpad:master.

Commit message:
Optimize short-circuit case in PersonSet.findPerson

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

For more details, see:
https://code.launchpad.net/~cjwatson/launchpad/+git/launchpad/+merge/436194

Ordering this query by `Person._storm_sortingColumns` (a somewhat dubiously-named attribute that is in fact just `(Person.displayname, Person.name)` without the `person_sort_key` function) causes PostgreSQL to use a very slow plan that has to sort the whole `Person` table before applying the row limit:

   Limit  (cost=798114.96..798115.15 rows=76 width=1049)
     ->  Sort  (cost=798114.96..806603.87 rows=3395564 width=1049)
           Sort Key: person.displayname, person.name
           ->  Merge Join  (cost=1.66..675060.95 rows=3395564 width=1049)
                 Merge Cond: (account.id = person.account)
                 ->  Index Scan using account_pkey on account  (cost=0.43..287109.00 rows=3416242 width=4)
                       Filter: (status <> ALL ('{5,30,40,50,60}'::integer[]))
                 ->  Index Scan using person__account__key on person  (cost=0.43..330359.42 rows=7441191 width=1049)
                       Filter: ((teamowner IS NULL) AND (merged IS NULL))

Using `Person.sortingColumns` instead is more in line with what this method does in the non-short-circuit case, and it produces a much more sensible plan:

   Limit  (cost=0.99..111.69 rows=76 width=1081)
     ->  Nested Loop  (cost=0.99..4946116.34 rows=3395564 width=1081)
           ->  Index Scan using person_sorting_idx on person  (cost=0.56..505451.00 rows=7441191 width=1049)
                 Filter: ((teamowner IS NULL) AND (merged IS NULL))
           ->  Index Scan using account_pkey on account  (cost=0.43..0.48 rows=1 width=4)
                 Index Cond: (id = person.account)
                 Filter: (status <> ALL ('{5,30,40,50,60}'::integer[]))
-- 
Your team Launchpad code reviewers is requested to review the proposed merge of ~cjwatson/launchpad:optimize-person-sorting into launchpad:master.

diff --git a/lib/lp/registry/model/person.py b/lib/lp/registry/model/person.py
index 175ccfc..1eef067 100644
--- a/lib/lp/registry/model/person.py
+++ b/lib/lp/registry/model/person.py
@@ -4417,7 +4417,7 @@ class PersonSet:
         # Short circuit for returning all users in order
         if not text:
             results = store.find(Person, base_query)
-            return results.order_by(Person._storm_sortingColumns)
+            return results.order_by(Person.sortingColumns)
 
         # We use a UNION here because this makes things *a lot* faster
         # than if we did a single SELECT with the two following clauses