Merge lp:~edwin-grubbs/launchpad/bug-535430-needspackaging-timeout-part3 into lp:launchpad/db-devel

[Edwin Grubbs]

Summary
-------

This branch extends the work from
bug-535430-needspackaging-timeout-part2, which was landed in db-devel
revision 9449 but got reverted in revision 9451 due to the NOT NULL
constraint on the DistributionSourcePackage.section column that was
added. After looking into it more, it became apparent that caching the
section in the DSP table wasn't really necessary, since it was only used
to guess if a package is a metapackage by checking if it was in the
'misc' section. A metapackage does not need a link to its upstream
project, so it can be safely ignored by the
$distroseries/+needs-packaging page, which is what we are optimizing in
the first place.

This branch changes the db patch to add the is_upstream_link_allowed
boolean to the DistributionSourcePackage table instead of the section
foreign key.

Implementation details
----------------------

DistributionSourcePackage.is_upstream_link_allowed:
    database/schema/comments.sql
    database/schema/patch-2207-56-0.sql
    lib/lp/registry/model/distributionsourcepackage.py

Modified the getPrioritizedUnlinkedSourcePackages() method that the
+needs-packaging page uses:
    lib/lp/registry/model/distroseries.py
    lib/lp/registry/templates/distroseries-needs-packaging.pt
    lib/lp/registry/doc/distroseries.txt

Tests
-----

./bin/test -vv -t 'xx-show-distroseries-packaging.txt|xx-sourcepackage-packaging.txt|doc/distroseries.txt|test_distroseries|bugheat|bug-heat'

Demo and Q/A
------------

* Open http://launchpad.dev/ubuntu/hoary/+needs-packaging
  * On staging, this should no longer timeout.

[Brad Crittenden (bac)]

Hi Edwin,

We already discussed some of these on IRC:

* "Whether an upstream link should be added if it does not already exist." s/should/may

* For clarity I'd change
   is_upstream_link_allowed = spph.section.name == 'misc' to
   is_upstream_link_allowed = (spph.section.name == 'misc')

I also note that code appears twice in the module. Perhaps you can refactor it.

Otherwise the branch looks good. Thanks.

review: Approve (code)

[Stuart Bishop (stub)]

The first INSERT needs an ORDER BY clause, or we could end up with inconsistent data between the master and slave databases:

INSERT INTO DistributionSourcePackage (distribution,sourcepackagename)
SELECT ds.distribution, sourcepackagename
FROM SourcePackagePublishingHistory spph
JOIN Archive ON spph.archive = Archive.id
JOIN SourcePackageRelease spr ON spph.sourcepackagerelease = spr.id
JOIN DistroSeries ds ON spph.distroseries = ds.id
WHERE ds.releasestatus = 4 -- CURRENT
    AND Archive.purpose = 1 -- PRIMARY
EXCEPT
SELECT distribution, sourcepackagename
FROM DistributionSourcePackage
ORDER BY distribution, sourcepackagename;

We also need to repack the table when we are done, so add at the end:

CLUSTER DistributionSourcePackage
    USING distributionpackage__sourcepackagename__distribution__key;

review: Approve (db)

=== modified file 'database/schema/comments.sql'
--- database/schema/comments.sql	2010-06-25 17:47:31 +0000
+++ database/schema/comments.sql	2010-06-25 17:47:32 +0000
@@ -476,7 +476,7 @@
 COMMENT ON COLUMN DistributionSourcePackage.total_bug_heat IS 'Sum of bug heat matching the package distribution and sourcepackagename. NULL means it has not yet been calculated.';
 COMMENT ON COLUMN DistributionSourcePackage.bug_count IS 'Number of bugs matching the package distribution and sourcepackagename. NULL means it has not yet been calculated.';
 COMMENT ON COLUMN DistributionSourcePackage.po_message_count IS 'Number of translations matching the package distribution and sourcepackagename. NULL means it has not yet been calculated.';
