allow trigger to get updated columns
Started by Peter Eisentrautalmost 6 years ago5 messages
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]/messages/by-id/b05e781a-fa16-6b52-6738-761181204567@2ndquadrant.com: 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]: /messages/by-id/b05e781a-fa16-6b52-6738-761181204567@2ndquadrant.com
/messages/by-id/b05e781a-fa16-6b52-6738-761181204567@2ndquadrant.com
--
Peter Eisentraut http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Attachments:
0001-Code-simplification.patchtext/plain; charset=UTF-8; name=0001-Code-simplification.patch; x-mac-creator=0; x-mac-type=0Download
From 0c901266a1b40a257320166fdacaaefd00e4dbce Mon Sep 17 00:00:00 2001
From: Peter Eisentraut <peter@eisentraut.org>
Date: Mon, 24 Feb 2020 10:12:10 +0100
Subject: [PATCH 1/2] Code simplification
Initialize TriggerData to 0 for the whole struct together, instead of
each field separately.
---
src/backend/commands/tablecmds.c | 4 +-
src/backend/commands/trigger.c | 78 +++++---------------------------
2 files changed, 12 insertions(+), 70 deletions(-)
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index b7c8d663fc..6af984ff10 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -10264,7 +10264,7 @@ validateForeignKeyConstraint(char *conname,
while (table_scan_getnextslot(scan, ForwardScanDirection, slot))
{
LOCAL_FCINFO(fcinfo, 0);
- TriggerData trigdata;
+ TriggerData trigdata = {0};
CHECK_FOR_INTERRUPTS();
@@ -10283,8 +10283,6 @@ validateForeignKeyConstraint(char *conname,
trigdata.tg_relation = rel;
trigdata.tg_trigtuple = ExecFetchSlotHeapTuple(slot, false, NULL);
trigdata.tg_trigslot = slot;
- trigdata.tg_newtuple = NULL;
- trigdata.tg_newslot = NULL;
trigdata.tg_trigger = &trig;
fcinfo->context = (Node *) &trigdata;
diff --git a/src/backend/commands/trigger.c b/src/backend/commands/trigger.c
index b9b1262e30..26593576fd 100644
--- a/src/backend/commands/trigger.c
+++ b/src/backend/commands/trigger.c
@@ -2458,7 +2458,7 @@ ExecBSInsertTriggers(EState *estate, ResultRelInfo *relinfo)
{
TriggerDesc *trigdesc;
int i;
- TriggerData LocTriggerData;
+ TriggerData LocTriggerData = {0};
trigdesc = relinfo->ri_TrigDesc;
@@ -2476,12 +2476,6 @@ ExecBSInsertTriggers(EState *estate, ResultRelInfo *relinfo)
LocTriggerData.tg_event = TRIGGER_EVENT_INSERT |
TRIGGER_EVENT_BEFORE;
LocTriggerData.tg_relation = relinfo->ri_RelationDesc;
- LocTriggerData.tg_trigtuple = NULL;
- LocTriggerData.tg_newtuple = NULL;
- LocTriggerData.tg_trigslot = NULL;
- LocTriggerData.tg_newslot = NULL;
- LocTriggerData.tg_oldtable = NULL;
- LocTriggerData.tg_newtable = NULL;
for (i = 0; i < trigdesc->numtriggers; i++)
{
Trigger *trigger = &trigdesc->triggers[i];
@@ -2528,7 +2522,7 @@ ExecBRInsertTriggers(EState *estate, ResultRelInfo *relinfo,
TriggerDesc *trigdesc = relinfo->ri_TrigDesc;
HeapTuple newtuple = NULL;
bool should_free;
- TriggerData LocTriggerData;
+ TriggerData LocTriggerData = {0};
int i;
LocTriggerData.type = T_TriggerData;
@@ -2536,12 +2530,6 @@ ExecBRInsertTriggers(EState *estate, ResultRelInfo *relinfo,
TRIGGER_EVENT_ROW |
TRIGGER_EVENT_BEFORE;
LocTriggerData.tg_relation = relinfo->ri_RelationDesc;
- LocTriggerData.tg_trigtuple = NULL;
- LocTriggerData.tg_newtuple = NULL;
- LocTriggerData.tg_trigslot = NULL;
- LocTriggerData.tg_newslot = NULL;
- LocTriggerData.tg_oldtable = NULL;
- LocTriggerData.tg_newtable = NULL;
for (i = 0; i < trigdesc->numtriggers; i++)
{
Trigger *trigger = &trigdesc->triggers[i];
@@ -2610,7 +2598,7 @@ ExecIRInsertTriggers(EState *estate, ResultRelInfo *relinfo,
TriggerDesc *trigdesc = relinfo->ri_TrigDesc;
HeapTuple newtuple = NULL;
bool should_free;
- TriggerData LocTriggerData;
+ TriggerData LocTriggerData = {0};
int i;
LocTriggerData.type = T_TriggerData;
@@ -2618,12 +2606,6 @@ ExecIRInsertTriggers(EState *estate, ResultRelInfo *relinfo,
TRIGGER_EVENT_ROW |
TRIGGER_EVENT_INSTEAD;
LocTriggerData.tg_relation = relinfo->ri_RelationDesc;
- LocTriggerData.tg_trigtuple = NULL;
- LocTriggerData.tg_newtuple = NULL;
- LocTriggerData.tg_trigslot = NULL;
- LocTriggerData.tg_newslot = NULL;
- LocTriggerData.tg_oldtable = NULL;
- LocTriggerData.tg_newtable = NULL;
for (i = 0; i < trigdesc->numtriggers; i++)
{
Trigger *trigger = &trigdesc->triggers[i];
@@ -2675,7 +2657,7 @@ ExecBSDeleteTriggers(EState *estate, ResultRelInfo *relinfo)
{
TriggerDesc *trigdesc;
int i;
- TriggerData LocTriggerData;
+ TriggerData LocTriggerData = {0};
trigdesc = relinfo->ri_TrigDesc;
@@ -2693,12 +2675,6 @@ ExecBSDeleteTriggers(EState *estate, ResultRelInfo *relinfo)
LocTriggerData.tg_event = TRIGGER_EVENT_DELETE |
TRIGGER_EVENT_BEFORE;
LocTriggerData.tg_relation = relinfo->ri_RelationDesc;
- LocTriggerData.tg_trigtuple = NULL;
- LocTriggerData.tg_newtuple = NULL;
- LocTriggerData.tg_trigslot = NULL;
- LocTriggerData.tg_newslot = NULL;
- LocTriggerData.tg_oldtable = NULL;
- LocTriggerData.tg_newtable = NULL;
for (i = 0; i < trigdesc->numtriggers; i++)
{
Trigger *trigger = &trigdesc->triggers[i];
@@ -2755,7 +2731,7 @@ ExecBRDeleteTriggers(EState *estate, EPQState *epqstate,
TupleTableSlot *slot = ExecGetTriggerOldSlot(estate, relinfo);
TriggerDesc *trigdesc = relinfo->ri_TrigDesc;
bool result = true;
- TriggerData LocTriggerData;
+ TriggerData LocTriggerData = {0};
HeapTuple trigtuple;
bool should_free = false;
int i;
@@ -2794,12 +2770,6 @@ ExecBRDeleteTriggers(EState *estate, EPQState *epqstate,
TRIGGER_EVENT_ROW |
TRIGGER_EVENT_BEFORE;
LocTriggerData.tg_relation = relinfo->ri_RelationDesc;
- LocTriggerData.tg_trigtuple = NULL;
- LocTriggerData.tg_newtuple = NULL;
- LocTriggerData.tg_trigslot = NULL;
- LocTriggerData.tg_newslot = NULL;
- LocTriggerData.tg_oldtable = NULL;
- LocTriggerData.tg_newtable = NULL;
for (i = 0; i < trigdesc->numtriggers; i++)
{
HeapTuple newtuple;
@@ -2872,7 +2842,7 @@ ExecIRDeleteTriggers(EState *estate, ResultRelInfo *relinfo,
{
TriggerDesc *trigdesc = relinfo->ri_TrigDesc;
TupleTableSlot *slot = ExecGetTriggerOldSlot(estate, relinfo);
- TriggerData LocTriggerData;
+ TriggerData LocTriggerData = {0};
int i;
LocTriggerData.type = T_TriggerData;
@@ -2880,12 +2850,6 @@ ExecIRDeleteTriggers(EState *estate, ResultRelInfo *relinfo,
TRIGGER_EVENT_ROW |
TRIGGER_EVENT_INSTEAD;
LocTriggerData.tg_relation = relinfo->ri_RelationDesc;
- LocTriggerData.tg_trigtuple = NULL;
- LocTriggerData.tg_newtuple = NULL;
- LocTriggerData.tg_trigslot = NULL;
- LocTriggerData.tg_newslot = NULL;
- LocTriggerData.tg_oldtable = NULL;
- LocTriggerData.tg_newtable = NULL;
ExecForceStoreHeapTuple(trigtuple, slot, false);
@@ -2924,7 +2888,7 @@ ExecBSUpdateTriggers(EState *estate, ResultRelInfo *relinfo)
{
TriggerDesc *trigdesc;
int i;
- TriggerData LocTriggerData;
+ TriggerData LocTriggerData = {0};
Bitmapset *updatedCols;
trigdesc = relinfo->ri_TrigDesc;
@@ -2945,12 +2909,6 @@ ExecBSUpdateTriggers(EState *estate, ResultRelInfo *relinfo)
LocTriggerData.tg_event = TRIGGER_EVENT_UPDATE |
TRIGGER_EVENT_BEFORE;
LocTriggerData.tg_relation = relinfo->ri_RelationDesc;
- LocTriggerData.tg_trigtuple = NULL;
- LocTriggerData.tg_newtuple = NULL;
- LocTriggerData.tg_trigslot = NULL;
- LocTriggerData.tg_newslot = NULL;
- LocTriggerData.tg_oldtable = NULL;
- LocTriggerData.tg_newtable = NULL;
for (i = 0; i < trigdesc->numtriggers; i++)
{
Trigger *trigger = &trigdesc->triggers[i];
@@ -3005,7 +2963,7 @@ ExecBRUpdateTriggers(EState *estate, EPQState *epqstate,
HeapTuple trigtuple;
bool should_free_trig = false;
bool should_free_new = false;
- TriggerData LocTriggerData;
+ TriggerData LocTriggerData = {0};
int i;
Bitmapset *updatedCols;
LockTupleMode lockmode;
@@ -3058,8 +3016,6 @@ ExecBRUpdateTriggers(EState *estate, EPQState *epqstate,
TRIGGER_EVENT_ROW |
TRIGGER_EVENT_BEFORE;
LocTriggerData.tg_relation = relinfo->ri_RelationDesc;
- LocTriggerData.tg_oldtable = NULL;
- LocTriggerData.tg_newtable = NULL;
updatedCols = GetAllUpdatedColumns(relinfo, estate);
for (i = 0; i < trigdesc->numtriggers; i++)
{
@@ -3173,7 +3129,7 @@ ExecIRUpdateTriggers(EState *estate, ResultRelInfo *relinfo,
TupleTableSlot *oldslot = ExecGetTriggerOldSlot(estate, relinfo);
HeapTuple newtuple = NULL;
bool should_free;
- TriggerData LocTriggerData;
+ TriggerData LocTriggerData = {0};
int i;
LocTriggerData.type = T_TriggerData;
@@ -3181,8 +3137,6 @@ ExecIRUpdateTriggers(EState *estate, ResultRelInfo *relinfo,
TRIGGER_EVENT_ROW |
TRIGGER_EVENT_INSTEAD;
LocTriggerData.tg_relation = relinfo->ri_RelationDesc;
- LocTriggerData.tg_oldtable = NULL;
- LocTriggerData.tg_newtable = NULL;
ExecForceStoreHeapTuple(trigtuple, oldslot, false);
@@ -3238,7 +3192,7 @@ ExecBSTruncateTriggers(EState *estate, ResultRelInfo *relinfo)
{
TriggerDesc *trigdesc;
int i;
- TriggerData LocTriggerData;
+ TriggerData LocTriggerData = {0};
trigdesc = relinfo->ri_TrigDesc;
@@ -3251,12 +3205,6 @@ ExecBSTruncateTriggers(EState *estate, ResultRelInfo *relinfo)
LocTriggerData.tg_event = TRIGGER_EVENT_TRUNCATE |
TRIGGER_EVENT_BEFORE;
LocTriggerData.tg_relation = relinfo->ri_RelationDesc;
- LocTriggerData.tg_trigtuple = NULL;
- LocTriggerData.tg_newtuple = NULL;
- LocTriggerData.tg_trigslot = NULL;
- LocTriggerData.tg_newslot = NULL;
- LocTriggerData.tg_oldtable = NULL;
- LocTriggerData.tg_newtable = NULL;
for (i = 0; i < trigdesc->numtriggers; i++)
{
@@ -4182,7 +4130,7 @@ AfterTriggerExecute(EState *estate,
Relation rel = relInfo->ri_RelationDesc;
AfterTriggerShared evtshared = GetTriggerSharedData(event);
Oid tgoid = evtshared->ats_tgoid;
- TriggerData LocTriggerData;
+ TriggerData LocTriggerData = {0};
HeapTuple rettuple;
int tgindx;
bool should_free_trig = false;
@@ -4191,10 +4139,6 @@ AfterTriggerExecute(EState *estate,
/*
* Locate trigger in trigdesc.
*/
- LocTriggerData.tg_trigger = NULL;
- LocTriggerData.tg_trigslot = NULL;
- LocTriggerData.tg_newslot = NULL;
-
for (tgindx = 0; tgindx < trigdesc->numtriggers; tgindx++)
{
if (trigdesc->triggers[tgindx].tgoid == tgoid)
--
2.25.0
0002-Add-tg_updatedcols-to-TriggerData.patchtext/plain; charset=UTF-8; name=0002-Add-tg_updatedcols-to-TriggerData.patch; x-mac-creator=0; x-mac-type=0Download
From a713a831c464cd629ffc9d8627199b9ab88a41ba Mon Sep 17 00:00:00 2001
From: Peter Eisentraut <peter@eisentraut.org>
Date: Mon, 24 Feb 2020 10:12:10 +0100
Subject: [PATCH 2/2] Add tg_updatedcols to TriggerData
This allows a trigger function to determine for an UPDATE trigger
which columns were actually updated. This allows some optimizations
in generic trigger functions such as lo_manage and
tsvector_update_trigger.
---
contrib/lo/lo.c | 3 ++-
doc/src/sgml/trigger.sgml | 20 ++++++++++++++++++++
src/backend/commands/trigger.c | 6 ++++++
src/backend/utils/adt/tsvector_op.c | 29 +++++++++++++++++++++--------
src/include/commands/trigger.h | 1 +
5 files changed, 50 insertions(+), 9 deletions(-)
diff --git a/contrib/lo/lo.c b/contrib/lo/lo.c
index 4585923ee2..b9847081db 100644
--- a/contrib/lo/lo.c
+++ b/contrib/lo/lo.c
@@ -74,7 +74,8 @@ lo_manage(PG_FUNCTION_ARGS)
* Here, if the value of the monitored attribute changes, then the large
* object associated with the original value is unlinked.
*/
- if (newtuple != NULL)
+ if (newtuple != NULL &&
+ bms_is_member(attnum - FirstLowInvalidHeapAttributeNumber, trigdata->tg_updatedcols))
{
char *orig = SPI_getvalue(trigtuple, tupdesc, attnum);
char *newv = SPI_getvalue(newtuple, tupdesc, attnum);
diff --git a/doc/src/sgml/trigger.sgml b/doc/src/sgml/trigger.sgml
index ba94acad69..bda7866ecb 100644
--- a/doc/src/sgml/trigger.sgml
+++ b/doc/src/sgml/trigger.sgml
@@ -515,6 +515,7 @@ <title>Writing Trigger Functions in C</title>
TupleTableSlot *tg_newslot;
Tuplestorestate *tg_oldtable;
Tuplestorestate *tg_newtable;
+ const Bitmapset *tg_updatedcols;
} TriggerData;
</programlisting>
@@ -757,6 +758,25 @@ <title>Writing Trigger Functions in C</title>
</listitem>
</varlistentry>
+ <varlistentry>
+ <term><structfield>tg_updatedcols</structfield></term>
+ <listitem>
+ <para>
+ For <literal>UPDATE</literal> triggers, a bitmap set indicating the
+ columns that were updated by the triggering command. Generic trigger
+ functions can use this to optimize actions by not having to deal with
+ columns that were not changed.
+ </para>
+
+ <para>
+ As an example, to determine whether a column with attribute number
+ <varname>attnum</varname> (1-based) is a member of this bitmap set,
+ call <literal>bms_is_member(attnum -
+ FirstLowInvalidHeapAttributeNumber,
+ trigdata->tg_updatedcols))</literal>.
+ </para>
+ </listitem>
+ </varlistentry>
</variablelist>
</para>
diff --git a/src/backend/commands/trigger.c b/src/backend/commands/trigger.c
index 26593576fd..9bb5afc8d9 100644
--- a/src/backend/commands/trigger.c
+++ b/src/backend/commands/trigger.c
@@ -2909,6 +2909,7 @@ ExecBSUpdateTriggers(EState *estate, ResultRelInfo *relinfo)
LocTriggerData.tg_event = TRIGGER_EVENT_UPDATE |
TRIGGER_EVENT_BEFORE;
LocTriggerData.tg_relation = relinfo->ri_RelationDesc;
+ LocTriggerData.tg_updatedcols = updatedCols;
for (i = 0; i < trigdesc->numtriggers; i++)
{
Trigger *trigger = &trigdesc->triggers[i];
@@ -3017,6 +3018,7 @@ ExecBRUpdateTriggers(EState *estate, EPQState *epqstate,
TRIGGER_EVENT_BEFORE;
LocTriggerData.tg_relation = relinfo->ri_RelationDesc;
updatedCols = GetAllUpdatedColumns(relinfo, estate);
+ LocTriggerData.tg_updatedcols = updatedCols;
for (i = 0; i < trigdesc->numtriggers; i++)
{
Trigger *trigger = &trigdesc->triggers[i];
@@ -3573,6 +3575,7 @@ typedef struct AfterTriggerSharedData
Oid ats_relid; /* the relation it's on */
CommandId ats_firing_id; /* ID for firing cycle */
struct AfterTriggersTableData *ats_table; /* transition table access */
+ Bitmapset *ats_modifiedcols; /* modified columns */
} AfterTriggerSharedData;
typedef struct AfterTriggerEventData *AfterTriggerEvent;
@@ -4272,6 +4275,8 @@ AfterTriggerExecute(EState *estate,
LocTriggerData.tg_event =
evtshared->ats_event & (TRIGGER_EVENT_OPMASK | TRIGGER_EVENT_ROW);
LocTriggerData.tg_relation = rel;
+ if (TRIGGER_FOR_UPDATE(LocTriggerData.tg_trigger->tgtype))
+ LocTriggerData.tg_updatedcols = evtshared->ats_modifiedcols;
MemoryContextReset(per_tuple_context);
@@ -5959,6 +5964,7 @@ AfterTriggerSaveEvent(EState *estate, ResultRelInfo *relinfo,
new_shared.ats_table = transition_capture->tcs_private;
else
new_shared.ats_table = NULL;
+ new_shared.ats_modifiedcols = modifiedCols;
afterTriggerAddEvent(&afterTriggers.query_stack[afterTriggers.query_depth].events,
&new_event, &new_shared);
diff --git a/src/backend/utils/adt/tsvector_op.c b/src/backend/utils/adt/tsvector_op.c
index cab6874e70..784132aecf 100644
--- a/src/backend/utils/adt/tsvector_op.c
+++ b/src/backend/utils/adt/tsvector_op.c
@@ -2416,6 +2416,7 @@ tsvector_update_trigger(PG_FUNCTION_ARGS, bool config_column)
bool isnull;
text *txt;
Oid cfgId;
+ bool update_needed;
/* Check call context */
if (!CALLED_AS_TRIGGER(fcinfo)) /* internal error */
@@ -2428,9 +2429,15 @@ tsvector_update_trigger(PG_FUNCTION_ARGS, bool config_column)
elog(ERROR, "tsvector_update_trigger: must be fired BEFORE event");
if (TRIGGER_FIRED_BY_INSERT(trigdata->tg_event))
+ {
rettuple = trigdata->tg_trigtuple;
+ update_needed = true;
+ }
else if (TRIGGER_FIRED_BY_UPDATE(trigdata->tg_event))
+ {
rettuple = trigdata->tg_newtuple;
+ update_needed = false; /* computed below */
+ }
else
elog(ERROR, "tsvector_update_trigger: must be fired for INSERT or UPDATE");
@@ -2518,6 +2525,9 @@ tsvector_update_trigger(PG_FUNCTION_ARGS, bool config_column)
errmsg("column \"%s\" is not of a character type",
trigger->tgargs[i])));
+ if (bms_is_member(numattr - FirstLowInvalidHeapAttributeNumber, trigdata->tg_updatedcols))
+ update_needed = true;
+
datum = SPI_getbinval(rettuple, rel->rd_att, numattr, &isnull);
if (isnull)
continue;
@@ -2530,16 +2540,19 @@ tsvector_update_trigger(PG_FUNCTION_ARGS, bool config_column)
pfree(txt);
}
- /* make tsvector value */
- datum = TSVectorGetDatum(make_tsvector(&prs));
- isnull = false;
+ if (update_needed)
+ {
+ /* make tsvector value */
+ datum = TSVectorGetDatum(make_tsvector(&prs));
+ isnull = false;
- /* and insert it into tuple */
- rettuple = heap_modify_tuple_by_cols(rettuple, rel->rd_att,
- 1, &tsvector_attr_num,
- &datum, &isnull);
+ /* and insert it into tuple */
+ rettuple = heap_modify_tuple_by_cols(rettuple, rel->rd_att,
+ 1, &tsvector_attr_num,
+ &datum, &isnull);
- pfree(DatumGetPointer(datum));
+ pfree(DatumGetPointer(datum));
+ }
return PointerGetDatum(rettuple);
}
diff --git a/src/include/commands/trigger.h b/src/include/commands/trigger.h
index 5d69192643..a40ddf5db5 100644
--- a/src/include/commands/trigger.h
+++ b/src/include/commands/trigger.h
@@ -39,6 +39,7 @@ typedef struct TriggerData
TupleTableSlot *tg_newslot;
Tuplestorestate *tg_oldtable;
Tuplestorestate *tg_newtable;
+ const Bitmapset *tg_updatedcols;
} TriggerData;
/*
--
2.25.0
Re: 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 triggerIs 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
Attachments:
lo_trig_test.diffapplication/octet-stream; name=lo_trig_test.diff; x-unix-mode=0644Download
diff --git a/contrib/lo/expected/lo.out b/contrib/lo/expected/lo.out
index f7104aee3f..de5dbcb0f9 100644
--- a/contrib/lo/expected/lo.out
+++ b/contrib/lo/expected/lo.out
@@ -36,6 +36,13 @@ SELECT lo_get(43214);
\x
(1 row)
+UPDATE image SET title = 'beautiful picture' WHERE title = 'beautiful image';
+SELECT lo_get(43214);
+ lo_get
+--------
+ \x
+(1 row)
+
DELETE FROM image;
SELECT lo_get(43214);
ERROR: large object 43214 does not exist
diff --git a/contrib/lo/sql/lo.sql b/contrib/lo/sql/lo.sql
index 34ba6f00ec..1e96e67aba 100644
--- a/contrib/lo/sql/lo.sql
+++ b/contrib/lo/sql/lo.sql
@@ -18,6 +18,10 @@ UPDATE image SET raster = 43214 WHERE title = 'beautiful image';
SELECT lo_get(43213);
SELECT lo_get(43214);
+UPDATE image SET title = 'beautiful picture' WHERE title = 'beautiful image';
+
+SELECT lo_get(43214);
+
DELETE FROM image;
SELECT lo_get(43214);
Re: allow trigger to get updated columns
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 triggerIs 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
Re: allow trigger to get updated columns
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
Re: allow trigger to get updated columns
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
Attachments:
fix-after-trigger-modified-bitmap-management.patchtext/x-diff; charset=us-ascii; name=fix-after-trigger-modified-bitmap-management.patchDownload
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);