launchpad-reviewers team mailing list archive

[Colin Watson <mp+439396@xxxxxxxxxxxxxxxxxx>]

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

Commit message:
Optimize Person.visible_specifications

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  Bug #1920045 in Launchpad itself: "Blueprints main page times out"
  https://bugs.launchpad.net/launchpad/+bug/1920045

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

I noticed `Person.visible_specifications` timing out while I was deleting some spam.  It seems that the query constructed here caused the PostgreSQL planner to use a sequential scan over the whole `Specification` table, which is very slow.  Moving the `Specification.id.is_in(...)` clause into a `UNION` avoids this.

This may also help with LP: #1920045, although I'm less sure of that.
-- 
Your team Launchpad code reviewers is requested to review the proposed merge of ~cjwatson/launchpad:optimize-person-visible-specifications into launchpad:master.

diff --git a/lib/lp/blueprints/model/specificationsearch.py b/lib/lp/blueprints/model/specificationsearch.py
index 243e4c9..1293fa7 100644
--- a/lib/lp/blueprints/model/specificationsearch.py
+++ b/lib/lp/blueprints/model/specificationsearch.py
@@ -23,6 +23,7 @@ from storm.expr import (
     Or,
     Select,
     Table,
+    Union,
 )
 from storm.locals import SQL, Desc
 from zope.component import getUtility
@@ -70,6 +71,7 @@ def search_specifications(
     need_people=True,
     need_branches=True,
     need_workitems=False,
+    base_id_clauses=None,
 ):
     store = IStore(Specification)
     if not default_acceptance:
@@ -93,22 +95,38 @@ def search_specifications(
 
     if not tables:
         tables = [Specification]
-    clauses = base_clauses
     product_tables, product_clauses = get_specification_active_product_filter(
         context
     )
     tables.extend(product_tables)
-    clauses.extend(product_clauses)
-    # If there are any base or product clauses, they typically have good
-    # selectivity, so use a CTE to force PostgreSQL to calculate them
-    # up-front rather than doing a sequential scan for visible
-    # specifications.
-    if clauses:
+
+    # If there are any base clauses, they typically have good selectivity,
+    # so use a CTE to force PostgreSQL to calculate them up-front rather
+    # than doing a sequential scan for visible specifications.
+    if base_clauses or base_id_clauses:
         RelevantSpecification = Table("RelevantSpecification")
+        # Base ID clauses (that is, those that search for specifications
+        # whose IDs are in a set computed by a subquery) pose an
+        # optimization problem.  If we include them in a disjunction with
+        # other clauses that search for well-indexed columns such as the
+        # owner, then rather than using each of the appropriate indexes and
+        # combining them, the PostgreSQL planner will perform a sequential
+        # scan over the whole Specification table.  To avoid this, we put
+        # such clauses in a separate query and UNION them together.
         relevant_specification_cte = WithMaterialized(
             RelevantSpecification.name,
             store,
-            Select(Specification.id, And(clauses), tables=tables),
+            Union(
+                *(
+                    Select(
+                        Specification.id,
+                        And(clauses + product_clauses),
+                        tables=tables,
+                    )
+                    for clauses in (base_clauses, base_id_clauses)
+                    if clauses
+                )
+            ),
         )
         store = store.with_(relevant_specification_cte)
         tables = [
@@ -119,6 +137,9 @@ def search_specifications(
             ),
         ]
         clauses = []
+    else:
+        clauses = list(product_clauses)
+
     clauses.extend(get_specification_privacy_filter(user))
     clauses.extend(get_specification_filters(spec_filter))
 
diff --git a/lib/lp/registry/model/person.py b/lib/lp/registry/model/person.py
index 8bf791f..5c02c46 100644
--- a/lib/lp/registry/model/person.py
+++ b/lib/lp/registry/model/person.py
@@ -868,6 +868,7 @@ class Person(
         if filter.intersection(roles) == set():
             filter.update(roles)
         role_clauses = []
+        id_clauses = []
         if SpecificationFilter.CREATOR in filter:
             role_clauses.append(Specification.owner == self)
         if SpecificationFilter.ASSIGNEE in filter:
@@ -877,7 +878,7 @@ class Person(
         if SpecificationFilter.APPROVER in filter:
             role_clauses.append(Specification._approver == self)
         if SpecificationFilter.SUBSCRIBER in filter:
-            role_clauses.append(
+            id_clauses.append(
                 Specification.id.is_in(
                     Select(
                         SpecificationSubscription.specification_id,
@@ -910,6 +911,7 @@ class Person(
             need_people=need_people,
             need_branches=need_branches,
             need_workitems=need_workitems,
+            base_id_clauses=id_clauses,
         )
 
     # XXX: Tom Berger 2008-04-14 bug=191799: