Обсуждение: allow trigger to get updated columns

This is a change to make the bitmap of updated columns available to a 
trigger in TriggerData.  This is the same idea as was recently done to 
generated columns [0]: Generic triggers such as tsvector_update_trigger 
can use this information to skip work if the columns they are interested 
in haven't changed.  With the generated columns change, perhaps this 
isn't so interesting anymore, but I suspect a lot of existing 
installations still use tsvector_update_trigger.  In any case, since I 
had already written the code, I figured I post it here.  Perhaps there 
are other use cases.


[0]: 
https://www.postgresql.org/message-id/flat/b05e781a-fa16-6b52-6738-761181204567@2ndquadrant.com

-- 
Peter Eisentraut              http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

> On 24 Feb 2020, at 10:58, Peter Eisentraut <peter.eisentraut@2ndquadrant.com> wrote:
>
> This is a change to make the bitmap of updated columns available to a trigger in TriggerData.  This is the same idea
aswas recently done to generated columns [0]: Generic triggers such as tsvector_update_trigger can use this information
toskip work if the columns they are interested in haven't changed.  With the generated columns change, perhaps this
isn'tso interesting anymore, but I suspect a lot of existing installations still use tsvector_update_trigger.  In any
case,since I had already written the code, I figured I post it here.  Perhaps there are other use cases. 

I wouldn't at all be surprised if there are usecases for this in the wild, and
given the very minor impact I absolutely think it's worth doing.  The patches
both apply, compile and pass tests without warnings.

The 0001 refactoring patch seems a clear win to me.

In the 0002 patch:

+        For <literal>UPDATE</literal> triggers, a bitmap set indicating the
+        columns that were updated by the triggering command.  Generic trigger

Is it worth pointing out that tg_updatedcols will be NULL rather than an empty
Bitmapset for non-UPDATE triggers?  bitmapset.c treats NULL as an empty bitmap
but since a Bitmapset can be allocated but empty, maybe it's worth being
explicit to help developers?

There isn't really a test suite that excercises this IIUC, how about adding
something like the attached diff to contrib/lo?  It seemed like a lower impact
change than widening test_tsvector.

+1 on the patchset, marking this entry as Ready For Committer.

cheers ./daniel

On 2020-03-05 13:53, Daniel Gustafsson wrote:
> The 0001 refactoring patch seems a clear win to me.
> 
> In the 0002 patch:
> 
> +        For <literal>UPDATE</literal> triggers, a bitmap set indicating the
> +        columns that were updated by the triggering command.  Generic trigger
> 
> Is it worth pointing out that tg_updatedcols will be NULL rather than an empty
> Bitmapset for non-UPDATE triggers?  bitmapset.c treats NULL as an empty bitmap
> but since a Bitmapset can be allocated but empty, maybe it's worth being
> explicit to help developers?

done

> There isn't really a test suite that excercises this IIUC, how about adding
> something like the attached diff to contrib/lo?  It seemed like a lower impact
> change than widening test_tsvector.

done

> +1 on the patchset, marking this entry as Ready For Committer.

and done

-- 
Peter Eisentraut              http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Peter Eisentraut <peter.eisentraut@2ndquadrant.com> writes:
> On 2020-03-05 13:53, Daniel Gustafsson wrote:
>> +1 on the patchset, marking this entry as Ready For Committer.

> and done

While looking at a pending patch, I happened to notice that commit
71d60e2aa added a field ats_modifiedcols to AfterTriggerSharedData,
but did not change the logic in afterTriggerAddEvent that decides
whether the new event's evtshared matches some existing one.
Thus, we could seize on a pre-existing entry that matches in
everything except ats_modifiedcols, resulting in ultimately
firing the trigger with the wrong modifiedcols information.

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, although if it were true we could perhaps put
the data somewhere else instead of bloating AfterTriggerSharedData.

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.

            regards, tom lane

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);