Merge lp:~adeuring/launchpad/bug-518746-faster-retrieval-of-patch-age into lp:launchpad/db-devel

[Abel Deuring]

[Abel Deuring (adeuring)]

This branch fixes timeout OOPSes that consistenly occur on the new +patches view, for nearly all targets. Examples: https://staging.launchpad.net/ubuntu/+patches , https://bugs.staging.launchpad.net/~adeuring/+patches

The OOPS reports show that ca. 15 seconds are spent with ca 30-40 queries which retrieve the bug attachments for a given bug. Each of these queries needs 300..500 milliseconds; they are issued for each of the up to 50 bug tasks we display on the +patches page.

The queries are issued by the method BugsPatchesView.youngestPatch(), which looks for the most recently added patch attachment by iterating over Bug.attachments.

The fix is quite straightforward: We don't need to retrieve all bug attachments of a bug, if we can directly find the one we are interested in using a "better" SQL query.

So I added a property Bug.latest_patch, which does just that: return the most recentl added patch attachment.

As noted in a comment in model/bug.py, the result is in very rare cases not necessarily exact, but I think the inaccuracy does not matter in this case. The alternative would have been to query over the joined tables BugAttachment and Message, and to sort by Message.datecreated, which would be much slower, since Message.datecreated is not indexed.

Chex already EXPLAIN ANALYZEd the new query on staging: It needs ca 40 milliseconds -- not blindingly fast, but, with a total execution time of ca 2 seconds for 50 bugtasks, "good enough" for the +patches view, I think.

tests:
./bin/test -vvv -t patches-view.txt
./bin/test -vvv -t doc/bug.txt

= Launchpad lint =

Checking for conflicts. and issues in doctests and templates.
Running jslint, xmllint, pyflakes, and pylint.
Using normal rules.

Linting changed files:
  lib/lp/bugs/configure.zcml
  lib/lp/bugs/browser/bugtarget.py
  lib/lp/bugs/doc/bug.txt
  lib/lp/bugs/interfaces/bug.py
  lib/lp/bugs/model/bug.py
  lib/lp/bugs/templates/bugtarget-patches.pt

== Pylint notices ==

lib/lp/bugs/browser/bugtarget.py
    333: [C0322, FileBugViewBase.submit_bug_action] Operator not preceded by a space
    failure=handleSubmitBugFailure)
    ^
    def submit_bug_action(self, action, data):
    511: [C0322, FileBugViewBase.this_is_my_bug_action] Operator not preceded by a space
    name="this_is_my_bug", failure=handleSubmitBugFailure)
    ^
    def this_is_my_bug_action(self, action, data):
    883: [C0322, FileBugGuidedView.continue_action] Operator not preceded by a space
    validator="validate_no_dupe_found")
    ^
    def continue_action(self, action, data):

lib/lp/bugs/interfaces/bug.py
    50: [F0401] Unable to import 'lazr.restful.declarations' (No module named restful)
  ...

[Abel Deuring (adeuring)]

sorry copy&paste error with the lint output... The complete lint message:

= Launchpad lint =

Checking for conflicts. and issues in doctests and templates.
Running jslint, xmllint, pyflakes, and pylint.
Using normal rules.

Linting changed files:
  lib/lp/bugs/configure.zcml
  lib/lp/bugs/browser/bugtarget.py
  lib/lp/bugs/doc/bug.txt
  lib/lp/bugs/interfaces/bug.py
  lib/lp/bugs/model/bug.py
  lib/lp/bugs/templates/bugtarget-patches.pt

== Pylint notices ==

lib/lp/bugs/browser/bugtarget.py
    333: [C0322, FileBugViewBase.submit_bug_action] Operator not preceded by a space
    failure=handleSubmitBugFailure)
    ^
    def submit_bug_action(self, action, data):
    511: [C0322, FileBugViewBase.this_is_my_bug_action] Operator not preceded by a space
    name="this_is_my_bug", failure=handleSubmitBugFailure)
    ^
    def this_is_my_bug_action(self, action, data):
    883: [C0322, FileBugGuidedView.continue_action] Operator not preceded by a space
    validator="validate_no_dupe_found")
    ^
    def continue_action(self, action, data):

