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
1=== modified file 'lib/lp/bugs/model/bugtask.py'
2--- lib/lp/bugs/model/bugtask.py 2011-01-14 00:35:43 +0000
3+++ lib/lp/bugs/model/bugtask.py 2011-01-14 12:10:43 +0000
4@@ -2632,6 +2632,60 @@
5 AND RelatedBugTask.status IN %s)
6 """ % sqlvalues(unexpirable_status_list)
7
8+ TARGET_SELECT = {
9+ IDistribution: """
10+ SELECT Distribution.id, NULL, NULL, NULL,
11+ Distribution.id, NULL
12+ FROM Distribution
13+ WHERE Distribution.enable_bug_expiration IS TRUE""",
14+ IDistroSeries: """
15+ SELECT NULL, DistroSeries.id, NULL, NULL,
16+ Distribution.id, NULL
17+ FROM DistroSeries
18+ JOIN Distribution
19+ ON DistroSeries.distribution = Distribution.id
20+ WHERE Distribution.enable_bug_expiration IS TRUE""",
21+ IProduct: """
22+ SELECT NULL, NULL, Product.id, NULL,
23+ NULL, Product.id
24+ FROM Product
25+ WHERE Product.enable_bug_expiration IS TRUE""",
26+ IProductSeries: """
27+ SELECT NULL, NULL, NULL, ProductSeries.id,
28+ NULL, Product.id
29+ FROM ProductSeries
30+ JOIN Product
31+ ON ProductSeries.Product = Product.id
32+ WHERE Product.enable_bug_expiration IS TRUE""",
33+ }
34+
35+ TARGET_JOIN_CLAUSE = {
36+ IDistribution: "BugTask.distribution = target.distribution",
37+ IDistroSeries: "BugTask.distroseries = target.distroseries",
38+ IProduct: "BugTask.product = target.product",
39+ IProductSeries: "BugTask.productseries = target.productseries",
40+ }
41+
42+ def _getJoinForTargets(self, *targets):
43+ """Build the UNION of the sub-query for the given set of targets."""
44+ selects = ' UNION '.join(
45+ self.TARGET_SELECT[target] for target in targets)
46+ join_clause = ' OR '.join(
47+ self.TARGET_JOIN_CLAUSE[target] for target in targets)
48+ # We create this rather bizarre looking structure
49+ # because we must replicate the behaviour of BugTask since
50+ # we are joining to it. So when distroseries is set,
51+ # distribution should be NULL. The two pillar columns will
52+ # be used in the WHERE clause.
53+ return """
54+ JOIN (
55+ SELECT 0 AS distribution, 0 AS distroseries,
56+ 0 AS product , 0 AS productseries,
57+ 0 AS distribution_pillar, 0 AS product_pillar
58+ UNION %s
59+ ) target
60+ ON (%s)""" % (selects, join_clause)
61+
62 def _getTargetJoinAndClause(self, target):
63 """Return a SQL join clause to a `BugTarget`.
64
65@@ -2644,54 +2698,23 @@
66 :raises AssertionError: If the target is not a known implementer of
67 `IBugTarget`
68 """
69- target_join = """
70- JOIN (
71- -- We create this rather bizarre looking structure
72- -- because we must replicate the behaviour of BugTask since
73- -- we are joining to it. So when distroseries is set,
74- -- distribution should be NULL. The two pillar columns will
75- -- be used in the WHERE clause.
76- SELECT 0 AS distribution, 0 AS distroseries,
77- 0 AS product , 0 AS productseries,
78- 0 AS distribution_pillar, 0 AS product_pillar
79- UNION
80- SELECT Distribution.id, NULL, NULL, NULL,
81- Distribution.id, NULL
82- FROM Distribution
83- WHERE Distribution.enable_bug_expiration IS TRUE
84- UNION
85- SELECT NULL, DistroSeries.id, NULL, NULL,
86- Distribution.id, NULL
87- FROM DistroSeries
88- JOIN Distribution
89- ON DistroSeries.distribution = Distribution.id
90- WHERE Distribution.enable_bug_expiration IS TRUE
91- UNION
92- SELECT NULL, NULL, Product.id, NULL,
93- NULL, Product.id
94- FROM Product
95- WHERE Product.enable_bug_expiration IS TRUE
96- UNION
97- SELECT NULL, NULL, NULL, ProductSeries.id,
98- NULL, Product.id
99- FROM ProductSeries
100- JOIN Product
101- ON ProductSeries.Product = Product.id
102- WHERE Product.enable_bug_expiration IS TRUE) target
103- ON (BugTask.distribution = target.distribution
104- OR BugTask.distroseries = target.distroseries
105- OR BugTask.product = target.product
106- OR BugTask.productseries = target.productseries)"""
107 if target is None:
108+ target_join = self._getJoinForTargets(
109+ IDistribution, IDistroSeries, IProduct, IProductSeries)
110 target_clause = "TRUE IS TRUE"
111 elif IDistribution.providedBy(target):
112+ target_join = self._getJoinForTargets(
113+ IDistribution, IDistroSeries)
114 target_clause = "target.distribution_pillar = %s" % sqlvalues(
115 target)
116 elif IDistroSeries.providedBy(target):
117+ target_join = self._getJoinForTargets(IDistroSeries)
118 target_clause = "BugTask.distroseries = %s" % sqlvalues(target)
119 elif IProduct.providedBy(target):
120+ target_join = self._getJoinForTargets(IProduct, IProductSeries)
121 target_clause = "target.product_pillar = %s" % sqlvalues(target)
122 elif IProductSeries.providedBy(target):
123+ target_join = self._getJoinForTargets(IProductSeries)
124 target_clause = "BugTask.productseries = %s" % sqlvalues(target)
125 elif (IProjectGroup.providedBy(target)
126 or ISourcePackage.providedBy(target)
127
128=== modified file 'lib/lp/bugs/model/tests/test_bugtask.py'
129--- lib/lp/bugs/model/tests/test_bugtask.py 2011-01-13 23:04:44 +0000
130+++ lib/lp/bugs/model/tests/test_bugtask.py 2011-01-14 12:10:43 +0000
131@@ -1258,14 +1258,16 @@
132 Four BugTarget types may passed as the target argument:
133 Distribution, DistroSeries, Product, ProductSeries.
134 """
135- supported_targets = [self.distribution, self.distroseries,
136- self.product, self.productseries]
137- for target in supported_targets:
138+ supported_targets_and_task_count = [
139+ (self.distribution, 2), (self.distroseries, 1), (self.product, 2),
140+ (self.productseries, 1), (None, 4)]
141+ for target, expected_count in supported_targets_and_task_count:
142 expirable_bugtasks = self.bugtaskset.findExpirableBugTasks(
143 0, self.user, target=target)
144- self.assertNotEqual(expirable_bugtasks.count(), 0,
145- "%s has %d expirable bugtasks." %
146- (self.distroseries, expirable_bugtasks.count()))
147+ self.assertEqual(expected_count, expirable_bugtasks.count(),
148+ "%s has %d expirable bugtasks, expected %d." %
149+ (self.distroseries, expirable_bugtasks.count(),
150+ expected_count))
151
152 def testUnsupportedBugTargetParam(self):
153 """Test that unsupported targets raise errors.