Merge lp:~adeuring/launchpad/bug-739065 into lp:launchpad

[Abel Deuring]

This branch is a first step to fix bug 739065: SourcePackage:+index timeouts

The OOPS reports from that bug show that two SQL queries which return only one row each are repeated quite often; this branch lets the query in DistributionSourcePackageRelease.sample_binary_packages prejoin DistroSeriesPackageCache records.

Since it can happen that no DistroSeriesPackageCache exists for a given DistroSeriesBinaryPackage, I also changed its constructor so that passing the optional parameter cache=None means "no cache available", instead of "parameter not specified or no cache available".

A test run of the new query in DistributionSourcePackageRelease.sample_binary_packages indicates that it is slightly faster than the old query; avoiding the retrieval DistroSeriesPackageCache records in separate queries would save more than 3 seconds in OOPS 1904E1683.

DistributionSourcePackageRelease.sample_binary_packages was not tested at all, so I added a few general tests.

tests:

./bin/test soyuz -vvt test_distributionsourcepackagerelease
./bin/test soyuz -vvt test_distroseriesbinarypackage

no lint

[Gavin Panella (allenap)]

Looks good. Some trivial comments, that's all.

[1]

+ all_published = DecoratedResultSet(
+ all_published, lambda row: row[1:3])

Just for interest, operator.itemgetter() can return values at multiple
indices:

        all_published = DecoratedResultSet(
            all_published, itemgetter(1, 2))

Or even:

        all_published = DecoratedResultSet(
            all_published, itemgetter(slice(1, 3)))

[2]

