I wrote:
> If this is not in fact completely broken, the reason must be that
> ats_modifiedcols will always be the same for the same values of
> ats_tgoid/ats_relid/ats_event/ats_table (within a given transaction).
> That seems unlikely,
Indeed, it's false. I was able to make a test case demonstrating
that a trigger relying on tg_updatedcols can be fooled: it will
see the updated-columns set for the first trigger event in the
transaction, although the current event could be from a later
UPDATE with a different column set.
> The obvious fix would involve adding a bms_equal() call to the
> comparison loop in afterTriggerAddEvent, which makes me quite
> sad on performance grounds. Maybe it hardly matters in the
> big scheme of things, though.
The attached patch buys back the performance loss, and incidentally
gets rid of rather serious memory bloat, by not performing
afterTriggerCopyBitmap() unless we actually need a new
AfterTriggerSharedData entry. According to my measurements,
that thinko roughly tripled the space consumed per AFTER UPDATE
event :-(. I'm surprised nobody complained about that yet.
regards, tom lane
diff --git a/contrib/lo/expected/lo.out b/contrib/lo/expected/lo.out
index 65798205a5..2c9c07f783 100644
--- 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 ca36cdb309..d1a9d7cf25 100644
--- 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 acf3e4a3f1..1fa63ab7d0 100644
--- 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);