-COMMENT ON COLUMN DistributionSourcePackage.section IS 'Cached section matching the latest SourcePackagePublishingHistory record by distribution and sourcepackagename whose archive purpose is PRIMARY and whose distroseries releasestatus is CURRENT.';
+COMMENT ON COLUMN DistributionSourcePackage.is_upstream_link_allowed IS 'Whether an upstream link may be added if it does not already exist.';
 COMMENT ON COLUMN DistributionSourcePackage.bug_reported_acknowledgement IS 'A message of acknowledgement to display to a bug reporter after they\'ve reported a new bug.';
 
 -- DistributionSourcePackageCache

=== modified file 'database/schema/patch-2207-56-0.sql'
--- database/schema/patch-2207-56-0.sql	2010-06-25 17:47:31 +0000
+++ database/schema/patch-2207-56-0.sql	2010-06-25 17:47:32 +0000
@@ -21,4 +21,55 @@
 ALTER TABLE DistributionSourcePackage ADD COLUMN bug_count INTEGER;
 ALTER TABLE DistributionSourcePackage ADD COLUMN po_message_count INTEGER;
 ALTER TABLE DistributionSourcePackage
-    ADD COLUMN section INTEGER NOT NULL REFERENCES section(id);
+    ADD COLUMN is_upstream_link_allowed BOOLEAN NOT NULL DEFAULT TRUE;
+
+/* Add DistributionSourcePackage row for each
+ * SourcePackagePublishingHistory entry whose archive is primary and
+ * whose distroseries is current.
+ */
+INSERT INTO DistributionSourcePackage (
+    distribution,
+    sourcepackagename
+    )
+    SELECT
+        ds.distribution,
+        sourcepackagename
+    FROM SourcePackagePublishingHistory spph
+        JOIN Archive ON spph.archive = Archive.id
+        JOIN SourcePackageRelease spr ON spph.sourcepackagerelease = spr.id
+        JOIN DistroSeries ds ON spph.distroseries = ds.id
+    WHERE ds.releasestatus = 4 -- CURRENT
+        AND Archive.purpose = 1 -- PRIMARY
+    EXCEPT
+    SELECT
+        distribution,
+        sourcepackagename
+    FROM DistributionSourcePackage
+ORDER BY distribution, sourcepackagename;
+
+
+
+/* Update cached bug values in DistributionSourcePackage. */
+UPDATE DistributionSourcePackage
+SET max_bug_heat = subquery.max_bug_heat,
+    total_bug_heat = subquery.total_bug_heat,
+    bug_count = subquery.bug_count
+FROM (
+    SELECT
+        COALESCE(MAX(Bug.heat), 0) as max_bug_heat,
+        COALESCE(SUM(Bug.heat), 0) as total_bug_heat,
+        COALESCE(COUNT(Bug.id), 0) as bug_count,
+        distribution as distro,
+        sourcepackagename as spn
+    FROM
+        DistributionSourcePackage
+        LEFT JOIN BugTask USING(distribution, sourcepackagename)
+        LEFT JOIN Bug ON BugTask.bug = Bug.id
+    GROUP BY distribution, sourcepackagename
+    ) AS subquery
+WHERE distribution = distro
+    AND sourcepackagename = spn;
+
+
+CLUSTER DistributionSourcePackage
+   USING distributionpackage__sourcepackagename__distribution__key;

=== modified file 'lib/lp/registry/doc/distroseries.txt'
--- lib/lp/registry/doc/distroseries.txt	2010-05-18 13:55:07 +0000
+++ lib/lp/registry/doc/distroseries.txt	2010-06-25 17:47:32 +0000
@@ -449,7 +449,7 @@
 
     >>> for summary in hoary.getPrioritizedUnlinkedSourcePackages():
     ...     print  summary['package'].name
-    ...     print '%(total_bugs)s %(total_messages)s' % summary
+    ...     print '%(bug_count)s %(total_messages)s' % summary
     pmount  0  64
     alsa-utils  0  0
     cnews  0  0

