launchpad-reviewers team mailing list archive

[Colin Watson <cjwatson@xxxxxxxxxxxxx>]

Colin Watson has proposed merging ~cjwatson/launchpad:db-bugsummary-statement-triggers into launchpad:db-devel.

Commit message:
Rewrite BugSummary triggers to be statement-level

Requested reviews:
  Stuart Bishop (stub): db
  Launchpad code reviewers (launchpad-reviewers): db

For more details, see:
https://code.launchpad.net/~cjwatson/launchpad/+git/launchpad/+merge/373765

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.
-- 
Your team Launchpad code reviewers is requested to review the proposed merge of ~cjwatson/launchpad:db-bugsummary-statement-triggers into launchpad:db-devel.

diff --git a/database/schema/patch-2210-06-0.sql b/database/schema/patch-2210-06-0.sql
new file mode 100644
index 0000000..b411d69
--- /dev/null
+++ b/database/schema/patch-2210-06-0.sql
@@ -0,0 +1,251 @@
+-- Copyright 2019 Canonical Ltd.  This software is licensed under the
+-- GNU Affero General Public License version 3 (see the file LICENSE).
+
+SET client_min_messages=ERROR;
+
+-- 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.
+
+DROP TRIGGER bugtaskflat_maintain_bug_summary ON bugtaskflat;
+DROP TRIGGER bugtag_maintain_bug_summary_before_trigger ON bugtag;
+DROP TRIGGER bugtag_maintain_bug_summary_after_trigger ON bugtag;
+
+-- Similar to the previous bugsummary_tags, but returns an array of tags on
+-- the given bug plus NULL; this can be passed to other functions, and is
+-- suitable for constructing a set of rows to insert into BugSummaryJournal
+-- when handling changes to BugTaskFlat.
+CREATE OR REPLACE FUNCTION bugsummary_tag_names(bug integer)
+    RETURNS text[]
+    LANGUAGE sql STABLE
+    AS $_$
+    SELECT array_agg(tag)
+    FROM (
+        SELECT BugTag.tag FROM BugTag WHERE BugTag.bug = $1
+        UNION ALL
+        SELECT NULL::text
+    ) AS tag;
+$_$;
+
+COMMENT ON FUNCTION bugsummary_tag_names IS
+    'Return an array of the tag names on the given bug, plus NULL; this is suitable for constructing BugSummaryJournal rows when handling changes to BugTaskFlat.';
+
+-- Modify the existing bugsummary_locations to accept an array of tags
+-- rather than looking them up for itself.
+DROP FUNCTION bugsummary_locations;
+CREATE FUNCTION bugsummary_locations(btf_row bugtaskflat, tags text[])
+    RETURNS SETOF bugsummary
+    LANGUAGE plpgsql
+    AS $$
+BEGIN
+    IF btf_row.duplicateof IS NOT NULL THEN
+        RETURN;
+    END IF;
+    RETURN QUERY
+        SELECT
+            CAST(NULL AS integer) AS id,
+            CAST(1 AS integer) AS count,
+            bug_targets.product, bug_targets.productseries,
+            bug_targets.distribution, bug_targets.distroseries,
+            bug_targets.sourcepackagename,
+            bug_viewers.viewed_by, bug_tags.tag, btf_row.status,
+            btf_row.milestone, btf_row.importance,
+            btf_row.latest_patch_uploaded IS NOT NULL AS has_patch,
+            bug_viewers.access_policy
+        FROM
+            bugsummary_targets(btf_row) AS bug_targets,
+            unnest(tags) AS bug_tags (tag),
+            bugsummary_viewers(btf_row) AS bug_viewers;
+END;
+$$;
+
+-- Modify the existing bugsummary_journal_bugtaskflat to accept an array of
+-- tags, and to return a set of rows to the caller rather than inserting
+-- them into a temporary table.  This can now be just SQL rather than
+-- PL/pgSQL.
+DROP FUNCTION bugsummary_journal_bugtaskflat;
+CREATE FUNCTION bugsummary_journal_bugtaskflat(btf_row bugtaskflat, tags text[], _count integer)
+    RETURNS SETOF bugsummaryjournal
+    LANGUAGE sql
+    AS $$
+    SELECT
+        CAST(NULL AS integer) as id,
+        _count, product, productseries, distribution,
+        distroseries, sourcepackagename, viewed_by, tag,
+        status, milestone, importance, has_patch, access_policy
+        FROM bugsummary_locations(btf_row, tags);
+$$;
+
+-- Rewrite the existing bugtaskflat_maintain_bug_summary as a
+-- statement-level trigger.
+CREATE OR REPLACE FUNCTION bugtaskflat_maintain_bug_summary()
+    RETURNS trigger
+    LANGUAGE plpgsql SECURITY DEFINER
+    SET search_path TO 'public'
+    AS $$
+DECLARE
+    summary_queries text[] DEFAULT '{}';
+BEGIN
+    -- Work out the subqueries we need to compute the set of
+    -- BugSummaryJournal rows that should be inserted.  (We have to use
+    -- dynamic commands for this, because the transition tables are not
+    -- visible in functions called by this trigger function.)
+    IF TG_OP = 'DELETE' OR TG_OP = 'UPDATE' THEN
+        summary_queries := array_append(summary_queries, $_$
+            SELECT summary.*
+            FROM
+                old_bugtaskflat btf_row,
+                LATERAL bugsummary_journal_bugtaskflat(
+                    btf_row, bugsummary_tag_names(btf_row.bug), -1
+                ) AS summary
+        $_$);
+    END IF;
+    IF TG_OP = 'INSERT' OR TG_OP = 'UPDATE' THEN
+        summary_queries := array_append(summary_queries, $_$
+            SELECT summary.*
+            FROM
+                new_bugtaskflat btf_row,
+                LATERAL bugsummary_journal_bugtaskflat(
+                    btf_row, bugsummary_tag_names(btf_row.bug), 1
+                ) AS summary
+        $_$);
+    END IF;
+    -- We sum the rows here to minimise the number of inserts into the
+    -- journal, as in the case of UPDATE statement we may have -1s and +1s
+    -- cancelling each other out.
+    EXECUTE ($_$
+        INSERT INTO BugSummaryJournal(
+            count, product, productseries, distribution,
+            distroseries, sourcepackagename, viewed_by, tag,
+            status, milestone, importance, has_patch, access_policy)
+        SELECT
+            SUM(count), product, productseries, distribution,
+            distroseries, sourcepackagename, viewed_by, tag,
+            status, milestone, importance, has_patch, access_policy
+        FROM (
+    $_$ || array_to_string(summary_queries, 'UNION ALL') || $_$
+        ) AS summary
+        GROUP BY
+            product, productseries, distribution,
+            distroseries, sourcepackagename, viewed_by, tag,
+            status, milestone, importance, has_patch, access_policy
+        HAVING SUM(count) != 0;
+    $_$);
+    RETURN NULL;
+END;
+$$;
+
+CREATE TRIGGER bugtaskflat_maintain_bug_summary_insert
+    AFTER INSERT ON bugtaskflat
+    REFERENCING NEW TABLE AS new_bugtaskflat
+    FOR EACH STATEMENT EXECUTE PROCEDURE bugtaskflat_maintain_bug_summary();
+
+CREATE TRIGGER bugtaskflat_maintain_bug_summary_update
+    AFTER UPDATE ON bugtaskflat
+    REFERENCING OLD TABLE AS old_bugtaskflat NEW TABLE AS new_bugtaskflat
+    FOR EACH STATEMENT EXECUTE PROCEDURE bugtaskflat_maintain_bug_summary();
+
+CREATE TRIGGER bugtaskflat_maintain_bug_summary_delete
+    AFTER DELETE ON bugtaskflat
+    REFERENCING OLD TABLE AS old_bugtaskflat
+    FOR EACH STATEMENT EXECUTE PROCEDURE bugtaskflat_maintain_bug_summary();
+
+-- Modify the existing bugsummary_journal_bug to accept an array of tags,
+-- and to return a set of rows to the caller rather than inserting them into
+-- a temporary table.  Rephrasing the loop using LATERAL allows this to be
+-- just SQL rather than PL/pgSQL.
+DROP FUNCTION bugsummary_journal_bug;
+CREATE FUNCTION bugsummary_journal_bug(bug_row bug, tags text[], _count integer)
+    RETURNS SETOF bugsummaryjournal
+    LANGUAGE sql
+    AS $$
+    SELECT summary.*
+    FROM
+        bugtaskflat btf_row,
+        LATERAL bugsummary_journal_bugtaskflat(
+            btf_row, tags, _count
+        ) AS summary
+    WHERE bug = bug_row.id;
+$$;
+
+-- Rewrite the existing bugtag_maintain_bug_summary as a statement-level
+-- trigger.
+CREATE OR REPLACE FUNCTION bugtag_maintain_bug_summary()
+    RETURNS trigger
+    LANGUAGE plpgsql SECURITY DEFINER
+    SET search_path TO 'public'
+    AS $$
+DECLARE
+    summary_queries text[] DEFAULT '{}';
+BEGIN
+    -- Work out the subqueries we need to compute the set of
+    -- BugSummaryJournal rows that should be inserted.  (We have to use
+    -- dynamic commands for this, because the transition tables are not
+    -- visible in functions called by this trigger function.)
+    IF TG_OP = 'DELETE' OR TG_OP = 'UPDATE' THEN
+        summary_queries := array_append(summary_queries, $_$
+            SELECT summary.*
+            FROM
+                (SELECT bug, array_agg(tag) AS tags
+                 FROM old_bugtag
+                 GROUP BY bug) AS grouped_bugtags,
+                LATERAL bugsummary_journal_bug(
+                    bug_row(grouped_bugtags.bug), grouped_bugtags.tags, -1
+                ) AS summary
+        $_$);
+    END IF;
+    IF TG_OP = 'INSERT' OR TG_OP = 'UPDATE' THEN
+        summary_queries := array_append(summary_queries, $_$
+            SELECT summary.*
+            FROM
+                (SELECT bug, array_agg(tag) AS tags
+                 FROM new_bugtag
+                 GROUP BY bug) AS grouped_bugtags,
+                LATERAL bugsummary_journal_bug(
+                    bug_row(grouped_bugtags.bug), grouped_bugtags.tags, 1
+                ) AS summary
+        $_$);
+    END IF;
+    -- We sum the rows here to minimise the number of inserts into the
+    -- journal, as in the case of UPDATE statement we may have -1s and +1s
+    -- cancelling each other out.
+    EXECUTE ($_$
+        INSERT INTO BugSummaryJournal(
+            count, product, productseries, distribution,
+            distroseries, sourcepackagename, viewed_by, tag,
+            status, milestone, importance, has_patch, access_policy)
+        SELECT
+            SUM(count), product, productseries, distribution,
+            distroseries, sourcepackagename, viewed_by, tag,
+            status, milestone, importance, has_patch, access_policy
+        FROM (
+    $_$ || array_to_string(summary_queries, 'UNION ALL') || $_$
+        ) AS summary
+        GROUP BY
+            product, productseries, distribution,
+            distroseries, sourcepackagename, viewed_by, tag,
+            status, milestone, importance, has_patch, access_policy
+        HAVING SUM(count) != 0;
+    $_$);
+    RETURN NULL;
+END;
+$$;
+
+CREATE TRIGGER bugtag_maintain_bug_summary_insert
+    AFTER INSERT ON bugtag
+    REFERENCING NEW TABLE AS new_bugtag
+    FOR EACH STATEMENT EXECUTE PROCEDURE bugtag_maintain_bug_summary();
+
+CREATE TRIGGER bugtag_maintain_bug_summary_update
+    AFTER UPDATE ON bugtag
+    REFERENCING OLD TABLE AS old_bugtag NEW TABLE AS new_bugtag
+    FOR EACH STATEMENT EXECUTE PROCEDURE bugtag_maintain_bug_summary();
+
+CREATE TRIGGER bugtag_maintain_bug_summary_delete
+    AFTER DELETE ON bugtag
+    REFERENCING OLD TABLE AS old_bugtag
+    FOR EACH STATEMENT EXECUTE PROCEDURE bugtag_maintain_bug_summary();
+
+INSERT INTO LaunchpadDatabaseRevision VALUES (2210, 06, 0);
diff --git a/database/schema/security.cfg b/database/schema/security.cfg
index 1e9ca05..878edbb 100644
--- a/database/schema/security.cfg
+++ b/database/schema/security.cfg
@@ -90,10 +90,11 @@ public.bug_row(integer)                    =
 public.bug_summary_dec(bugsummary)         =
 public.bug_summary_flush_temp_journal()    =
 public.bug_summary_inc(bugsummary)         =
-public.bugsummary_journal_bug(bug, integer) =
-public.bugsummary_journal_bugtaskflat(bugtaskflat, integer) =
-public.bugsummary_locations(bugtaskflat)   =
+public.bugsummary_journal_bug(bug, text[], integer) =
+public.bugsummary_journal_bugtaskflat(bugtaskflat, text[], integer) =
+public.bugsummary_locations(bugtaskflat, text[]) =
 public.bugsummary_tags(bugtaskflat)        =
+public.bugsummary_tag_names(integer)       =
 public.bugsummary_targets(bugtaskflat)     =
 public.bugsummary_viewers(bugtaskflat)     =
 public.ensure_bugsummary_temp_journal()    =