Launchpad itself

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

  1. bug-607958
  2. Merge into devel
Proposed by Abel Deuring
Status: Merged
Approved by: Abel Deuring
Approved revision: no longer in the source branch.
Merged at revision: 12210
Proposed branch: lp:~adeuring/launchpad/bug-607958
Merge into: lp:launchpad
Diff against target: 153 lines (+69/-44)
2 files modified
lib/lp/bugs/model/bugtask.py (+61/-38)
lib/lp/bugs/model/tests/test_bugtask.py (+8/-6)
To merge this branch: bzr merge lp:~adeuring/launchpad/bug-607958
Reviewer Review Type Date Requested Status
Henning Eggers (community) code Approve
Review via email: mp+45675@code.launchpad.net

Commit message

[r=henninge][ui=none][bug=607958] do not join unneeded target tables in BugTaskSet.findExpirableBugTasks()

Description of the change

This branch fixes bug 607958: timeouts on Distribution:+bugtarget-portlet-bugfilters-stats

The query to find expirable bugtasks is/was quite slow. An EXPLAIN ANALYZE showed that the sub-query to find all possible bug targets having bug expiration enabled retrieved _all_ bugtargets, even if findExpirableBugTasks() is called with a target. This branch modifies this sub-query so that only those bug targets are retrieved that are related to the given target parameter. (Note the difference between a target of a bug task and parameter "target" of findExpirableBugtasks(): If the method parameter is a distribution or a product, we want to retrieve bugtasks for a distroseries or a productseries too).

So I added a method _getJoinForTargets() which builds the JOIN expression for several targets. This method is called by _getTargetJoinAndClause() with the "right" selection of target types for a given parameter "target" of findExpirableBugTasks()

This makes the query twice as a fast as before for findExpriableBugTasks(target=ubuntu). (Note that the timeouts mentioned in the bug report already disappeared when we switched to PostgesQL 8.3)

test: ./bin/test -vvt test_bugtask.BugTaskSetFindExpirableBugTasksTest

no lint

To post a comment you must log in.
Revision history for this message
Henning Eggers (henninge) wrote :

Thanks for this improvement. I see you also fixed the test to actually do something useful. Good job!

Maybe you could move that SQL comment out into a Python comment? I find it rather unnecessary to have it in the SQL.

