launchpad-reviewers team mailing list archive

[William Grant <me@xxxxxxxxxxxxxxxxxx>]

William Grant has proposed merging lp:~wgrant/launchpad/bug-899776 into lp:launchpad.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  Bug #899776 in Launchpad itself: "Distribution:+bugs timeout searching by component universe"
  https://bugs.launchpad.net/launchpad/+bug/899776

For more details, see:
https://code.launchpad.net/~wgrant/launchpad/bug-899776/+merge/103806

This branch rewrites the package component filtering part of bugtasksearch. Old plan is https://pastebin.canonical.com/65085/, new is https://pastebin.canonical.com/65104/ -- it's about 200x faster. The full improvement is only seen with a new index that is being done in a separate branch, but it's still substantially faster without it.

I changed the SELECT to use the relatively newly denormalised SPPH.sourcepackagename, allowing rapid direct indexing into the table. It's now a normal subquery instead of a CTE, after extensive performance testing. In addition, the archive set is minimised: if we're just searching for main and universe, we don't need to search the partner archive.
-- 
https://code.launchpad.net/~wgrant/launchpad/bug-899776/+merge/103806
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~wgrant/launchpad/bug-899776 into lp:launchpad.

=== modified file 'lib/lp/bugs/model/bugtasksearch.py'
--- lib/lp/bugs/model/bugtasksearch.py	2012-04-26 05:00:19 +0000
+++ lib/lp/bugs/model/bugtasksearch.py	2012-04-27 06:44:19 +0000
@@ -91,6 +91,7 @@
     NULL,
     )
 from lp.soyuz.enums import PackagePublishingStatus
+from lp.soyuz.model.publishing import SourcePackagePublishingHistory
 
 
 # This abstracts most of the columns involved in search so we can switch
@@ -737,29 +738,30 @@
                 "distribution or distroseries.")
 
         if zope_isinstance(params.component, any):
-            component_ids = sqlvalues(*params.component.query_values)
+            components = params.component.query_values
         else:
-            component_ids = sqlvalues(params.component)
-
-        distro_archive_ids = [
-            archive.id
-            for archive in distroseries.distribution.all_distro_archives]
-        with_clauses.append("""spns as (
-            SELECT spr.sourcepackagename
-            FROM SourcePackagePublishingHistory
-            JOIN SourcePackageRelease AS spr ON spr.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))
+            components = [params.component]
+
+        # It's much faster to query for a single archive, so don't
+        # include partner unless we have to.
+        archive_ids = set(
+            distroseries.distribution.getArchiveByComponent(c.name).id
+            for c in components)
+
         extra_clauses.append(
             cols['BugTask.sourcepackagenameID'].is_in(
-                SQL('SELECT sourcepackagename FROM spns')))
+                Select(
+                    SourcePackagePublishingHistory.sourcepackagenameID,
+                    tables=[SourcePackagePublishingHistory],
+                    where=And(
+                        SourcePackagePublishingHistory.archiveID.is_in(
+                            archive_ids),
+                        SourcePackagePublishingHistory.distroseries ==
+                            distroseries,
+                        SourcePackagePublishingHistory.componentID.is_in(
+                            c.id for c in components),
+                        SourcePackagePublishingHistory.status ==
+                            PackagePublishingStatus.PUBLISHED))))
 
     upstream_clause = _build_upstream_clause(params, cols)
     if upstream_clause:

=== modified file 'lib/lp/bugs/tests/test_bugtask_search.py'
--- lib/lp/bugs/tests/test_bugtask_search.py	2012-04-26 04:53:53 +0000
+++ lib/lp/bugs/tests/test_bugtask_search.py	2012-04-27 06:44:19 +0000
@@ -39,6 +39,9 @@
     any,
     greater_than,
     )
+from lp.soyuz.interfaces.archive import ArchivePurpose
+from lp.soyuz.interfaces.component import IComponentSet
+from lp.soyuz.interfaces.publishing import PackagePublishingStatus
 from lp.testing import (
     person_logged_in,
     TestCase,
@@ -719,6 +722,48 @@
         self.assertSearchFinds(params, self.bugtasks)
 
 
+class DistributionAndDistroSeriesTests:
+    """Tests which are useful for distributions and their series."""
+
+    def makeBugInComponent(self, archive, series, component):
+        pub = self.factory.makeSourcePackagePublishingHistory(
+            archive=archive, distroseries=series, component=component,
+            status=PackagePublishingStatus.PUBLISHED)
+        return self.factory.makeBugTask(
+            target=self.searchtarget.getSourcePackage(pub.sourcepackagename))
+
+    def test_search_by_component(self):
+        series = self.getCurrentSeries()
+        distro = series.distribution
+        self.factory.makeArchive(
+            distribution=distro, purpose=ArchivePurpose.PARTNER)
+
+        main = getUtility(IComponentSet)['main']
+        main_task = self.makeBugInComponent(
+            distro.main_archive, series, main)
+        universe = getUtility(IComponentSet)['universe']
+        universe_task = self.makeBugInComponent(
+            distro.main_archive, series, universe)
+        partner = getUtility(IComponentSet)['partner']
+        partner_task = self.makeBugInComponent(
+            distro.getArchiveByComponent('partner'), series, partner)
+
+        # Searches for a single component work.
+        params = self.getBugTaskSearchParams(user=None, component=main)
+        self.assertSearchFinds(params, [main_task])
+        params = self.getBugTaskSearchParams(user=None, component=universe)
+        self.assertSearchFinds(params, [universe_task])
+
+        # Non-primary-archive component searches also work.
+        params = self.getBugTaskSearchParams(user=None, component=partner)
+        self.assertSearchFinds(params, [partner_task])
+
+        # A combination of archives works.
+        params = self.getBugTaskSearchParams(
+            user=None, component=any(partner, main))
+        self.assertSearchFinds(params, [main_task, partner_task])
+
+
 class BugTargetTestBase:
     """A base class for the bug target mixin classes.
 
@@ -1062,7 +1107,8 @@
 
 class DistributionTarget(BugTargetTestBase, ProductAndDistributionTests,
                          BugTargetWithBugSuperVisor,
-                         ProjectGroupAndDistributionTests):
+                         ProjectGroupAndDistributionTests,
+                         DistributionAndDistroSeriesTests):
     """Use a distribution as the bug target."""
 
     def setUp(self):
@@ -1087,6 +1133,11 @@
         """See `ProductAndDistributionTests`."""
         return self.factory.makeDistroSeries(distribution=self.searchtarget)
 
+    def getCurrentSeries(self):
+        if self.searchtarget.currentseries is None:
+            self.makeSeries()
+        return self.searchtarget.currentseries
+
     def setUpStructuralSubscriptions(self):
         # See `ProjectGroupAndDistributionTests`.
         subscriber = self.factory.makePerson()
@@ -1110,7 +1161,8 @@
         return self.bugtasks[1:] + self.bugtasks[:1]
 
 
-class DistroseriesTarget(BugTargetTestBase, ProjectGroupAndDistributionTests):
+class DistroseriesTarget(BugTargetTestBase, ProjectGroupAndDistributionTests,
+                         DistributionAndDistroSeriesTests):
     """Use a distro series as the bug target."""
 
     def setUp(self):
@@ -1132,6 +1184,9 @@
     def setBugParamsTarget(self, params, target):
         params.setDistroSeries(target)
 
+    def getCurrentSeries(self):
+        return self.searchtarget
+
     def setUpMilestoneSorting(self):
         with person_logged_in(self.owner):
             milestone_1 = self.factory.makeMilestone(