Merge lp:~wgrant/launchpad/bug-728246-copy-expiry-queries into lp:launchpad

[William Grant]

[r=allenap][bug=728246] Bring the package copier's expired file check down to constant queries.

This branch starts to address the package copier's scaling problems, making the expired binary check take a constant number of queries (bug #728246).

BinaryPackageRelease.files is now a cached list. getBuiltBinaries has been Stormified, had its prejoin replaced with a separate BPR preload query, and now takes an option to preload the BPFs and LFAs. I added a unit test to ensure that the query count is constant and that the preloading works.

This eliminates two queries per copied binary publication.

[Gavin Panella (allenap)]

Nice.

review: Approve

=== modified file 'lib/lp/soyuz/model/binarypackagerelease.py'
--- lib/lp/soyuz/model/binarypackagerelease.py	2010-12-24 02:22:11 +0000
+++ lib/lp/soyuz/model/binarypackagerelease.py	2011-03-03 09:01:49 +0000
@@ -22,6 +22,7 @@
     Date,
     Int,
     Reference,
+    Store,
     Storm,
     )
 from zope.interface import implements
@@ -35,6 +36,10 @@
     SQLBase,
     sqlvalues,
     )
+from lp.services.propertycache import (
+    cachedproperty,
+    get_property_cache,
+    )
 from lp.soyuz.enums import (
     BinaryPackageFileType,
     BinaryPackageFormat,
@@ -84,9 +89,6 @@
     debug_package = ForeignKey(dbName='debug_package',
                               foreignKey='BinaryPackageRelease')
 
-    files = SQLMultipleJoin('BinaryPackageFile',
-        joinColumn='binarypackagerelease', orderBy="libraryfile")
-
     _user_defined_fields = StringCol(dbName='user_defined_fields')
 
     def __init__(self, *args, **kwargs):
@@ -136,6 +138,11 @@
             self.binarypackagename)
         return distroarchseries_binary_package.currentrelease is None
 
+    @cachedproperty
+    def files(self):
+        return list(
+            Store.of(self).find(BinaryPackageFile, binarypackagerelease=self))
+
     def addFile(self, file):
         """See `IBinaryPackageRelease`."""
         determined_filetype = None
@@ -151,6 +158,7 @@
             raise AssertionError(
                 'Unsupported file type: %s' % file.filename)
 
+        del get_property_cache(self).files
         return BinaryPackageFile(binarypackagerelease=self,
                                  filetype=determined_filetype,
                                  libraryfile=file)

=== modified file 'lib/lp/soyuz/model/publishing.py'
--- lib/lp/soyuz/model/publishing.py	2011-02-22 22:14:32 +0000
+++ lib/lp/soyuz/model/publishing.py	2011-03-03 09:01:49 +0000
@@ -464,40 +464,51 @@
                 for source, binary_pub, binary, binary_name, arch
                 in result_set]
 
-    def getBuiltBinaries(self):
+    def getBuiltBinaries(self, want_files=False):
         """See `ISourcePackagePublishingHistory`."""
-        clauses = """
-            BinaryPackagePublishingHistory.binarypackagerelease=
-                BinaryPackageRelease.id AND
-            BinaryPackagePublishingHistory.distroarchseries=
-                DistroArchSeries.id AND
-            BinaryPackageRelease.build=BinaryPackageBuild.id AND
-            BinaryPackageBuild.source_package_release=%s AND
-            DistroArchSeries.distroseries=%s AND
-            BinaryPackagePublishingHistory.archive=%s AND
-            BinaryPackagePublishingHistory.pocket=%s
-        """ % sqlvalues(self.sourcepackagerelease, self.distroseries,
-                        self.archive, self.pocket)
-
-        clauseTables = [
-            'BinaryPackageBuild', 'BinaryPackageRelease', 'DistroArchSeries']
-        orderBy = ['-BinaryPackagePublishingHistory.id']
-        preJoins = ['binarypackagerelease']
-
-        results = BinaryPackagePublishingHistory.select(
-            clauses, orderBy=orderBy, clauseTables=clauseTables,
-            prejoins=preJoins)
-        binary_publications = list(results)
-
-        unique_binary_ids = set(
-            [pub.binarypackagerelease.id for pub in binary_publications])
+        from lp.soyuz.model.binarypackagebuild import BinaryPackageBuild
+        from lp.soyuz.model.distroarchseries import DistroArchSeries
+        binary_publications = list(Store.of(self).find(
+            BinaryPackagePublishingHistory,
+            BinaryPackagePublishingHistory.binarypackagereleaseID ==
+                BinaryPackageRelease.id,
+            BinaryPackagePublishingHistory.distroarchseriesID ==
+                DistroArchSeries.id,
+            BinaryPackagePublishingHistory.archiveID == self.archiveID,
+            BinaryPackagePublishingHistory.pocket == self.pocket,
+            BinaryPackageBuild.id == BinaryPackageRelease.buildID,
+            BinaryPackageBuild.source_package_release_id ==
+                self.sourcepackagereleaseID,
+            DistroArchSeries.distroseriesID == self.distroseriesID))
+
+        # Preload attached BinaryPackageReleases.
+        bpr_ids = set(
+            pub.binarypackagereleaseID for pub in binary_publications)
+        list(Store.of(self).find(
+            BinaryPackageRelease, BinaryPackageRelease.id.is_in(bpr_ids)))
+
+        if want_files:
+            # Preload BinaryPackageFiles.
+            bpfs = list(Store.of(self).find(
+                BinaryPackageFile,
+                BinaryPackageFile.binarypackagereleaseID.is_in(bpr_ids)))
+            bpfs_by_bpr = defaultdict(list)
+            for bpf in bpfs:
+                bpfs_by_bpr[bpf.binarypackagerelease].append(bpf)
+            for bpr in bpfs_by_bpr:
+                get_property_cache(bpr).files = bpfs_by_bpr[bpr]
+
+            # Preload LibraryFileAliases.
+            lfa_ids = set(bpf.libraryfileID for bpf in bpfs)
+            list(Store.of(self).find(
+                LibraryFileAlias, LibraryFileAlias.id.is_in(lfa_ids)))
 
         unique_binary_publications = []
         for pub in binary_publications:
