Merge lp:~gary/launchpad/bug787294 into lp:launchpad

[Gary Poster]

Translate the raw SQL optimization work Robert did for bug 787294 into code.

This branch translates the raw SQL optimization work Robert did for bug 787294 into code.

This was attempted once before, but the translation was not close enough. This time, I think we are close enough. I can share the SQL we are generating now if desired. The proof is in the pudding: the SQL from the previous attempt consistently takes > 35 seconds on staging; using the SQL generated in this branch, I've gotten < 1 second on staging after the cache is warm. (Of course, with a cold cache, staging takes some gargantuan amount of time.)

lint is happy. ec2 test is happy.

Thanks

Gary

[Henning Eggers (henninge)]

Congratulations on fixing this! The code looks good formally and since it passes tests I approve landing this. I do not pretend to understand the particulars of this query or its optimization so if you want a review on that, feel free to request another one. ;-)

review: Approve

=== modified file 'lib/lp/bugs/model/bugtask.py'
--- lib/lp/bugs/model/bugtask.py	2011-07-27 04:34:19 +0000
+++ lib/lp/bugs/model/bugtask.py	2011-07-30 13:24:38 +0000
@@ -128,7 +128,6 @@
     )
 from lp.bugs.model.bugnomination import BugNomination
 from lp.bugs.model.bugsubscription import BugSubscription
-from lp.bugs.model.structuralsubscription import StructuralSubscription
 from lp.registry.interfaces.distribution import (
     IDistribution,
     IDistributionSet,
@@ -1946,55 +1945,55 @@
                     sqlvalues(personid=params.subscriber.id))
 
         if params.structural_subscriber is not None:
-            ssub_match_product = (
-                BugTask.productID ==
-                StructuralSubscription.productID)
-            ssub_match_productseries = (
-                BugTask.productseriesID ==
-                StructuralSubscription.productseriesID)
-            # Prevent circular import problems.
-            from lp.registry.model.product import Product
-            ssub_match_project = And(
-                Product.projectID ==
-                StructuralSubscription.projectID,
-                BugTask.product == Product.id)
-            ssub_match_distribution = (
-                BugTask.distributionID ==
-                StructuralSubscription.distributionID)
-            ssub_match_sourcepackagename = (
-                BugTask.sourcepackagenameID ==
-                StructuralSubscription.sourcepackagenameID)
-            ssub_match_null_sourcepackagename = (
-                StructuralSubscription.sourcepackagename == None)
-            ssub_match_distribution_with_optional_package = And(
-                ssub_match_distribution, Or(
-                    ssub_match_sourcepackagename,
-                    ssub_match_null_sourcepackagename))
-            ssub_match_distribution_series = (
-                BugTask.distroseriesID ==
-                StructuralSubscription.distroseriesID)
-            ssub_match_milestone = (
-                BugTask.milestoneID ==
-                StructuralSubscription.milestoneID)
-
-            join_clause = Or(
-                ssub_match_product,
-                ssub_match_productseries,
-                ssub_match_project,
-                ssub_match_distribution_with_optional_package,
-                ssub_match_distribution_series,
-                ssub_match_milestone)
-
-            join_tables.append(
-                (Product, LeftJoin(Product, And(
-                                BugTask.productID == Product.id,
-                                Product.active))))
-            join_tables.append(
-                (StructuralSubscription,
-                 Join(StructuralSubscription, join_clause)))
-            extra_clauses.append(
-                'StructuralSubscription.subscriber = %s'
+            # See bug 787294 for the story that led to the query elements
+            # below.  Please change with care.
+            with_clauses.append(
+                '''ss as (SELECT * from StructuralSubscription
+                WHERE StructuralSubscription.subscriber = %s)'''
                 % sqlvalues(params.structural_subscriber))
+            # Prevent circular import problems.
+            from lp.registry.model.product import Product
+            join_tables.append(
+                (Product, LeftJoin(Product, And(
+                                BugTask.productID == Product.id,
+                                Product.active))))
+            join_tables.append(
+                (None,
+                 LeftJoin(
+                    SQL('ss ss1'),
+                    BugTask.product == SQL('ss1.product'))))
+            join_tables.append(
+                (None,
+                 LeftJoin(
+                    SQL('ss ss2'),
+                    BugTask.productseries == SQL('ss2.productseries'))))
+            join_tables.append(
+                (None,
+                 LeftJoin(
+                    SQL('ss ss3'),
+                    Product.project == SQL('ss3.project'))))
+            join_tables.append(
+                (None,
+                 LeftJoin(
+                    SQL('ss ss4'),
+                    And(BugTask.distribution == SQL('ss4.distribution'),
+                        Or(BugTask.sourcepackagename ==
+                            SQL('ss4.sourcepackagename'),
+                           SQL('ss4.sourcepackagename IS NULL'))))))
+            join_tables.append(
+                (None,
+                 LeftJoin(
+                    SQL('ss ss5'),
+                    BugTask.distroseries == SQL('ss5.distroseries'))))
+            join_tables.append(
+                (None,
+                 LeftJoin(
+                    SQL('ss ss6'),
+                    BugTask.milestone == SQL('ss6.milestone'))))
+            extra_clauses.append(
+                "NULL_COUNT("
+                "ARRAY[ss1.id, ss2.id, ss3.id, ss4.id, ss5.id, ss6.id]"
+                ") < 6")
             has_duplicate_results = True
 
         # Remove bugtasks from deactivated products, if necessary.
@@ -2488,9 +2487,10 @@
         origin = [BugTask]
         already_joined = set(origin)
         for table, join in join_tables:
-            if table not in already_joined:
+            if table is None or table not in already_joined:
                 origin.append(join)
-                already_joined.add(table)
+                if table is not None:
+                    already_joined.add(table)
         for table, join in prejoin_tables:
             if table not in already_joined:
                 origin.append(join)
@@ -2544,9 +2544,11 @@
             [query, clauseTables, ignore, decorator, join_tables,
              has_duplicate_results, with_clause] = self.buildQuery(arg)
             origin = self.buildOrigin(join_tables, [], clauseTables)
+            localstore = store
             if with_clause:
-                store = orig_store.with_(with_clause)
-            next_result = store.using(*origin).find(inner_resultrow, query)
+                localstore = orig_store.with_(with_clause)
+            next_result = localstore.using(*origin).find(
+                inner_resultrow, query)
             resultset = resultset.union(next_result)
             # NB: assumes the decorators are all compatible.
             # This may need revisiting if e.g. searches on behalf of different