Merge lp:~abentley/launchpad/optimize-spec-query into lp:launchpad

[Aaron Bentley]

Optimise specification-for-sprint queries.

= Summary =
Fix bug #1075365: Timeout when trying to visit previous sprint pages

== Proposed fix ==
Optimize specification sprint queries.

== Pre-implementation notes ==
Actual query profiling done with deryck.

== LOC Rationale ==
Part of Private Projects

== Implementation details ==
Flatten all the privacy filter code using LEFT JOINs, so that the query planner has more freedom to choose the correct plan.

This code introduces visible_specification_query, but does not remove get_specification_privacy_filter, because we want to first be sure that visible_specification_query is performant on production. We have measured its performance at 276 ms on staging and 543ms on production.

Because visible_specification_query uses left joins, it and spec_filter_clause return both a list of tables and a list of clauses.

DISTINCT is required because a user may have multiple AccessPolicyGrantFlat entries for a given artifact. In what is arguably buggy behaviour, Storm does not specify ON for SELECT DISTINCT when a custom ordering is specified and the result set is configured with distinct=True. Therefore, Sprint.specifications specifies the particular distinct clause to use.

== Tests ==
bin/test test_sprint

== Demo and Q/A ==
https://staging.launchpad.net/sprints/uds-r should not time out. (Cannot use wiht qastaging, because it has stale data).

= Launchpad lint =

Checking for conflicts and issues in changed files.

Linting changed files:
  lib/lp/blueprints/model/specification.py
  lib/lp/blueprints/model/sprint.py

[Brad Crittenden (bac)]

This approach looks reasonable to me Aaron. Please remember to follow up to remove the obsoleted code after QA.

review: Approve (code)

[Aaron Bentley (abentley)]

As discussed on IRC, the "obsolete" code currently has other call sites. I do plan to remove it, but that will involve more changes.

=== modified file 'lib/lp/blueprints/model/specification.py'
--- lib/lp/blueprints/model/specification.py	2012-10-30 05:07:30 +0000
+++ lib/lp/blueprints/model/specification.py	2012-11-12 16:53:21 +0000
@@ -13,6 +13,7 @@
     'SPECIFICATION_POLICY_DEFAULT_TYPES',
     'SpecificationSet',
     'spec_started_clause',
+    'visible_specification_query',
     ]
 
 from lazr.lifecycle.event import (
@@ -1281,17 +1282,63 @@
                     AccessArtifact.specification_id == Specification.id))))
 
 
-def get_specification_filters(filter):
+def visible_specification_query(user):
+    """Return a Storm expression and list of tables for filtering
+    specifications by privacy.
+
+    :param user: A Person ID or a column reference.
+    :return: A tuple of tables, clauses to filter out specifications that the
+        user cannot see.
+    """
+    from lp.registry.model.product import Product
+    from lp.registry.model.accesspolicy import (
+        AccessArtifact,
+        AccessPolicy,
+        AccessPolicyGrantFlat,
+        )
+    tables = [
+        Specification,
+        LeftJoin(Product, Specification.productID == Product.id),
+        LeftJoin(AccessPolicy, And(
+            Or(Specification.productID == AccessPolicy.product_id,
+               Specification.distributionID ==
+               AccessPolicy.distribution_id),
+            Specification.information_type == AccessPolicy.type)),
+        LeftJoin(AccessPolicyGrantFlat,
+                 AccessPolicy.id == AccessPolicyGrantFlat.policy_id),
+        LeftJoin(
+            TeamParticipation,
+            And(AccessPolicyGrantFlat.grantee == TeamParticipation.teamID,
+                TeamParticipation.person == user)),
+        LeftJoin(AccessArtifact,
+                 AccessPolicyGrantFlat.abstract_artifact_id ==
+                 AccessArtifact.id)
+        ]
+    clauses = [
+        Or(Specification.information_type.is_in(PUBLIC_INFORMATION_TYPES),
+           And(AccessPolicyGrantFlat.id != None,
+               TeamParticipation.personID != None,
+               Or(AccessPolicyGrantFlat.abstract_artifact == None,
+                  AccessArtifact.specification_id == Specification.id))),
+        Or(Specification.product == None, Product.active == True)]
+    return tables, clauses
+
+
+def get_specification_filters(filter, assume_product_active=False):
     """Return a list of Storm expressions for filtering Specifications.
 
     :param filters: A collection of SpecificationFilter and/or strings.
         Strings are used for text searches.
+    :param assume_product_active: If True, assume the Product is active,
+        instead of ensuring it is active.
     """
     from lp.registry.model.product import Product