review: Approve (code)

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
=== modified file 'lib/lp/bugs/model/bugtask.py'
--- lib/lp/bugs/model/bugtask.py 2011-01-14 00:35:43 +0000
+++ lib/lp/bugs/model/bugtask.py 2011-01-14 12:10:43 +0000
@@ -2632,6 +2632,60 @@
2632 AND RelatedBugTask.status IN %s)2632 AND RelatedBugTask.status IN %s)
2633 """ % sqlvalues(unexpirable_status_list)2633 """ % sqlvalues(unexpirable_status_list)
26342634
2635 TARGET_SELECT = {
2636 IDistribution: """
2637 SELECT Distribution.id, NULL, NULL, NULL,
2638 Distribution.id, NULL
2639 FROM Distribution
2640 WHERE Distribution.enable_bug_expiration IS TRUE""",
2641 IDistroSeries: """
2642 SELECT NULL, DistroSeries.id, NULL, NULL,
2643 Distribution.id, NULL
2644 FROM DistroSeries
2645 JOIN Distribution
2646 ON DistroSeries.distribution = Distribution.id
2647 WHERE Distribution.enable_bug_expiration IS TRUE""",
2648 IProduct: """
2649 SELECT NULL, NULL, Product.id, NULL,
2650 NULL, Product.id
2651 FROM Product
2652 WHERE Product.enable_bug_expiration IS TRUE""",
2653 IProductSeries: """
2654 SELECT NULL, NULL, NULL, ProductSeries.id,
2655 NULL, Product.id
2656 FROM ProductSeries
2657 JOIN Product
2658 ON ProductSeries.Product = Product.id
2659 WHERE Product.enable_bug_expiration IS TRUE""",
2660 }
2661
2662 TARGET_JOIN_CLAUSE = {
2663 IDistribution: "BugTask.distribution = target.distribution",
2664 IDistroSeries: "BugTask.distroseries = target.distroseries",
2665 IProduct: "BugTask.product = target.product",
2666 IProductSeries: "BugTask.productseries = target.productseries",
2667 }
2668
2669 def _getJoinForTargets(self, *targets):
2670 """Build the UNION of the sub-query for the given set of targets."""
2671 selects = ' UNION '.join(
2672 self.TARGET_SELECT[target] for target in targets)
2673 join_clause = ' OR '.join(
2674 self.TARGET_JOIN_CLAUSE[target] for target in targets)
2675 # We create this rather bizarre looking structure
2676 # because we must replicate the behaviour of BugTask since
2677 # we are joining to it. So when distroseries is set,
2678 # distribution should be NULL. The two pillar columns will
2679 # be used in the WHERE clause.
2680 return """
2681 JOIN (
2682 SELECT 0 AS distribution, 0 AS distroseries,
2683 0 AS product , 0 AS productseries,
2684 0 AS distribution_pillar, 0 AS product_pillar
2685 UNION %s
2686 ) target
2687 ON (%s)""" % (selects, join_clause)
2688
2635 def _getTargetJoinAndClause(self, target):2689 def _getTargetJoinAndClause(self, target):
2636 """Return a SQL join clause to a `BugTarget`.2690 """Return a SQL join clause to a `BugTarget`.
26372691
@@ -2644,54 +2698,23 @@
2644 :raises AssertionError: If the target is not a known implementer of2698 :raises AssertionError: If the target is not a known implementer of
2645 `IBugTarget`2699 `IBugTarget`
2646 """2700 """
2647 target_join = """
2648 JOIN (
2649 -- We create this rather bizarre looking structure
2650 -- because we must replicate the behaviour of BugTask since
2651 -- we are joining to it. So when distroseries is set,
2652 -- distribution should be NULL. The two pillar columns will
2653 -- be used in the WHERE clause.
2654 SELECT 0 AS distribution, 0 AS distroseries,
2655 0 AS product , 0 AS productseries,
2656 0 AS distribution_pillar, 0 AS product_pillar
2657 UNION
2658 SELECT Distribution.id, NULL, NULL, NULL,
2659 Distribution.id, NULL
2660 FROM Distribution
2661 WHERE Distribution.enable_bug_expiration IS TRUE
2662 UNION
2663 SELECT NULL, DistroSeries.id, NULL, NULL,
2664 Distribution.id, NULL
2665 FROM DistroSeries
2666 JOIN Distribution
2667 ON DistroSeries.distribution = Distribution.id
2668 WHERE Distribution.enable_bug_expiration IS TRUE
2669 UNION
2670 SELECT NULL, NULL, Product.id, NULL,
2671 NULL, Product.id
2672 FROM Product
2673 WHERE Product.enable_bug_expiration IS TRUE
2674 UNION
2675 SELECT NULL, NULL, NULL, ProductSeries.id,
2676 NULL, Product.id
2677 FROM ProductSeries
2678 JOIN Product
2679 ON ProductSeries.Product = Product.id
2680 WHERE Product.enable_bug_expiration IS TRUE) target
2681 ON (BugTask.distribution = target.distribution
2682 OR BugTask.distroseries = target.distroseries
2683 OR BugTask.product = target.product
2684 OR BugTask.productseries = target.productseries)"""
2685 if target is None:2701 if target is None:
2702 target_join = self._getJoinForTargets(
2703 IDistribution, IDistroSeries, IProduct, IProductSeries)
2686 target_clause = "TRUE IS TRUE"2704 target_clause = "TRUE IS TRUE"
2687 elif IDistribution.providedBy(target):2705 elif IDistribution.providedBy(target):
2706 target_join = self._getJoinForTargets(
2707 IDistribution, IDistroSeries)
2688 target_clause = "target.distribution_pillar = %s" % sqlvalues(2708 target_clause = "target.distribution_pillar = %s" % sqlvalues(
2689 target)2709 target)
2690 elif IDistroSeries.providedBy(target):2710 elif IDistroSeries.providedBy(target):
2711 target_join = self._getJoinForTargets(IDistroSeries)
2691 target_clause = "BugTask.distroseries = %s" % sqlvalues(target)2712 target_clause = "BugTask.distroseries = %s" % sqlvalues(target)
2692 elif IProduct.providedBy(target):2713 elif IProduct.providedBy(target):
2714 target_join = self._getJoinForTargets(IProduct, IProductSeries)
2693 target_clause = "target.product_pillar = %s" % sqlvalues(target)2715 target_clause = "target.product_pillar = %s" % sqlvalues(target)
2694 elif IProductSeries.providedBy(target):2716 elif IProductSeries.providedBy(target):
2717 target_join = self._getJoinForTargets(IProductSeries)
2695 target_clause = "BugTask.productseries = %s" % sqlvalues(target)2718 target_clause = "BugTask.productseries = %s" % sqlvalues(target)
2696 elif (IProjectGroup.providedBy(target)2719 elif (IProjectGroup.providedBy(target)
2697 or ISourcePackage.providedBy(target)2720 or ISourcePackage.providedBy(target)
26982721
=== modified file 'lib/lp/bugs/model/tests/test_bugtask.py'
--- lib/lp/bugs/model/tests/test_bugtask.py 2011-01-13 23:04:44 +0000
+++ lib/lp/bugs/model/tests/test_bugtask.py 2011-01-14 12:10:43 +0000
@@ -1258,14 +1258,16 @@
1258 Four BugTarget types may passed as the target argument:1258 Four BugTarget types may passed as the target argument:
1259 Distribution, DistroSeries, Product, ProductSeries.1259 Distribution, DistroSeries, Product, ProductSeries.
1260 """1260 """
1261 supported_targets = [self.distribution, self.distroseries,1261 supported_targets_and_task_count = [
1262 self.product, self.productseries]1262 (self.distribution, 2), (self.distroseries, 1), (self.product, 2),
1263 for target in supported_targets:1263 (self.productseries, 1), (None, 4)]
1264 for target, expected_count in supported_targets_and_task_count:
1264 expirable_bugtasks = self.bugtaskset.findExpirableBugTasks(1265 expirable_bugtasks = self.bugtaskset.findExpirableBugTasks(
1265 0, self.user, target=target)1266 0, self.user, target=target)
1266 self.assertNotEqual(expirable_bugtasks.count(), 0,1267 self.assertEqual(expected_count, expirable_bugtasks.count(),
1267 "%s has %d expirable bugtasks." %1268 "%s has %d expirable bugtasks, expected %d." %
1268 (self.distroseries, expirable_bugtasks.count()))1269 (self.distroseries, expirable_bugtasks.count(),
1270 expected_count))
12691271
1270 def testUnsupportedBugTargetParam(self):1272 def testUnsupportedBugTargetParam(self):
1271 """Test that unsupported targets raise errors.1273 """Test that unsupported targets raise errors.