Merge ~cjwatson/launchpad:db-bugsummary-statement-triggers into launchpad:db-devel

Proposed by Colin Watson
Status: Merged
Approved by: Colin Watson
Approved revision: 63fbb23e544d312b59caf14814b2c64f8948fb92
Merge reported by: Otto Co-Pilot
Merged at revision: not available
Proposed branch: ~cjwatson/launchpad:db-bugsummary-statement-triggers
Merge into: launchpad:db-devel
Diff against target: 256 lines (+226/-9)
2 files modified
database/schema/patch-2210-06-0.sql (+224/-0)
database/schema/security.cfg (+2/-9)
Reviewer Review Type Date Requested Status
William Grant db Approve
Ioana Lasc (community) Approve
Review via email: mp+373765@code.qastaging.launchpad.net

Commit message

Rewrite BugSummary triggers to be statement-level

Description of the change

Rewrite the BugSummaryJournal maintenance triggers to make use of the new transition tables provided to AFTER ... FOR EACH STATEMENT triggers as of PostgreSQL 10. Instead of using row-level triggers which accumulate changes in a temporary table and flush it into the journal, we now write directly to the journal at the end of each statement.

There's a considerable amount of complexity here due to transition tables only being visible in the trigger function itself, not in functions that it calls. I used a combination of LATERAL and dynamic commands to minimise the amount of repeated code resulting from this. This does mean that the outermost part of each trigger function's query needs to be replanned each time the trigger runs, but I don't expect that to make a significant performance difference.

The transformations related to bug tags aren't completely obvious. I eliminated summarise_bug and unsummarise_bug from the call chain, as they didn't seem to be pulling their weight. When BugTag is changed, rather than decrementing/incrementing all the BugSummary rows that the changed bugs expand to, it now makes more sense to do so only for the rows relating to the changed tags. This necessitated extending bugsummary_journal_bugtaskflat and friends to take an array of tags, so it now processes all tags on the bug plus NULL when handling BugTaskFlat changes, and only the changed tags when handling BugTag changes.

I can't say for sure whether this will fix the periodic bug update timeouts we've been seeing, since we've never completely got to the bottom of their cause. However, reducing the number of times the trigger functions need to be called and eliminating their use of an explicit temporary table seem likely to improve matters.

This is essentially the same as https://code.launchpad.net/~cjwatson/launchpad/db-bugsummary-statement-triggers/+merge/371297, converted to git and rebased on master.

To post a comment you must log in.
Revision history for this message
Ioana Lasc (ilasc) wrote :

Looks good to me.

review: Approve
Revision history for this message
William Grant (wgrant) wrote :

During the review I wanted to see if a few changes in approach were viable, but ended up going further than expected and wound up with a solution that is structured a bit differently, and doesn't use any dynamic queries at all: https://paste.ubuntu.com/p/SQzFYMBBqZ/

Thoughts on that?

Revision history for this message
Colin Watson (cjwatson) wrote :

After going through very carefully to make sure I understood it: I like it! I think the essential things I'd missed were the local types, the inline rewrite of bugsummary_tag_names using array_append, the trick of accumulating rows in a function-local array, and the trick of assigning to temp_journal.count to simplify fixing up the count in journal rows.

I tidied up your paste slightly (removing a couple of now-obsolete comments, and signing the XXX comments), and committed it. If you're happy with the result then we can probably go ahead?

Revision history for this message
William Grant (wgrant) :
review: Approve (db)

There was an error fetching revisions from git servers. Please try again in a few minutes. If the problem persists, contact Launchpad support.

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
The diff is not available at this time. You can reload the page or download it.

Subscribers

People subscribed via source and target branches

to status/vote changes: