launchpad-reviewers team mailing list archive

[Colin Watson <cjwatson@xxxxxxxxxxxxx>]

Colin Watson has proposed merging ~cjwatson/launchpad:getPrecachedPersons-performance into launchpad:master.

Commit message:
Fix PersonSet._getPrecachedPersons performance for large teams

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  Bug #1921727 in Launchpad itself: "Error 503 when trying to iterate over lp.people[team].participants"
  https://bugs.launchpad.net/launchpad/+bug/1921727

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

Looking up membership of a large team can tip over a performance cliff where PostgreSQL decides to loop over `person_sorting_idx` first and select team membership from that, which performs badly.  Use a CTE to force it to look at more selective conditions first.
-- 
Your team Launchpad code reviewers is requested to review the proposed merge of ~cjwatson/launchpad:getPrecachedPersons-performance into launchpad:master.

diff --git a/lib/lp/registry/model/person.py b/lib/lp/registry/model/person.py
index 6785698..4127c91 100644
--- a/lib/lp/registry/model/person.py
+++ b/lib/lp/registry/model/person.py
@@ -58,6 +58,7 @@ from storm.expr import (
     Alias,
     And,
     Coalesce,
+    Column,
     Desc,
     Exists,
     In,
@@ -68,6 +69,7 @@ from storm.expr import (
     Or,
     Select,
     SQL,
+    Table,
     Union,
     Upper,
     With,
@@ -3912,8 +3914,8 @@ class PersonSet:
         """Lookup all members of the team with optional precaching.
 
         :param store: Provide ability to specify the store.
-        :param origin: List of storm tables and joins. This list will be
-            appended to. The Person table is required.
+        :param origin: List of storm tables and joins. The Person table is
+            required.
         :param conditions: Storm conditions for tables in origin.
         :param need_karma: The karma attribute will be cached.
         :param need_ubuntu_coc: The is_ubuntu_coc_signer attribute will be
@@ -3928,6 +3930,17 @@ class PersonSet:
         """
         if store is None:
             store = IStore(Person)
+        # The conditions we've been given are almost certainly more
+        # selective that anything PostgreSQL might guess at.  Use a CTE to
+        # ensure that it looks at them first.
+        store = store.with_(With(
+            'RelevantPerson',
+            Select(Person.id, where=conditions, tables=origin)))
+        relevant_person = Table('RelevantPerson')
+        origin = [
+            Person,
+            Join(relevant_person, Column('id', relevant_person) == Person.id),
+            ]
         columns = [Person]
         decorators = []
         if need_karma or need_api:
@@ -3999,10 +4012,10 @@ class PersonSet:
         if len(columns) == 1:
             column = columns[0]
             # Return a simple ResultSet
-            return store.using(*origin).find(column, conditions)
+            return store.using(*origin).find(column)
         # Adapt the result into a cached Person.
         columns = tuple(columns)
-        raw_result = store.using(*origin).find(columns, conditions)
+        raw_result = store.using(*origin).find(columns)
         if need_api:
             # Collections exported on the API need to be sorted in order to
             # provide stable batching.  In some ways Person.name might make