+ for publishing, package_cache in all_published:
+ samples.append(
+ DistroSeriesBinaryPackage(
+ publishing.distroarchseries.distroseries,
+ publishing.binarypackagerelease.binarypackagename,

You seem to be selecting BinaryPackageName in all_published; perhaps,
but adjusting the slicing requested of the DecoratedResultSet you can
get it straight back from the query?

[3]

+ with StormStatementRecorder() as recorder:
+ binary_package.cache
+ self.assertEqual(0, len(recorder.statements))

I think you should do here:

        self.assertThat(recorder, HasQueryCount(Equals(0)))

This ensures you get a statement log in the event that the match
fails.

[4]

+ with StormStatementRecorder() as recorder:
+ self.distroseries_binary_package.cache
+ self.assertTrue(len(recorder.statements) > 0)

Likewise, I think you should do:

        self.assertThat(recorder, HasQueryCount(NotEquals(0)))

review: Approve

[Abel Deuring (adeuring)]

Hi Gavin,

thanks for your review!

All sugestions implemented.

On 19.05.2011 15:38, Gavin Panella wrote:
> Review: Approve
> Looks good. Some trivial comments, that's all.
>
>
> [1]
>
> + all_published = DecoratedResultSet(
> + all_published, lambda row: row[1:3])
>
> Just for interest, operator.itemgetter() can return values at multiple
> indices:
>
> all_published = DecoratedResultSet(
> all_published, itemgetter(1, 2))
>
> Or even:
>
> all_published = DecoratedResultSet(
> all_published, itemgetter(slice(1, 3)))
>
>
> [2]
>
> + for publishing, package_cache in all_published:
> + samples.append(
> + DistroSeriesBinaryPackage(
> + publishing.distroarchseries.distroseries,
> + publishing.binarypackagerelease.binarypackagename,
>
> You seem to be selecting BinaryPackageName in all_published; perhaps,
> but adjusting the slicing requested of the DecoratedResultSet you can
> get it straight back from the query?
>
>
> [3]
>
> + with StormStatementRecorder() as recorder:
> + binary_package.cache
> + self.assertEqual(0, len(recorder.statements))
>
> I think you should do here:
>
> self.assertThat(recorder, HasQueryCount(Equals(0)))
>
> This ensures you get a statement log in the event that the match
> fails.
>
>
> [4]
>
> + with StormStatementRecorder() as recorder:
> + self.distroseries_binary_package.cache
> + self.assertTrue(len(recorder.statements) > 0)
>
> Likewise, I think you should do:
>
> self.assertThat(recorder, HasQueryCount(NotEquals(0)))
>

[Robert Collins (lifeless)]

Only testing on production data will tell, but I suspect this will still be a problem: this is a wider query, which means more work for the planner - and more likelyhood the planner will get it wrong.

using load_data (or load_related) in the pre_iter_hook of DecoratedResultSet is probably faster.

No need to change this - I may be wrong - but if in QA it times out on qastaging, I expect query analysis to show this up.

=== modified file 'lib/lp/soyuz/model/distributionsourcepackagerelease.py'
--- lib/lp/soyuz/model/distributionsourcepackagerelease.py	2010-08-20 20:31:18 +0000
+++ lib/lp/soyuz/model/distributionsourcepackagerelease.py	2011-05-19 14:19:45 +0000
@@ -12,11 +12,20 @@
     ]
 
 from lazr.delegates import delegates
-from storm.expr import Desc
+from storm.expr import (
+    And,
+    Desc,
+    Join,
+    LeftJoin,
+    SQL,
+    )
 from zope.component import getUtility
 from zope.interface import implements
 
 from canonical.database.sqlbase import sqlvalues
+from canonical.launchpad.components.decoratedresultset import (
+    DecoratedResultSet,
+    )
 from canonical.launchpad.webapp.interfaces import (
     DEFAULT_FLAVOR,
     IStoreSelector,
@@ -34,6 +43,7 @@
 from lp.soyuz.model.binarypackagename import BinaryPackageName
 from lp.soyuz.model.binarypackagerelease import BinaryPackageRelease
 from lp.soyuz.model.distroseriesbinarypackage import DistroSeriesBinaryPackage
+from lp.soyuz.model.distroseriespackagecache import DistroSeriesPackageCache
 from lp.soyuz.model.publishing import (
     BinaryPackagePublishingHistory,
     SourcePackagePublishingHistory,
@@ -152,33 +162,57 @@
     @property
     def sample_binary_packages(self):
         """See IDistributionSourcePackageRelease."""
-        all_published = BinaryPackagePublishingHistory.select("""
-            BinaryPackagePublishingHistory.distroarchseries =
-                DistroArchSeries.id AND
-            DistroArchSeries.distroseries = DistroSeries.id AND
-            DistroSeries.distribution = %s AND
-            BinaryPackagePublishingHistory.archive IN %s AND
-            BinaryPackagePublishingHistory.binarypackagerelease =
-                BinaryPackageRelease.id AND
-            BinaryPackageRelease.binarypackagename = BinaryPackageName.id AND
-            BinaryPackageRelease.build = BinaryPackageBuild.id AND
-            BinaryPackageBuild.source_package_release = %s
-            """ % sqlvalues(self.distribution,
-                            self.distribution.all_distro_archive_ids,
-                            self.sourcepackagerelease),
-            distinct=True,
-            orderBy=['BinaryPackageName.name'],
-            clauseTables=['DistroArchSeries', 'DistroSeries',
-                          'BinaryPackageRelease', 'BinaryPackageName',
-                          'BinaryPackageBuild'],
-            prejoinClauseTables=['BinaryPackageRelease', 'BinaryPackageName'])
-        samples = []
-        names = set()
-        for publishing in all_published:
-            if publishing.binarypackagerelease.binarypackagename not in names:
-                names.add(publishing.binarypackagerelease.binarypackagename)
-                samples.append(
-                    DistroSeriesBinaryPackage(
-                        publishing.distroarchseries.distroseries,
-                        publishing.binarypackagerelease.binarypackagename))
-        return samples
+        #avoid circular imports.
+        from lp.registry.model.distroseries import DistroSeries
+        from lp.soyuz.model.distroarchseries import DistroArchSeries
+        store = getUtility(IStoreSelector).get(MAIN_STORE, DEFAULT_FLAVOR)
+        archive_ids = list(self.distribution.all_distro_archive_ids)
+        result_row = (
+            SQL('DISTINCT ON(BinaryPackageName.name) 0 AS ignore'),
+            BinaryPackagePublishingHistory, DistroSeriesPackageCache,
+            BinaryPackageRelease, BinaryPackageName)
+        tables = (
+            BinaryPackagePublishingHistory,
+            Join(
+                DistroArchSeries,
+                DistroArchSeries.id ==
+                 BinaryPackagePublishingHistory.distroarchseriesID),
+            Join(
+                DistroSeries,
+                DistroArchSeries.distroseriesID == DistroSeries.id),
+            Join(
+                BinaryPackageRelease,
+                BinaryPackageRelease.id ==
+                BinaryPackagePublishingHistory.binarypackagereleaseID),
+            Join(
+                BinaryPackageName,
+                BinaryPackageName.id ==
+                BinaryPackageRelease.binarypackagenameID),
+            Join(
+                BinaryPackageBuild,
+                BinaryPackageBuild.id == BinaryPackageRelease.buildID),
+            LeftJoin(
+                DistroSeriesPackageCache,
+                And(
+                    DistroSeriesPackageCache.distroseries == DistroSeries.id,
+                    DistroSeriesPackageCache.archiveID.is_in(archive_ids),
+                    DistroSeriesPackageCache.binarypackagename ==
+                    BinaryPackageName.id)))
+
+        all_published = store.using(*tables).find(
+            result_row,
+            DistroSeries.distribution == self.distribution,
+            BinaryPackagePublishingHistory.archiveID.is_in(archive_ids),
+            BinaryPackageBuild.source_package_release ==
+                self.sourcepackagerelease)
+        all_published = all_published.order_by(
+            BinaryPackageName.name)
+
+        def make_dsb_package(row):
+            publishing = row[1]
+            package_cache = row[2]
+            return DistroSeriesBinaryPackage(
+                publishing.distroarchseries.distroseries,
+                publishing.binarypackagerelease.binarypackagename,
+                package_cache)
+        return DecoratedResultSet(all_published, make_dsb_package)

=== modified file 'lib/lp/soyuz/model/distroseriesbinarypackage.py'
--- lib/lp/soyuz/model/distroseriesbinarypackage.py	2010-10-24 12:46:23 +0000
+++ lib/lp/soyuz/model/distroseriesbinarypackage.py	2011-05-19 14:19:45 +0000
@@ -39,10 +39,12 @@
 
     implements(IDistroSeriesBinaryPackage)
 
-    def __init__(self, distroseries, binarypackagename, cache=None):
+    default = object()
+
+    def __init__(self, distroseries, binarypackagename, cache=default):
         self.distroseries = distroseries
         self.binarypackagename = binarypackagename
-        if cache is not None:
+        if cache is not self.default:
             get_property_cache(self).cache = cache
 
     @property
@@ -69,9 +71,9 @@
             self.distroseries.distribution.all_distro_archive_ids)
         result = store.find(
             DistroSeriesPackageCache,
-            DistroSeriesPackageCache.distroseries==self.distroseries,
+            DistroSeriesPackageCache.distroseries == self.distroseries,
             DistroSeriesPackageCache.archiveID.is_in(archive_ids),
-            (DistroSeriesPackageCache.binarypackagename==
+            (DistroSeriesPackageCache.binarypackagename ==
                 self.binarypackagename))
         return result.any()
 

=== added file 'lib/lp/soyuz/tests/test_distributionsourcepackagerelease.py'
--- lib/lp/soyuz/tests/test_distributionsourcepackagerelease.py	1970-01-01 00:00:00 +0000
+++ lib/lp/soyuz/tests/test_distributionsourcepackagerelease.py	2011-05-19 14:19:45 +0000
@@ -0,0 +1,152 @@
+# Copyright 2011 Canonical Ltd.  This software is licensed under the
+# GNU Affero General Public License version 3 (see the file LICENSE).
+
+"""Tests of DistributionSourcePackageRelease."""
+
+from testtools.matchers import LessThan
+from zope.component import getUtility
+
+from canonical.testing.layers import DatabaseFunctionalLayer
+from lp.soyuz.enums import PackagePublishingStatus
+from lp.soyuz.interfaces.distroarchseries import IDistroArchSeriesSet
+from lp.soyuz.model.distributionsourcepackagerelease import (
+    DistributionSourcePackageRelease,
+    )
+from lp.soyuz.tests.test_publishing import SoyuzTestPublisher
+from lp.testing import (
+    StormStatementRecorder,
+    TestCaseWithFactory,
+    )
+from lp.testing.matchers import HasQueryCount
+
+
+class TestDistributionSourcePackageRelease(TestCaseWithFactory):
+    """Tests for DistributionSourcePackageRelease."""
+
+    layer = DatabaseFunctionalLayer
+
+    def setUp(self):
+        super(TestDistributionSourcePackageRelease, self).setUp()
+        self.sourcepackagerelease = self.factory.makeSourcePackageRelease()
+        self.distroarchseries = self.factory.makeDistroArchSeries(
+            distroseries=self.sourcepackagerelease.sourcepackage.distroseries)
+        distribution = self.distroarchseries.distroseries.distribution
+        self.dsp_release = DistributionSourcePackageRelease(
+            distribution, self.sourcepackagerelease)
+
+    def makeBinaryPackageRelease(self, name=None):
+        if name is None:
+            name = self.factory.makeBinaryPackageName()
+        bp_build = self.factory.makeBinaryPackageBuild(
+            source_package_release=self.sourcepackagerelease,
+            distroarchseries=self.distroarchseries)
+        bp_release = self.factory.makeBinaryPackageRelease(
+            build=bp_build, binarypackagename=name)
+        sourcepackagename = self.sourcepackagerelease.sourcepackagename
+        self.factory.makeSourcePackagePublishingHistory(
+            sourcepackagename,
+            sourcepackagerelease=self.sourcepackagerelease,
+            distroseries=self.sourcepackagerelease.sourcepackage.distroseries,
+            status=PackagePublishingStatus.PUBLISHED)
+        self.factory.makeBinaryPackagePublishingHistory(
+            binarypackagerelease=bp_release,
+            distroarchseries=self.distroarchseries)
+
+    def test_sample_binary_packages__no_releases(self):
+        # If no binary releases exist,
+        # DistributionSourcePackageRelease.sample_binary_packages is empty.
+        self.assertEqual(0, self.dsp_release.sample_binary_packages.count())
+
+    def test_sample_binary_packages__one_release(self):
+        # If a binary release exists,
+        # DistributionSourcePackageRelease.sample_binary_packages
+        # returns it.
+        self.makeBinaryPackageRelease(
+            self.factory.makeBinaryPackageName(name='binary-package'))
+        self.assertEqual(
+            ['binary-package'],
+            [release.name
+             for release in self.dsp_release.sample_binary_packages])
+
+    def test_sample_binary_packages__two_releases_one_binary_package(self):
+        # If two binary releases with the same name exist,
+        # DistributionSourcePackageRelease.sample_binary_packages
+        # returns only one.
+        name = self.factory.makeBinaryPackageName(name='binary-package')
+        self.makeBinaryPackageRelease(name)
+        self.makeBinaryPackageRelease(name)
+        self.assertEqual(
+            ['binary-package'],
+            [release.name
+             for release in self.dsp_release.sample_binary_packages])
+
+    def test_sample_binary_packages__two_release_two_binary_packages(self):
+        # If a two binary releases with different names exist,
+        # DistributionSourcePackageRelease.sample_binary_packages
+        # returns both.
+        self.makeBinaryPackageRelease(
+            self.factory.makeBinaryPackageName(name='binary-package'))
+        self.makeBinaryPackageRelease(
+            self.factory.makeBinaryPackageName(name='binary-package-2'))
+        self.assertEqual(
+            ['binary-package', 'binary-package-2'],
+            [release.name
+             for release in self.dsp_release.sample_binary_packages])
+
+    def updateDistroSeriesPackageCache(self):
+        # Create DistroSeriesPackageCache records for new binary
+        # packages.
+        #
+        # SoyuzTestPublisher.updateDistroSeriesPackageCache() creates
+        # a DistroSeriesPackageCache record for the new binary package.
+        # The method closes the current DB connection, making references
+        # to DB objects in other DB objects unusable. Starting with
+        # the distroarchseries, we can create new, valid, instances of
+        # objects required later in the test again.
+        # of the objects we need later.
+        sourcepackagename = self.sourcepackagerelease.sourcepackagename
+        publisher = SoyuzTestPublisher()
+        publisher.updateDistroSeriesPackageCache(
+            self.distroarchseries.distroseries)
+        self.distroarchseries = getUtility(IDistroArchSeriesSet).get(
+            self.distroarchseries.id)
+        distribution = self.distroarchseries.distroseries.distribution
+        releases = distribution.getCurrentSourceReleases([sourcepackagename])
+        [(distribution_sourcepackage, dsp_release)] = releases.items()
+        self.dsp_release = dsp_release
+        self.sourcepackagerelease = dsp_release.sourcepackagerelease
+
+    def test_sample_binary_packages__constant_number_sql_queries(self):
+        # Retrieving
+        # DistributionSourcePackageRelease.sample_binary_packages and
+        # accessing the property "summary" of its items requires a
+        # constant number of SQL queries, regardless of the number
+        # of existing binary package releases.
+        self.makeBinaryPackageRelease()
+        self.updateDistroSeriesPackageCache()
+        with StormStatementRecorder() as recorder:
+            for ds_package in self.dsp_release.sample_binary_packages:
+                ds_package.summary
+        self.assertThat(recorder, HasQueryCount(LessThan(5)))
+        self.assertEqual(1, self.dsp_release.sample_binary_packages.count())
+
+        for iteration in range(5):
+            self.makeBinaryPackageRelease()
+        self.updateDistroSeriesPackageCache()
+        with StormStatementRecorder() as recorder:
+            for ds_package in self.dsp_release.sample_binary_packages:
+                ds_package.summary
+        self.assertThat(recorder, HasQueryCount(LessThan(5)))
+        self.assertEqual(6, self.dsp_release.sample_binary_packages.count())
+
+        # Even if the cache is not updated for binary packages,
+        # DistributionSourcePackageRelease objects do not try to
+        # retrieve DistroSeriesPackageCache records if they know
+        # that such records do not exist.
+        for iteration in range(5):
+            self.makeBinaryPackageRelease()
+        with StormStatementRecorder() as recorder:
+            for ds_package in self.dsp_release.sample_binary_packages:
+                ds_package.summary
+        self.assertThat(recorder, HasQueryCount(LessThan(5)))
+        self.assertEqual(11, self.dsp_release.sample_binary_packages.count())

=== modified file 'lib/lp/soyuz/tests/test_distroseriesbinarypackage.py'
--- lib/lp/soyuz/tests/test_distroseriesbinarypackage.py	2010-12-20 03:21:03 +0000
+++ lib/lp/soyuz/tests/test_distroseriesbinarypackage.py	2011-05-19 14:19:45 +0000
@@ -9,6 +9,10 @@
     'test_suite',
     ]
 
+from testtools.matchers import (
+    Equals,
+    NotEquals,
+    )
 import transaction
 
 from canonical.config import config
@@ -16,7 +20,11 @@
 from lp.services.log.logger import BufferLogger
 from lp.soyuz.model.distroseriesbinarypackage import DistroSeriesBinaryPackage
 from lp.soyuz.tests.test_publishing import SoyuzTestPublisher
-from lp.testing import TestCaseWithFactory
+from lp.testing import (
+    StormStatementRecorder,
+    TestCaseWithFactory,
+    )
+from lp.testing.matchers import HasQueryCount
 
 
 class TestDistroSeriesBinaryPackage(TestCaseWithFactory):
@@ -61,3 +69,22 @@
 
         self.failUnlessEqual(
             'Foo is the best', self.distroseries_binary_package.summary)
+
+    def test_none_cache_passed_at_init_counts_as_cached(self):
+        # If the value None is passed as the constructor parameter
+        # "cache", it is considered as a valid value.
+        # Accesing the property DistroSeriesBinaryPackage.cache
+        # later does not lead to the execution of an SQL query to
+        # retrieve a DistroSeriesPackageCache record.
+        binary_package = DistroSeriesBinaryPackage(
+            self.distroseries, self.binary_package_name, cache=None)
+        with StormStatementRecorder() as recorder:
+            binary_package.cache
+        self.assertThat(recorder, HasQueryCount(Equals(0)))
+
+        # If the parameter "cache" was not passed, accessing
+        # DistroSeriesBinaryPackage.cache for the first time requires
+        # at least one SQL query.
+        with StormStatementRecorder() as recorder:
+            self.distroseries_binary_package.cache
+        self.assertThat(recorder, HasQueryCount(NotEquals(0)))