-            if pub.binarypackagerelease.id in unique_binary_ids:
+            if pub.binarypackagerelease.id in bpr_ids:
                 unique_binary_publications.append(pub)
-                unique_binary_ids.remove(pub.binarypackagerelease.id)
-                if len(unique_binary_ids) == 0:
+                bpr_ids.remove(pub.binarypackagerelease.id)
+                if len(bpr_ids) == 0:
                     break
 
         return unique_binary_publications

=== modified file 'lib/lp/soyuz/scripts/packagecopier.py'
--- lib/lp/soyuz/scripts/packagecopier.py	2011-01-14 14:03:28 +0000
+++ lib/lp/soyuz/scripts/packagecopier.py	2011-03-03 09:01:49 +0000
@@ -410,7 +410,7 @@
                 raise CannotCopy('source contains expired files')
 
         if self.include_binaries:
-            built_binaries = source.getBuiltBinaries()
+            built_binaries = source.getBuiltBinaries(want_files=True)
             if len(built_binaries) == 0:
                 raise CannotCopy("source has no binaries to be copied")
             # Deny copies of binary publications containing files with

=== modified file 'lib/lp/soyuz/tests/test_publishing.py'
--- lib/lp/soyuz/tests/test_publishing.py	2011-01-15 00:05:55 +0000
+++ lib/lp/soyuz/tests/test_publishing.py	2011-03-03 09:01:49 +0000
@@ -11,6 +11,8 @@
 import tempfile
 
 import pytz
+from storm.store import Store
+from testtools.matchers import Equals
 import transaction
 from zope.component import getUtility
 from zope.security.proxy import removeSecurityProxy
@@ -58,9 +60,10 @@
     SourcePackagePublishingHistory,
     )
 from lp.testing import (
-    login_as,
+    StormStatementRecorder,
     TestCaseWithFactory,
     )
+from lp.testing.matchers import HasQueryCount
 from lp.testing.factory import LaunchpadObjectFactory
 
 
@@ -1362,3 +1365,37 @@
         spph = self.factory.makeSourcePackagePublishingHistory(
             ancestor=ancestor)
         self.assertEquals(spph.ancestor.displayname, ancestor.displayname)
+
+
+class TestGetBuiltBinaries(TestNativePublishingBase):
+    """Test SourcePackagePublishingHistory.getBuiltBinaries() works."""
+
+    def test_flat_query_count(self):
+        spph = self.getPubSource(architecturehintlist='any')
+        store = Store.of(spph)
+        store.flush()
+        store.invalidate()
+
+        # An initial invocation issues one query for the each of the
+        # SPPH, BPPHs and BPRs.
+        with StormStatementRecorder() as recorder:
+            bins = spph.getBuiltBinaries()
+        self.assertEquals(0, len(bins))
+        self.assertThat(recorder, HasQueryCount(Equals(3)))
+
+        self.getPubBinaries(pub_source=spph)
+        store.flush()
+        store.invalidate()
+
+        # A subsequent invocation with files preloaded queries the SPPH,
+        # BPPHs, BPRs, BPFs and LFAs. Checking the filenames of each
+        # BPF has no query penalty.
+        with StormStatementRecorder() as recorder:
+            bins = spph.getBuiltBinaries(want_files=True)
+            self.assertEquals(2, len(bins))
+            for bpph in bins:
+                files = bpph.binarypackagerelease.files
+                self.assertEqual(1, len(files))
+                for bpf in files:
+                    bpf.libraryfile.filename
+        self.assertThat(recorder, HasQueryCount(Equals(5)))