lib/lp/bugs/interfaces/bug.py
    50: [F0401] Unable to import 'lazr.restful.declarations' (No module named restful)
    56: [F0401] Unable to import 'lazr.restful.fields' (No module named restful)
    57: [F0401] Unable to import 'lazr.restful.interface' (No module named restful)
    501: [C0322, IBug.addAttachment] Operator not preceded by a space
    comment=Text(), filename=TextLine(), is_patch=Bool(),
    ^
    content_type=TextLine(), description=Text())
    @export_factory_operation(IBugAttachment, [])
    def addAttachment(owner, data, comment, filename, is_patch=False,
    content_type=None, description=None):
    636: [C0322, IBug.getNominations] Operator not preceded by a space
    nominations=List(
    ^
    title=_("Nominations to search through."),
    value_type=Reference(schema=Interface), # IBugNomination
    required=False))
    @operation_returns_collection_of(Interface) # IBugNomination
    @export_read_operation()
    def getNominations(target=None, nominations=None):
    716: [C0322, IBug.markUserAffected] Operator not preceded by a space
    required=False, default=True))
    ^
    @call_with(user=REQUEST_USER)
    @export_write_operation()
    def markUserAffected(user, affected=True):
    731: [C0322, IBug.setCommentVisibility] Operator not preceded by a space
    required=True),
    ^
    visible=Bool(title=_('Show this comment?'), required=True))
    @call_with(user=REQUEST_USER)
    @export_write_operation()
    def setCommentVisibility(user, comment_number, visible):
    743: [C0322, IBug.linkHWSubmission] Operator not preceded by a space
    Interface, title=_(...

[Brad Crittenden (bac)]

Hi Abel,

Thanks for the nice work to solve those timeouts. Here are some suggestions to improve the branch.

* I think in bug.txt you are missing one test: when there are multiple patches and the latest patch is deleted. In that case I assume the next oldest is shown, correct? Having that demonstrated in the test would be great. It'll also make the property regression-proof in the event someone decided to cache the latest patch rather than doing a query.

* s/The most recently patch of this bug./The most recent patch of this bug./

* s/datecrated/datecreated/

* In your template I'd discourage the use of the one letter variable name 'p' (though I recognized you inherited it.)

review: Approve (code)

[Abel Deuring (adeuring)]

Hi Brad,

thanks for your review!

On 09.02.2010 19:59, Brad Crittenden wrote:
> Review: Approve code
> Hi Abel,
>
> Thanks for the nice work to solve those timeouts. Here are some suggestions to improve the branch.
>
> * I think in bug.txt you are missing one test: when there are multiple patches and the latest patch is deleted. In that case I assume the next oldest is shown, correct? Having that demonstrated in the test would be great. It'll also make the property regression-proof in the event someone decided to cache the latest patch rather than doing a query.

Good idea, I extended the tests a bit.

>
> * s/The most recently patch of this bug./The most recent patch of this bug./

Fixed.

>
> * s/datecrated/datecreated/

Fixed.

>
> * In your template I'd discourage the use of the one letter variable name 'p' (though I recognized you inherited it.)

Fixed.

Aebl

[Abel Deuring (adeuring)]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Hi Brad,

thanks for your review!

On 09.02.2010 19:59, Brad Crittenden wrote:
> Review: Approve code
> Hi Abel,
>
> Thanks for the nice work to solve those timeouts. Here are some suggestions to improve the branch.
>
> * I think in bug.txt you are missing one test: when there are multiple patches and the latest patch is deleted. In that case I assume the next oldest is shown, correct? Having that demonstrated in the test would be great. It'll also make the property regression-proof in the event someone decided to cache the latest patch rather than doing a query.

Good idea, I extended the tests a bit.

>
> * s/The most recently patch of this bug./The most recent patch of this bug./
>

Fixed.

> * s/datecrated/datecreated/

Fixed.

>
> * In your template I'd discourage the use of the one letter variable name 'p' (though I recognized you inherited it.)

Fixed.

Abel

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.9 (GNU/Linux)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org

iD8DBQFLccKIekBPhm8NrtARAh5cAJoDGSel+wQGQZj4eoWzYdZdpu1hPwCfTloc
kpwtKELVUwXrt9A2onyz6dc=
=iC3D
-----END PGP SIGNATURE-----

=== modified file 'lib/lp/bugs/browser/bugtarget.py'
--- lib/lp/bugs/browser/bugtarget.py	2010-02-04 03:12:35 +0000
+++ lib/lp/bugs/browser/bugtarget.py	2010-02-09 20:16:22 +0000
@@ -1278,23 +1278,6 @@
         else:
             return None
 
-    def youngestPatch(self, bug):
-        """Return the youngest patch attached to a bug, else error."""
-        youngest = None
-        # Loop over bugtasks, gathering youngest patch for each's bug.
-        for attachment in bug.attachments:
-            if attachment.is_patch:
-                if youngest is None:
-                    youngest = attachment
-                elif (attachment.message.datecreated >
-                      youngest.message.datecreated):
-                    youngest = attachment
-        if youngest is None:
-            # This is the patches view, so every bug under
-            # consideration should have at least one patch attachment.
-            raise AssertionError("bug %i has no patch attachments" % bug.id)
-        return youngest
-
     def patchAge(self, patch):
         """Return a timedelta object for the age of a patch attachment."""
         now = datetime.now(timezone('UTC'))

=== modified file 'lib/lp/bugs/configure.zcml'
--- lib/lp/bugs/configure.zcml	2010-02-03 18:57:40 +0000
+++ lib/lp/bugs/configure.zcml	2010-02-09 20:16:22 +0000
@@ -576,6 +576,7 @@
                     personIsSubscribedToDuplicate
                     heat
                     has_patches
+                    latest_patch
                     latest_patch_uploaded"/>
             <require
                 permission="launchpad.View"

=== modified file 'lib/lp/bugs/doc/bug.txt'
--- lib/lp/bugs/doc/bug.txt	2010-02-02 16:48:42 +0000
+++ lib/lp/bugs/doc/bug.txt	2010-02-09 20:16:22 +0000
@@ -1639,16 +1639,18 @@
     True
 
 
-Date when the most recent patch was added
------------------------------------------
-
-If a bug has attachments of type BugAttachmentType.PATCH, the property
-Bug.latest_patch_uploaded is set to the time when the latest patch was
-uploaded.
-
-If a bug has no attachments, latest_patch_uploaded is None.
+Most recently added patch
+-------------------------
+
+Bug.latest_patch provides the most recently added bug attachment of
+type BugAttachmentType.PATCH; the property Bug.latest_patch_uploaded
+is set to the time when the latest patch was uploaded.
+
+If a bug has no attachments, both properties are None.
 
     >>> bug = factory.makeBug()
+    >>> print bug.latest_patch
+    None
     >>> print bug.latest_patch_uploaded
     None
 
@@ -1667,37 +1669,59 @@
 
 If we declare the existing attachment to be a patch,
 latest_patch_uploaded is set to the date_created value of the Message
-record for this attachment.
+record for this attachment, and we can access the attachment via
+Bug.latest_patch.
 
     >>> from lp.bugs.interfaces.bugattachment import BugAttachmentType
     >>> attachment_1.type = BugAttachmentType.PATCH
     >>> transaction.commit()
     >>> date_message_1_created = bug.attachments[0].message.datecreated
+    >>> print bug.latest_patch == attachment_1
+    True
     >>> print bug.latest_patch_uploaded == date_message_1_created
     True
 
 If we add another attachment, this time declared to be a patch
-at creation time, the new value of bug.latest_patch_uploaded will
-change to the value of message.datecreated for this new attachment.
+at creation time, we can access this attachment via Bug.latest_patch,
+and the new value of bug.latest_patch_uploaded will change to the value
+of message.datecreated for this new attachment.
 
     >>> attachment_2 = factory.makeBugAttachment(bug, is_patch=True)
     >>> transaction.commit()
     >>> date_message_2_created = bug.attachments[1].message.datecreated
+    >>> print bug.latest_patch == attachment_2
+    True
     >>> print bug.latest_patch_uploaded == date_message_2_created
     True
 
-If we say that attachment_1 is not a patch, the value of
-bug.latest_patch_uploaded does not change.
+If we say that attachment_1 is not a patch, the values of
+bug.latest_patch and bug.latest_patch_uploaded does not change.
 
     >>> attachment_1.type = BugAttachmentType.UNSPECIFIED
     >>> transaction.commit()
+    >>> print bug.latest_patch == attachment_2
+    True
     >>> print bug.latest_patch_uploaded == date_message_2_created
     True
 
-And if we delete attachment_2, bug.latest_patch_uploaded is again None,
-because the bug has no longer any attachments that are patches.
+If we declare attachment_1 again to be a patch and if we delete
+attachment_2, bug.latest_patch references again attachment_1, and
+bug.bug.latest_patch_uploaded is its creation time.
 
+    >>> attachment_1.type = BugAttachmentType.PATCH
     >>> attachment_2.removeFromBug(user=bug.owner)
     >>> transaction.commit()
+    >>> print bug.latest_patch == attachment_1
+    True
+    >>> print bug.latest_patch_uploaded == date_message_1_created
+    True
+
+If we delete attachment_1 too, bug.latest_patch and
+bug.latest_patch_uploaded are again None.
+
+    >>> attachment_1.removeFromBug(user=bug.owner)
+    >>> transaction.commit()
+    >>> print bug.latest_patch
+    None
     >>> print bug.latest_patch_uploaded
     None

=== modified file 'lib/lp/bugs/interfaces/bug.py'
--- lib/lp/bugs/interfaces/bug.py	2010-02-03 18:57:40 +0000
+++ lib/lp/bugs/interfaces/bug.py	2010-02-09 20:16:22 +0000
@@ -351,6 +351,8 @@
             title=_('Date when the most recent patch was uploaded.'),
             required=False, readonly=True))
 
