Repair incorrect handling of AfterTriggerSharedData.ats_modifiedcols.

This patch fixes two distinct errors that both ultimately trace
to commit 71d60e2aa, which added the ats_modifiedcols field.

The more severe error is that ats_modifiedcols wasn't accounted for
in afterTriggerAddEvent's scanning loop that looks for a pre-existing
duplicate AfterTriggerSharedData.  Thus, a new event could be
incorrectly matched to an AfterTriggerSharedData that has a different
value of ats_modifiedcols, resulting in the wrong tg_updatedcols
bitmap getting passed to the trigger whenever it finally gets fired.
We'd not noticed because (a) few triggers consult tg_updatedcols,
and (b) we had no tests exercising a case where such a trigger was
called as an AFTER trigger.  In the test case added by this commit,
contrib/lo's trigger fails to remove a large object when expected
because (without this fix) it thinks the LO OID column hasn't changed.

The other problem was introduced by commit ce5aaea8c, which copied the
modified-columns bitmap into trigger-related storage.  It made a copy
for every trigger event, whereas what we really want is to make a new
copy only when we make a new AfterTriggerSharedData entry.  (We could
imagine adding extra logic to reduce the number of bitmap copies still
more, but it doesn't look worthwhile at the moment.)  In a simple test
of an UPDATE of 10000000 rows with a single AFTER trigger, this thinko
roughly tripled the amount of memory consumed by the pending-triggers
data structures, from 160446744 to 480443440 bytes.

Fixing the first problem requires introducing a bms_equal() call into
afterTriggerAddEvent's scanning loop, which is slightly annoying from
a speed perspective.  However, getting rid of the excessive bms_copy()
calls from the second problem balances that out; overall speed of
trigger operations is the same or slightly better, in my tests.

Discussion: https://postgr.es/m/3496294.1737501591@sss.pgh.pa.us
Backpatch-through: 13

diff --git a/contrib/lo/expected/lo.out b/contrib/lo/expected/lo.out
index 65798205a5ae07b093d88396e6aa0755920a3830..2c9c07f783092885167efa146b10d980ab5f385e 100644 (file)
--- a/contrib/lo/expected/lo.out
+++ b/contrib/lo/expected/lo.out
@@ -47,6 +47,75 @@ SELECT lo_get(43214);
 DELETE FROM image;
 SELECT lo_get(43214);
 ERROR:  large object 43214 does not exist
+-- Now let's try it with an AFTER trigger
+DROP TRIGGER t_raster ON image;
+CREATE CONSTRAINT TRIGGER t_raster AFTER UPDATE OR DELETE ON image
+    DEFERRABLE INITIALLY DEFERRED
+    FOR EACH ROW EXECUTE PROCEDURE lo_manage(raster);
+SELECT lo_create(43223);
+ lo_create 
+-----------
+     43223
+(1 row)
+
+SELECT lo_create(43224);
+ lo_create 
+-----------
+     43224
+(1 row)
+
+SELECT lo_create(43225);
+ lo_create 
+-----------
+     43225
+(1 row)
+
+INSERT INTO image (title, raster) VALUES ('beautiful image', 43223);
+SELECT lo_get(43223);
+ lo_get 
+--------
+ \x
+(1 row)
+
+UPDATE image SET raster = 43224 WHERE title = 'beautiful image';
+SELECT lo_get(43223);  -- gone
+ERROR:  large object 43223 does not exist
+SELECT lo_get(43224);
+ lo_get 
+--------
+ \x
+(1 row)
+
+-- test updating of unrelated column
+UPDATE image SET title = 'beautiful picture' WHERE title = 'beautiful image';
+SELECT lo_get(43224);
+ lo_get 
+--------
+ \x
+(1 row)
+
+-- this case used to be buggy
+BEGIN;
+UPDATE image SET title = 'beautiful image' WHERE title = 'beautiful picture';
+UPDATE image SET raster = 43225 WHERE title = 'beautiful image';
+SELECT lo_get(43224);
+ lo_get 
+--------
+ \x
+(1 row)
+
+COMMIT;
+SELECT lo_get(43224);  -- gone
+ERROR:  large object 43224 does not exist
+SELECT lo_get(43225);
+ lo_get 
+--------
+ \x
+(1 row)
+
+DELETE FROM image;
+SELECT lo_get(43225);  -- gone
+ERROR:  large object 43225 does not exist
 SELECT lo_oid(1::lo);
  lo_oid 
 --------

diff --git a/contrib/lo/sql/lo.sql b/contrib/lo/sql/lo.sql
index ca36cdb30982dd240bb1550e4c0444302a47caef..d1a9d7cf25540232b22c4d1d1e52203359fc6e61 100644 (file)
--- a/contrib/lo/sql/lo.sql
+++ b/contrib/lo/sql/lo.sql
@@ -27,6 +27,47 @@ DELETE FROM image;
 
 SELECT lo_get(43214);
 
+-- Now let's try it with an AFTER trigger
+
+DROP TRIGGER t_raster ON image;
+
+CREATE CONSTRAINT TRIGGER t_raster AFTER UPDATE OR DELETE ON image
+    DEFERRABLE INITIALLY DEFERRED
+    FOR EACH ROW EXECUTE PROCEDURE lo_manage(raster);
+
+SELECT lo_create(43223);
+SELECT lo_create(43224);
+SELECT lo_create(43225);
+
+INSERT INTO image (title, raster) VALUES ('beautiful image', 43223);
+
+SELECT lo_get(43223);
+
+UPDATE image SET raster = 43224 WHERE title = 'beautiful image';
+
+SELECT lo_get(43223);  -- gone
+SELECT lo_get(43224);
+
+-- test updating of unrelated column
+UPDATE image SET title = 'beautiful picture' WHERE title = 'beautiful image';
+
+SELECT lo_get(43224);
+
+-- this case used to be buggy
+BEGIN;
+UPDATE image SET title = 'beautiful image' WHERE title = 'beautiful picture';
+UPDATE image SET raster = 43225 WHERE title = 'beautiful image';
+SELECT lo_get(43224);
+COMMIT;
+
+SELECT lo_get(43224);  -- gone
+SELECT lo_get(43225);
+
+DELETE FROM image;
+
+SELECT lo_get(43225);  -- gone
+
+
 SELECT lo_oid(1::lo);
 
 DROP TABLE image;

diff --git a/src/backend/commands/trigger.c b/src/backend/commands/trigger.c
index acf3e4a3f1f7de95cf909ea5e4731201d4fba7e2..1fa63ab7d0fe59dae912c69d67d6b44cac35316c 100644 (file)
--- a/src/backend/commands/trigger.c
+++ b/src/backend/commands/trigger.c
@@ -4006,13 +4006,6 @@ afterTriggerCopyBitmap(Bitmapset *src)
    if (src == NULL)
        return NULL;
 
-   /* Create event context if we didn't already */
-   if (afterTriggers.event_cxt == NULL)
-       afterTriggers.event_cxt =
-           AllocSetContextCreate(TopTransactionContext,
-                                 "AfterTriggerEvents",
-                                 ALLOCSET_DEFAULT_SIZES);
-
    oldcxt = MemoryContextSwitchTo(afterTriggers.event_cxt);
 
    dst = bms_copy(src);
@@ -4117,16 +4110,21 @@ afterTriggerAddEvent(AfterTriggerEventList *events,
         (char *) newshared >= chunk->endfree;
         newshared--)
    {
+       /* compare fields roughly by probability of them being different */
        if (newshared->ats_tgoid == evtshared->ats_tgoid &&
-           newshared->ats_relid == evtshared->ats_relid &&
            newshared->ats_event == evtshared->ats_event &&
+           newshared->ats_firing_id == 0 &&
            newshared->ats_table == evtshared->ats_table &&
-           newshared->ats_firing_id == 0)
+           newshared->ats_relid == evtshared->ats_relid &&
+           bms_equal(newshared->ats_modifiedcols,
+                     evtshared->ats_modifiedcols))
            break;
    }
    if ((char *) newshared < chunk->endfree)
    {
        *newshared = *evtshared;
+       /* now we must make a suitably-long-lived copy of the bitmap */
+       newshared->ats_modifiedcols = afterTriggerCopyBitmap(evtshared->ats_modifiedcols);
        newshared->ats_firing_id = 0;   /* just to be sure */
        chunk->endfree = (char *) newshared;
    }
@@ -6437,7 +6435,7 @@ AfterTriggerSaveEvent(EState *estate, ResultRelInfo *relinfo,
            new_shared.ats_table = transition_capture->tcs_private;
        else
            new_shared.ats_table = NULL;
-       new_shared.ats_modifiedcols = afterTriggerCopyBitmap(modifiedCols);
+       new_shared.ats_modifiedcols = modifiedCols;
 
        afterTriggerAddEvent(&afterTriggers.query_stack[afterTriggers.query_depth].events,
                             &new_event, &new_shared);