allow trigger to get updated columns

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 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.

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