+    clauses = []
     # If Product is used, it must be active.
-    clauses = [Or(Specification.product == None,
-                  Not(Specification.productID.is_in(Select(Product.id,
-                      Product.active == False))))]
+    if not assume_product_active:
+        clauses.extend([Or(Specification.product == None,
+                        Not(Specification.productID.is_in(Select(Product.id,
+                        Product.active == False))))])
     # ALL is the trump card.
     if SpecificationFilter.ALL in filter:
         return clauses

=== modified file 'lib/lp/blueprints/model/sprint.py'
--- lib/lp/blueprints/model/sprint.py	2012-10-15 23:35:20 +0000
+++ lib/lp/blueprints/model/sprint.py	2012-11-12 16:53:21 +0000
@@ -17,6 +17,7 @@
     )
 from storm.locals import (
     Desc,
+    Join,
     Or,
     Store,
     )
@@ -40,8 +41,8 @@
     )
 from lp.blueprints.model.specification import (
     get_specification_filters,
-    get_specification_privacy_filter,
     HasSpecificationsMixin,
+    visible_specification_query,
     )
 from lp.blueprints.model.sprintattendance import SprintAttendance
 from lp.blueprints.model.sprintspecification import SprintSpecification
@@ -121,9 +122,12 @@
         """
         # import here to avoid circular deps
         from lp.blueprints.model.specification import Specification
-        query = [SprintSpecification.sprintID == self.id,
-                 SprintSpecification.specificationID == Specification.id]
-        query.append(get_specification_privacy_filter(user))
+        tables, query = visible_specification_query(user)
+        tables.append(Join(
+            SprintSpecification,
+            SprintSpecification.specification == Specification.id
+        ))
+        query.extend([SprintSpecification.sprintID == self.id])
         if not filter:
             # filter could be None or [] then we decide the default
             # which for a sprint is to show everything approved
@@ -151,8 +155,9 @@
         if len(statuses) > 0:
             query.append(Or(*statuses))
         # Filter for specification text
-        query.extend(get_specification_filters(filter))
-        return query
+        query.extend(
+            get_specification_filters(filter, assume_product_active=True))
+        return tables, query
 
     def all_specifications(self, user):
         return self.specifications(user, filter=[SpecificationFilter.ALL])
@@ -165,12 +170,14 @@
         assert not prejoin_people
         if filter is None:
             filter = set([SpecificationFilter.ACCEPTED])
-        query = self.spec_filter_clause(user, filter=filter)
+        tables, query = self.spec_filter_clause(user, filter)
         # import here to avoid circular deps
         from lp.blueprints.model.specification import Specification
-        results = Store.of(self).find(Specification, *query)
+        store = Store.of(self)
+        results = store.using(*tables).find(Specification, *query)
         if sort == SpecificationSort.DATE:
             order = (Desc(SprintSpecification.date_created), Specification.id)
+            distinct = [SprintSpecification.date_created, Specification.id]
             # we need to establish if the listing will show specs that have
             # been decided only, or will include proposed specs.
             if (SpecificationFilter.ALL not in filter and
@@ -178,19 +185,21 @@
                 # this will show only decided specs so use the date the spec
                 # was accepted or declined for the sprint
                 order = (Desc(SprintSpecification.date_decided),) + order
+                distinct = [SprintSpecification.date_decided] + distinct
             results = results.order_by(*order)
         else:
             assert sort is None or sort == SpecificationSort.PRIORITY
             # fall back to default, which is priority, descending.
+            distinct = True
         if quantity is not None:
             results = results[:quantity]
-        return results
+        return results.config(distinct=distinct)
 
     def specificationLinks(self, filter=None):
         """See `ISprint`."""
-        query = self.spec_filter_clause(None, filter=filter)
-        result = Store.of(self).find(SprintSpecification, *query)
-        return result
+        tables, query = self.spec_filter_clause(None, filter=filter)
+        t_set = Store.of(self).using(*tables)
+        return t_set.find(SprintSpecification, *query).config(distinct=True)
 
     def getSpecificationLink(self, speclink_id):
         """See `ISprint`.