launchpad-reviewers team mailing list archive

[Aaron Bentley <aaron@xxxxxxxxxxxxx>]

Aaron Bentley has proposed merging lp:~abentley/launchpad/optimize-spec-query into lp:launchpad.

Commit message:
Optimise specification-for-sprint queries.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

For more details, see:
https://code.launchpad.net/~abentley/launchpad/optimize-spec-query/+merge/133716

= 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
-- 
https://code.launchpad.net/~abentley/launchpad/optimize-spec-query/+merge/133716
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~abentley/launchpad/optimize-spec-query into lp:launchpad.

=== 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-09 17:17:24 +0000
@@ -13,6 +13,7 @@
     'SPECIFICATION_POLICY_DEFAULT_TYPES',
     'SpecificationSet',
     'spec_started_clause',
+    'visible_specification_query',
     ]
 
 from lazr.lifecycle.event import (
@@ -1281,6 +1282,46 @@
                     AccessArtifact.specification_id == Specification.id))))
 
 
+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, 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,
+               AccessPolicyGrantFlat.grantee_id == TeamParticipation.teamID,
+               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):
     """Return a list of Storm expressions for filtering Specifications.
 

=== 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-09 17:17:24 +0000
@@ -40,8 +40,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 +121,11 @@
         """
         # 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(SprintSpecification)
+        query.extend([
+            SprintSpecification.sprintID == self.id,
+            Specification.id == SprintSpecification.specificationID])
         if not filter:
             # filter could be None or [] then we decide the default
             # which for a sprint is to show everything approved
@@ -152,7 +154,7 @@
             query.append(Or(*statuses))
         # Filter for specification text
         query.extend(get_specification_filters(filter))
-        return query
+        return tables, query
 
     def all_specifications(self, user):
         return self.specifications(user, filter=[SpecificationFilter.ALL])
@@ -165,12 +167,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 +182,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`.