public inbox for [email protected]help / color / mirror / Atom feed
allow trigger to get updated columns 3+ messages / 2 participants [nested] [flat]
* allow trigger to get updated columns @ 2020-02-24 09:58 Peter Eisentraut <[email protected]> 0 siblings, 1 reply; 3+ messages in thread From: Peter Eisentraut @ 2020-02-24 09:58 UTC (permalink / raw) To: pgsql-hackers 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/[email protected] -- Peter Eisentraut http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services From 0c901266a1b40a257320166fdacaaefd00e4dbce Mon Sep 17 00:00:00 2001 From: Peter Eisentraut <[email protected]> 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 From a713a831c464cd629ffc9d8627199b9ab88a41ba Mon Sep 17 00:00:00 2001 From: Peter Eisentraut <[email protected]> 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 Attachments: [text/plain] 0001-Code-simplification.patch (9.5K, ../../[email protected]/2-0001-Code-simplification.patch) download | inline diff: From 0c901266a1b40a257320166fdacaaefd00e4dbce Mon Sep 17 00:00:00 2001 From: Peter Eisentraut <[email protected]> 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 [text/plain] 0002-Add-tg_updatedcols-to-TriggerData.patch (7.0K, ../../[email protected]/3-0002-Add-tg_updatedcols-to-TriggerData.patch) download | inline diff: From a713a831c464cd629ffc9d8627199b9ab88a41ba Mon Sep 17 00:00:00 2001 From: Peter Eisentraut <[email protected]> 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 ^ permalink raw reply [nested|flat] 3+ messages in thread
* Re: allow trigger to get updated columns @ 2020-03-05 12:53 Daniel Gustafsson <[email protected]> parent: Peter Eisentraut <[email protected]> 0 siblings, 1 reply; 3+ messages in thread From: Daniel Gustafsson @ 2020-03-05 12:53 UTC (permalink / raw) To: Peter Eisentraut <[email protected]>; +Cc: pgsql-hackers > On 24 Feb 2020, at 10:58, Peter Eisentraut <[email protected]> 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 Attachments: [application/octet-stream] lo_trig_test.diff (886B, ../../[email protected]/2-lo_trig_test.diff) download | inline diff: 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); ^ permalink raw reply [nested|flat] 3+ messages in thread
* Re: allow trigger to get updated columns @ 2020-03-09 08:39 Peter Eisentraut <[email protected]> parent: Daniel Gustafsson <[email protected]> 0 siblings, 0 replies; 3+ messages in thread From: Peter Eisentraut @ 2020-03-09 08:39 UTC (permalink / raw) To: Daniel Gustafsson <[email protected]>; +Cc: pgsql-hackers 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 ^ permalink raw reply [nested|flat] 3+ messages in thread
end of thread, other threads:[~2020-03-09 08:39 UTC | newest] Thread overview: 3+ messages (download: mbox mbox.gz follow: Atom feed) -- links below jump to the message on this page -- 2020-02-24 09:58 allow trigger to get updated columns Peter Eisentraut <[email protected]> 2020-03-05 12:53 ` Daniel Gustafsson <[email protected]> 2020-03-09 08:39 ` Peter Eisentraut <[email protected]>
This inbox is served by agora; see mirroring instructions for how to clone and mirror all data and code used for this inbox