From: Tom Lane <tgl@sss.pgh.pa.us>
To: Peter Eisentraut <peter@eisentraut.org>
cc: Daniel Gustafsson <daniel@yesql.se>,
        pgsql-hackers <pgsql-hackers@postgresql.org>
Subject: Re: allow trigger to get updated columns
In-reply-to: <3496294.1737501591@sss.pgh.pa.us>
References: <11c5f156-67a9-0fb5-8200-2a8018eb2e0c@2ndquadrant.com> <CEA65957-59AA-4BEA-AD33-5334EAC3E34C@yesql.se> <4e534c8d-4c04-6a30-e8e9-aecdec917ccc@2ndquadrant.com> <3496294.1737501591@sss.pgh.pa.us>
Comments: In-reply-to Tom Lane <tgl@sss.pgh.pa.us>
	message dated "Tue, 21 Jan 2025 18:19:51 -0500"
MIME-Version: 1.0
Content-Type: multipart/mixed; boundary="----- =_aaaaaaaaaa0"
Content-ID: <3605257.1737514793.0@sss.pgh.pa.us>
Date: Tue, 21 Jan 2025 21:59:58 -0500
Message-ID: <3605267.1737514798@sss.pgh.pa.us>
Archived-At: <https://www.postgresql.org/message-id/3605267.1737514798%40sss.pgh.pa.us>
Precedence: bulk

------- =_aaaaaaaaaa0
Content-Type: text/plain; charset="us-ascii"
Content-ID: <3605257.1737514793.1@sss.pgh.pa.us>

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


------- =_aaaaaaaaaa0
Content-Type: text/x-diff;
	name="fix-after-trigger-modified-bitmap-management.patch";
	charset="us-ascii"
Content-ID: <3605257.1737514793.2@sss.pgh.pa.us>
Content-Description: fix-after-trigger-modified-bitmap-management.patch
Content-Transfer-Encoding: quoted-printable

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 =3D 43224 WHERE title =3D '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 =3D 'beautiful picture' WHERE title =3D 'beautiful=
 image';
+SELECT lo_get(43224);
+ lo_get =

+--------
+ \x
+(1 row)
+
+-- this case used to be buggy
+BEGIN;
+UPDATE image SET title =3D 'beautiful image' WHERE title =3D 'beautiful p=
icture';
+UPDATE image SET raster =3D 43225 WHERE title =3D '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 =3D 43224 WHERE title =3D 'beautiful image';
+
+SELECT lo_get(43223);  -- gone
+SELECT lo_get(43224);
+
+-- test updating of unrelated column
+UPDATE image SET title =3D 'beautiful picture' WHERE title =3D 'beautiful=
 image';
+
+SELECT lo_get(43224);
+
+-- this case used to be buggy
+BEGIN;
+UPDATE image SET title =3D 'beautiful image' WHERE title =3D 'beautiful p=
icture';
+UPDATE image SET raster =3D 43225 WHERE title =3D '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 =3D=3D NULL)
 		return NULL;
 =

-	/* Create event context if we didn't already */
-	if (afterTriggers.event_cxt =3D=3D NULL)
-		afterTriggers.event_cxt =3D
-			AllocSetContextCreate(TopTransactionContext,
-								  "AfterTriggerEvents",
-								  ALLOCSET_DEFAULT_SIZES);
-
 	oldcxt =3D MemoryContextSwitchTo(afterTriggers.event_cxt);
 =

 	dst =3D bms_copy(src);
@@ -4117,16 +4110,21 @@ afterTriggerAddEvent(AfterTriggerEventList *events=
,
 		 (char *) newshared >=3D chunk->endfree;
 		 newshared--)
 	{
+		/* compare fields roughly by probability of them being different */
 		if (newshared->ats_tgoid =3D=3D evtshared->ats_tgoid &&
-			newshared->ats_relid =3D=3D evtshared->ats_relid &&
 			newshared->ats_event =3D=3D evtshared->ats_event &&
+			newshared->ats_firing_id =3D=3D 0 &&
 			newshared->ats_table =3D=3D evtshared->ats_table &&
-			newshared->ats_firing_id =3D=3D 0)
+			newshared->ats_relid =3D=3D evtshared->ats_relid &&
+			bms_equal(newshared->ats_modifiedcols,
+					  evtshared->ats_modifiedcols))
 			break;
 	}
 	if ((char *) newshared < chunk->endfree)
 	{
 		*newshared =3D *evtshared;
+		/* now we must make a suitably-long-lived copy of the bitmap */
+		newshared->ats_modifiedcols =3D afterTriggerCopyBitmap(evtshared->ats_m=
odifiedcols);
 		newshared->ats_firing_id =3D 0;	/* just to be sure */
 		chunk->endfree =3D (char *) newshared;
 	}
@@ -6437,7 +6435,7 @@ AfterTriggerSaveEvent(EState *estate, ResultRelInfo =
*relinfo,
 			new_shared.ats_table =3D transition_capture->tcs_private;
 		else
 			new_shared.ats_table =3D NULL;
-		new_shared.ats_modifiedcols =3D afterTriggerCopyBitmap(modifiedCols);
+		new_shared.ats_modifiedcols =3D modifiedCols;
 =

 		afterTriggerAddEvent(&afterTriggers.query_stack[afterTriggers.query_dep=
th].events,
 							 &new_event, &new_shared);

------- =_aaaaaaaaaa0--