=== modified file 'lib/lp/registry/model/distributionsourcepackage.py'
--- lib/lp/registry/model/distributionsourcepackage.py	2010-06-25 17:47:31 +0000
+++ lib/lp/registry/model/distributionsourcepackage.py	2010-06-25 17:47:32 +0000
@@ -17,7 +17,7 @@
 from sqlobject.sqlbuilder import SQLConstant
 from storm.expr import And, Count, Desc, In, Join, Lower, Max, Sum
 from storm.store import EmptyResultSet
-from storm.locals import Int, Reference, Store, Storm, Unicode
+from storm.locals import Bool, Int, Reference, Store, Storm, Unicode
 from zope.component import getUtility
 from zope.error.interfaces import IErrorReportingUtility
 from zope.interface import implements
@@ -25,7 +25,6 @@
 
 from canonical.database.sqlbase import sqlvalues
 from canonical.launchpad.database.emailaddress import EmailAddress
-from lp.registry.interfaces.series import SeriesStatus
 from lp.registry.model.distroseries import DistroSeries
 from lp.registry.model.packaging import Packaging
 from lp.registry.model.structuralsubscription import (
@@ -47,7 +46,6 @@
     SourcePackage, SourcePackageQuestionTargetMixin)
 from lp.soyuz.interfaces.archive import ArchivePurpose
 from lp.soyuz.interfaces.publishing import PackagePublishingStatus
-from lp.soyuz.interfaces.section import ISectionSet
 from lp.soyuz.model.archive import Archive
 from lp.soyuz.model.distributionsourcepackagerelease import (
     DistributionSourcePackageRelease)
@@ -58,6 +56,15 @@
 from lp.translations.model.customlanguagecode import (
     CustomLanguageCode, HasCustomLanguageCodesMixin)
 
+def is_upstream_link_allowed(spph):
+    """Metapackages shouldn't have upstream links.
+
+    Metapackages normally are in the 'misc' section.
+    """
+    if spph is None:
+        return True
+    return spph.section.name == 'misc'
+
 
 class DistributionSourcePackageProperty:
     def __init__(self, attrname):
@@ -85,11 +92,8 @@
                 SourcePackageRelease.sourcepackagenameID ==
                     obj.sourcepackagename.id
                 ).order_by(Desc(SourcePackagePublishingHistory.id)).first()
-            if spph is None:
-                section = getUtility(ISectionSet)['misc']
-            else:
-                section = spph.section
-            obj._new(obj.distribution, obj.sourcepackagename, section)
+            obj._new(obj.distribution, obj.sourcepackagename,
+                     is_upstream_link_allowed(spph))
         setattr(obj._self_in_database, self.attrname, value)
 
 
@@ -119,7 +123,8 @@
     total_bug_heat = DistributionSourcePackageProperty('total_bug_heat')
     bug_count = DistributionSourcePackageProperty('bug_count')
     po_message_count = DistributionSourcePackageProperty('po_message_count')
-    section = DistributionSourcePackageProperty('section')
+    is_upstream_link_allowed = DistributionSourcePackageProperty(
+        'is_upstream_link_allowed')
 
     def __init__(self, distribution, sourcepackagename):
         self.distribution = distribution
@@ -171,30 +176,19 @@
 
     def recalculateBugHeatCache(self):
         """See `IHasBugHeat`."""