+    latest_patch = Attribute("The most recent patch of this bug.")
+
     @operation_parameters(
         subject=optional_message_subject_field(),
         content=copy_field(IMessage['content']))

=== modified file 'lib/lp/bugs/model/bug.py'
--- lib/lp/bugs/model/bug.py	2010-02-03 18:57:40 +0000
+++ lib/lp/bugs/model/bug.py	2010-02-09 20:16:22 +0000
@@ -32,7 +32,7 @@
 from sqlobject import SQLMultipleJoin, SQLRelatedJoin
 from sqlobject import SQLObjectNotFound
 from storm.expr import (
-    And, Count, Func, In, LeftJoin, Not, Select, SQLRaw, Union)
+    And, Count, Func, In, LeftJoin, Max, Not, Select, SQLRaw, Union)
 from storm.store import EmptyResultSet, Store
 
 from lazr.lifecycle.event import (
@@ -261,6 +261,31 @@
     latest_patch_uploaded = UtcDateTimeCol(default=None)
 
     @property
+    def latest_patch(self):
+        """See `IBug`."""
+        # We want to retrieve the most recently added bug attachment
+        # that is of type BugAttachmentType.PATCH. In order to find
+        # this attachment, we should in theory sort by
+        # BugAttachment.message.datecreated. Since we don't have
+        # an index for Message.datecreated, such a query would be
+        # quite slow. We search instead for the BugAttachment with
+        # the largest ID for a given bug. This is "nearly" equivalent
+        # to searching the record with the maximum value of
+        # message.datecreated: The only exception is the rare case when
+        # two BugAttachment records are simultaneuosly added to the same
+        # bug, where bug_attachment_1.id < bug_attachment_2.id, while
+        # the Message record for bug_attachment_2 is created before
+        # the Message record for bug_attachment_1. The difference of
+        # the datecreated values of the Message records is in this case
+        # probably smaller than one second and the selection of the
+        # "most recent" patch anyway somewhat arbitrary.
+        return Store.of(self).find(
+            BugAttachment, BugAttachment.id == Select(
+                Max(BugAttachment.id),
+                And(BugAttachment.bug == self.id,
+                    BugAttachment.type == BugAttachmentType.PATCH))).one()
+
+    @property
     def comment_count(self):
         """See `IBug`."""
         return self.message_count - 1

=== modified file 'lib/lp/bugs/templates/bugtarget-patches.pt'
--- lib/lp/bugs/templates/bugtarget-patches.pt	2010-02-03 15:37:11 +0000
+++ lib/lp/bugs/templates/bugtarget-patches.pt	2010-02-09 20:16:22 +0000
@@ -93,20 +93,20 @@
                    <a tal:attributes="href patch_task/target/fmt:url"
                       tal:content="patch_task/target/name"></a></td>
              <td class="patches-view-cell"
-                 tal:define="p python:view.youngestPatch(patch_task.bug);
-                             age python:view.patchAge(p)"
+                 tal:define="patch patch_task/bug/latest_patch;
+                             age python:view.patchAge(patch)"
                  tal:attributes="id string:patch-cell-${repeat/patch_task/index}">
-                   <a tal:attributes="href p/libraryfile/http_url"
+                   <a tal:attributes="href patch/libraryfile/http_url"
                       tal:content="age/fmt:approximateduration/use-digits"></a>
                <div class="popupTitle"
                     tal:attributes="id string:patch-popup-${repeat/patch_task/index};">
-                 <p tal:define="submitter p/message/owner">
+                 <p tal:define="submitter patch/message/owner">
                     <strong>From:</strong>
                     <a tal:replace="structure submitter/fmt:link"></a><br/>
                     <strong>Link:</strong>
-                    <a tal:attributes="href p/libraryfile/http_url"
-                       tal:content="p/libraryfile/filename"></a></p>
-                 <p tal:content="string:${p/title}"></p>
+                    <a tal:attributes="href patch/libraryfile/http_url"
+                       tal:content="patch/libraryfile/filename"></a></p>
+                 <p tal:content="string:${patch/title}"></p>
                </div>
                <script type="text/javascript" tal:content="string:
                  YUI().use('base', 'node', 'event', function(Y) {