launchpad-reviewers team mailing list archive

[Abel Deuring <abel.deuring@xxxxxxxxxxxxx>]

Abel Deuring has proposed merging lp:~adeuring/launchpad/bug-739052-11 into lp:launchpad.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

For more details, see:
https://code.launchpad.net/~adeuring/launchpad/bug-739052-11/+merge/76772

Doing QA for lp:~adeuring/launchpad/bug-739052-10 , it turned out that StormRangeFactory is not a silver bullet to fix each and every SQL timeout: While StormRangeFactory avoids the expensive COUNT(*) query that can be seen ion the OOPS reports for bug 739052, the main query still timed out.

I stared today long enough onto an EXPLAIN ANALYZE report of the query to notice that 5 seconds or more are spent to join an intermediate sub-join with ca 1 million rows with the table buildfarmjob, which also has more than two million rows -- only to sort the result by BuildFarmJob.datecreated.

This huge join can be avoided if we sort the result by BuildFarmJob.id, or PackageBuild.build_farm_job_id.

An EXPLAIN ANALYZE for this query on staging shows an execution time of ca. 2.5 seconds. Should be finally good enough to fix this bug.

Sorting by ID instead of date_created is not strcitly equvalent, but it is "good enough", I think: There should only be minor differences.

Thanks to mthaddon and deryck who ran numerous SQL queries for me.

Tests of STormRangeFactory used for resultsets from this query and tests of the changed method still pass:

./bin/test soyuz -vvt test_build_views
./bin/test soyuz -vvt lp.soyuz.tests.test_build_set

no lint.

PS: Working for a longer time on StormRangeFactory but finally needing a relatively simple modification to fix this bug made me feel a bit like being on both sides of the interview in "The Enigma of Kaspar Hauser": http://www.youtube.com/watch?v=oAnOi0fnxuE
-- 
https://code.launchpad.net/~adeuring/launchpad/bug-739052-11/+merge/76772
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~adeuring/launchpad/bug-739052-11 into lp:launchpad.

=== modified file 'lib/lp/soyuz/model/binarypackagebuild.py'
--- lib/lp/soyuz/model/binarypackagebuild.py	2011-09-16 08:39:28 +0000
+++ lib/lp/soyuz/model/binarypackagebuild.py	2011-09-23 16:13:58 +0000
@@ -1014,7 +1014,9 @@
 
         # Ordering according status
         # * NEEDSBUILD, BUILDING & UPLOADING by -lastscore
-        # * SUPERSEDED & All by -datecreated
+        # * SUPERSEDED & All by -PackageBuild.build_farm_job
+        #   (nearly equivalent to -datecreated, but much more
+        #   efficient.)
         # * FULLYBUILT & FAILURES by -datebuilt
         # It should present the builds in a more natural order.
         if status in [
@@ -1029,9 +1031,8 @@
                 'BuildPackageJob.build = BinaryPackageBuild.id')
             condition_clauses.append('BuildPackageJob.job = BuildQueue.job')
         elif status == BuildStatus.SUPERSEDED or status is None:
-            order_by = [Desc(BuildFarmJob.date_created),
-                        BinaryPackageBuild.id]
-            order_by_table = BuildFarmJob
+            order_by = [Desc(PackageBuild.build_farm_job_id)]
+            order_by_table = PackageBuild
         else:
             order_by = [Desc(BuildFarmJob.date_finished),
                         BinaryPackageBuild.id]