-        self.max_bug_heat = IStore(Bug).find(
-            Max(Bug.heat),
-            BugTask.bug == Bug.id,
-            BugTask.distributionID == self.distribution.id,
-            BugTask.sourcepackagenameID == self.sourcepackagename.id).one()
-        if self.max_bug_heat is None:
-            # SELECT MAX(Bug.heat) returns NULL if zero rows match.
-            self.max_bug_heat = 0
-
-        self.total_bug_heat = IStore(Bug).find(
-            Sum(Bug.heat),
-            BugTask.bug == Bug.id,
-            BugTask.distributionID == self.distribution.id,
-            BugTask.sourcepackagenameID == self.sourcepackagename.id).one()
-        if self.total_bug_heat is None:
-            self.total_bug_heat = 0
-
-        self.bug_count = IStore(Bug).find(
-            Count(Bug.heat),
-            BugTask.bug == Bug.id,
-            BugTask.distributionID == self.distribution.id,
-            BugTask.sourcepackagenameID == self.sourcepackagename.id).one()
-        if self.bug_count is None:
-            self.bug_count = 0
+        row = IStore(Bug).find(
+            (Max(Bug.heat), Sum(Bug.heat), Count(Bug.id)),
+            BugTask.bug == Bug.id,
+            BugTask.distributionID == self.distribution.id,
+            BugTask.sourcepackagenameID == self.sourcepackagename.id).one()
+
+        # Aggregate functions return NULL if zero rows match.
+        row = list(row)
+        for i in range(len(row)):
+            if row[i] is None:
+                row[i] = 0
+
+        self.max_bug_heat, self.total_bug_heat, self.bug_count = row
 
     @property
     def latest_overall_publication(self):
@@ -523,11 +517,12 @@
             ).one()
 
     @classmethod
-    def _new(cls, distribution, sourcepackagename, section):
+    def _new(cls, distribution, sourcepackagename,
+             is_upstream_link_allowed=False):
         dsp = DistributionSourcePackageInDatabase()
         dsp.distribution = distribution
         dsp.sourcepackagename = sourcepackagename
-        dsp.section = section
+        dsp.is_upstream_link_allowed = is_upstream_link_allowed
         Store.of(distribution).add(dsp)
         return dsp
 
@@ -536,22 +531,15 @@
         """Create DistributionSourcePackage record, if necessary.
 
         Only create a record for primary archives (i.e. not for PPAs).
-        If it already exists, update the section attribute.
         """
-        # Import here to avoid import loop.
         sourcepackagename = spph.sourcepackagerelease.sourcepackagename
         distribution = spph.distroseries.distribution
-        section = spph.section
 
         if spph.archive.purpose == ArchivePurpose.PRIMARY:
             dsp = cls._get(distribution, sourcepackagename)
             if dsp is None:
-                dsp = cls._new(distribution, sourcepackagename, section)
-            elif spph.distroseries.status == SeriesStatus.CURRENT:
-                # If the distroseries.status is not CURRENT, it should
-                # not override the section from a previously created
-                # SourcePackagePublishingHistory.
-                dsp.section = section
+                cls._new(distribution, sourcepackagename,
+                         is_upstream_link_allowed(spph))
 
 
 class DistributionSourcePackageInDatabase(Storm):
@@ -581,6 +569,4 @@
     total_bug_heat = Int()
     bug_count = Int()
     po_message_count = Int()
-
-    section_id = Int(name='section')
-    section = Reference(section_id, 'Section.id')
+    is_upstream_link_allowed = Bool()

=== modified file 'lib/lp/registry/model/distroseries.py'
--- lib/lp/registry/model/distroseries.py	2010-06-15 09:10:01 +0000
+++ lib/lp/registry/model/distroseries.py	2010-06-25 17:47:32 +0000
@@ -48,7 +48,6 @@
 from lp.bugs.model.bugtarget import BugTargetBase, HasBugHeatMixin
 from lp.bugs.model.bugtask import BugTask
 from lp.bugs.interfaces.bugtarget import IHasBugHeat
-from lp.bugs.interfaces.bugtask import UNRESOLVED_BUGTASK_STATUSES
 from lp.soyuz.model.component import Component
 from lp.soyuz.model.distroarchseries import (
     DistroArchSeries, DistroArchSeriesSet, PocketChroot)
