launchpad-reviewers team mailing list archive

[Robert Collins <robertc@xxxxxxxxxxxxxxxxx>]

Robert Collins has proposed merging lp:~lifeless/launchpad/bug-731679 into lp:launchpad.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

For more details, see:
https://code.launchpad.net/~lifeless/launchpad/bug-731679/+merge/52631

Change from a join to a with query for establishing the sourcepackagenames that are relevant to bugsearches. postgresql thinks that spph is 12M rows of related data even though the constraints on the query mean its at most 1 row per sourcepackagename. The updated search is 800ms on qastaging for me (down from 14000ms).
-- 
https://code.launchpad.net/~lifeless/launchpad/bug-731679/+merge/52631
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~lifeless/launchpad/bug-731679 into lp:launchpad.

=== modified file 'lib/lp/bugs/model/bugtask.py'
--- lib/lp/bugs/model/bugtask.py	2011-03-07 22:07:46 +0000
+++ lib/lp/bugs/model/bugtask.py	2011-03-09 03:24:56 +0000
@@ -1768,6 +1768,7 @@
         join_tables = []
         decorators = []
         has_duplicate_results = False
+        with_clauses = []
 
         # These arguments can be processed in a loop without any other
         # special handling.
@@ -1959,8 +1960,6 @@
                                 Product.active))))
 
         if params.component:
-            clauseTables += [SourcePackagePublishingHistory,
-                             SourcePackageRelease]
             distroseries = None
             if params.distribution:
                 distroseries = params.distribution.currentseries
@@ -1978,19 +1977,21 @@
             distro_archive_ids = [
                 archive.id
                 for archive in distroseries.distribution.all_distro_archives]
-            extra_clauses.extend(["""
-            BugTask.sourcepackagename =
-                SourcePackageRelease.sourcepackagename AND
-            SourcePackageRelease.id =
-                SourcePackagePublishingHistory.sourcepackagerelease AND
-            SourcePackagePublishingHistory.distroseries = %s AND
-            SourcePackagePublishingHistory.archive IN %s AND
-            SourcePackagePublishingHistory.component IN %s AND
-            SourcePackagePublishingHistory.status = %s
-            """ % sqlvalues(distroseries,
-                            distro_archive_ids,
-                            component_ids,
-                            PackagePublishingStatus.PUBLISHED)])
+            with_clauses.append("""spns as (
+                SELECT sourcepackagename from SourcePackagePublishingHistory
+                JOIN SourcePackageRelease on SourcePackageRelease.id =
+                    SourcePackagePublishingHistory.sourcepackagerelease AND
+                SourcePackagePublishingHistory.distroseries = %s AND
+                SourcePackagePublishingHistory.archive IN %s AND
+                SourcePackagePublishingHistory.component IN %s AND
+                SourcePackagePublishingHistory.status = %s
+                )""" % sqlvalues(distroseries,
+                                distro_archive_ids,
+                                component_ids,
+                                PackagePublishingStatus.PUBLISHED))
+            extra_clauses.append(
+                """BugTask.sourcepackagename in (
+                    select sourcepackagename from spns)""")
 
         upstream_clause = self._buildUpstreamClause(params)
         if upstream_clause:
@@ -2135,9 +2136,13 @@
                 for decor in decorators:
                     obj = decor(obj)
                 return obj
+        if with_clauses:
+            with_clause = SQL(', '.join(with_clauses))
+        else:
+            with_clause = None
         return (
             query, clauseTables, orderby_arg, decorator, join_tables,
-            has_duplicate_results)
+            has_duplicate_results, with_clause)
 
     def _buildUpstreamClause(self, params):
         """Return an clause for returning upstream data if the data exists.
@@ -2429,9 +2434,11 @@
         :param params: A BugTaskSearchParams instance.
         :param args: optional additional BugTaskSearchParams instances,
         """
-        store = IStore(BugTask)
+        orig_store = store = IStore(BugTask)
         [query, clauseTables, orderby, bugtask_decorator, join_tables,
-        has_duplicate_results] = self.buildQuery(params)
+        has_duplicate_results, with_clause] = self.buildQuery(params)
+        if with_clause:
+            store = store.with_(with_clause)
         if len(args) == 0:
             if has_duplicate_results:
                 origin = self.buildOrigin(join_tables, [], clauseTables)
@@ -2459,9 +2466,10 @@
         decorators = [bugtask_decorator]
         for arg in args:
             [query, clauseTables, ignore, decorator, join_tables,
-             has_duplicate_results] = self.buildQuery(arg)
+             has_duplicate_results, with_clause] = self.buildQuery(arg)
             origin = self.buildOrigin(join_tables, [], clauseTables)
-            next_result = store.using(*origin).find(inner_resultrow, query)
+            next_result = orig_store.with_(with_clause).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

=== modified file 'versions.cfg'
--- versions.cfg	2011-03-01 10:33:03 +0000
+++ versions.cfg	2011-03-09 03:24:56 +0000
@@ -72,7 +72,7 @@
 Sphinx = 1.0.7
 soupmatchers = 0.1r53
 sourcecodegen = 0.6.9
-storm = 0.18
+storm = 0.18.0.99
 testtools = 0.9.8
 transaction = 1.0.0
 Twisted = 10.2.0-4395fix-1-4909