@@ -326,9 +325,9 @@
         find_spec = (
             SourcePackageName,
             SQL("""
-                coalesce(total_heat, 0) + coalesce(po_messages, 0) +
+                coalesce(total_bug_heat, 0) + coalesce(po_messages, 0) +
                 CASE WHEN spr.component = 1 THEN 1000 ELSE 0 END AS score"""),
-            SQL("coalesce(total_bugs, 0) AS total_bugs"),
+            SQL("coalesce(bug_count, 0) AS bug_count"),
             SQL("coalesce(total_messages, 0) AS total_messages"))
         joins, conditions = self._current_sourcepackage_joins_and_conditions
         origin = SQL(joins)
@@ -338,9 +337,9 @@
         return [{
                  'package': SourcePackage(
                     sourcepackagename=spn, distroseries=self),
-                 'total_bugs': total_bugs,
+                 'bug_count': bug_count,
                  'total_messages': total_messages}
-                for (spn, score, total_bugs, total_messages) in results]
+                for (spn, score, bug_count, total_messages) in results]
 
     def getPrioritizedlPackagings(self):
         """See `IDistroSeries`.
@@ -359,7 +358,7 @@
             SQL("""
                 CASE WHEN spr.component = 1 THEN 1000 ELSE 0 END +
                     CASE WHEN Product.bugtracker IS NULL
-                        THEN coalesce(total_heat, 10) ELSE 0 END +
+                        THEN coalesce(total_bug_heat, 10) ELSE 0 END +
                     CASE WHEN ProductSeries.translations_autoimport_mode = 1
                         THEN coalesce(po_messages, 10) ELSE 0 END +
                     CASE WHEN ProductSeries.branch IS NULL THEN 500
@@ -383,29 +382,7 @@
         # Bugs and PO messages are heuristically scored. These queries
         # can easily timeout so filters and weights are used to create
         # an acceptable prioritization of packages that is fast to excecute.
-        bug_heat_filter = 200
         po_message_weight = .5
-        heat_score = ("""
-            LEFT JOIN (
-                SELECT
-                    BugTask.sourcepackagename,
-                    sum(Bug.heat) AS total_heat,
-                    count(Bug.id) AS total_bugs
-                FROM BugTask
-                    JOIN Bug
-                        ON bugtask.bug = Bug.id
-                WHERE
-                    BugTask.sourcepackagename is not NULL
-                    AND BugTask.distribution = %(distribution)s
-                    AND BugTask.status in %(statuses)s
-                    AND Bug.heat > %(bug_heat_filter)s
-                GROUP BY BugTask.sourcepackagename
-                ) bugs
-                ON SourcePackageName.id = bugs.sourcepackagename
-            """ % sqlvalues(
-                distribution=self.distribution,
-                statuses=UNRESOLVED_BUGTASK_STATUSES,
-                bug_heat_filter=bug_heat_filter))
         message_score = ("""
             LEFT JOIN (
                 SELECT
@@ -442,7 +419,10 @@
             LEFT JOIN Packaging
                 ON SourcePackageName.id = Packaging.sourcepackagename
                 AND Packaging.distroseries = DistroSeries.id
-            """ + heat_score + message_score)
+            LEFT JOIN DistributionSourcePackage dsp
+                ON dsp.sourcepackagename = spr.sourcepackagename
+                    AND dsp.distribution = DistroSeries.distribution
+            """ + message_score)
         conditions = ("""
             DistroSeries.id = %(distroseries)s
             AND spph.status IN %(active_status)s

=== modified file 'lib/lp/registry/templates/distroseries-needs-packaging.pt'
--- lib/lp/registry/templates/distroseries-needs-packaging.pt	2010-01-30 18:15:36 +0000
+++ lib/lp/registry/templates/distroseries-needs-packaging.pt	2010-06-25 17:47:32 +0000
@@ -53,7 +53,7 @@
                   tal:attributes="href summary/package/fmt:url"
                   tal:content="summary/package/name">evolution</a>
               </td>
-              <td tal:define="count summary/total_bugs;
+              <td tal:define="count summary/bug_count;
                               singular string:bug;
                               plural string:bugs;">
                 <tal:no-bugs condition="not: count">