From ff260840eadfd1cc41528fc503435c04be421083 Mon Sep 17 00:00:00 2001 From: Greg Burd Date: Tue, 10 Mar 2026 08:17:31 -0400 Subject: [PATCH v35 2/3] Identify and track columns modified by heap_modifiy_tuple() on update ExecGetAllUpdatedCols() misses attributes modified using heap_modify_tuple() that are not explictly SET in the UPDATE or by triggers. This happens in one test (tsearch.sql) when the tsvector_update_trigger() is invoked and modifies an indexed attribute that isn't referenced in any SQL. The net is that the functions like HeapDetermineColumnsInfo() have to scan all indexed attributes for changes rather than being able to first reduce the indexed set by intersecting it with the set of attributes known to be potentially updated. While this isn't so bad, it is an oversight should someone in the future build some security related feature using that incomplete result. It also might save a fraction of overhead calculating modified index attributes in heap_update(). This commit adds to ExecBRUpdateTriggers() code that identify changes to indexed columns not found by ExecGetAllUpdatedCols() and adds those attributes to ri_extraUpdatedCols. This commit introduces ExecCompareSlotAttrs() as a utility function to identify those attributes that have changed. It compares a subset of attributes between two TupleTableSlots and returns a Bitmapset of attributes that differ. It would be nice to integrate this into HeapDetermineColumnsInfo(), however it would be a layering violation given that it is within heap_update(). --- src/backend/commands/trigger.c | 20 +++++++- src/backend/executor/execTuples.c | 78 +++++++++++++++++++++++++++++++ src/include/executor/executor.h | 5 ++ 3 files changed, 102 insertions(+), 1 deletion(-) diff --git a/src/backend/commands/trigger.c b/src/backend/commands/trigger.c index 98d402c0a3b..bbe077a9ca9 100644 --- a/src/backend/commands/trigger.c +++ b/src/backend/commands/trigger.c @@ -2978,6 +2978,7 @@ ExecBRUpdateTriggers(EState *estate, EPQState *epqstate, bool is_merge_update) { TriggerDesc *trigdesc = relinfo->ri_TrigDesc; + TupleDesc tupdesc = RelationGetDescr(relinfo->ri_RelationDesc); TupleTableSlot *oldslot = ExecGetTriggerOldSlot(estate, relinfo); HeapTuple newtuple = NULL; HeapTuple trigtuple; @@ -2985,7 +2986,9 @@ ExecBRUpdateTriggers(EState *estate, EPQState *epqstate, bool should_free_new = false; TriggerData LocTriggerData = {0}; int i; - Bitmapset *updatedCols; + Bitmapset *updatedCols = NULL; + Bitmapset *remainingCols = NULL; + Bitmapset *modifiedCols; LockTupleMode lockmode; /* Determine lock mode to use */ @@ -3127,6 +3130,21 @@ ExecBRUpdateTriggers(EState *estate, EPQState *epqstate, if (should_free_trig) heap_freetuple(trigtuple); + /* + * Before UPDATE triggers may have updated attributes not known to + * ExecGetAllUpdatedColumns() using heap_modify_tuple() or + * heap_modifiy_tuple_by_cols(). Find and record those now. + */ + remainingCols = bms_add_range(NULL, 1 - FirstLowInvalidHeapAttributeNumber, + tupdesc->natts - FirstLowInvalidHeapAttributeNumber); + remainingCols = bms_del_members(remainingCols, updatedCols); + modifiedCols = ExecCompareSlotAttrs(tupdesc, remainingCols, oldslot, newslot); + relinfo->ri_extraUpdatedCols = + bms_add_members(relinfo->ri_extraUpdatedCols, modifiedCols); + + bms_free(remainingCols); + bms_free(modifiedCols); + return true; } diff --git a/src/backend/executor/execTuples.c b/src/backend/executor/execTuples.c index b768eae9e53..1064ebe845b 100644 --- a/src/backend/executor/execTuples.c +++ b/src/backend/executor/execTuples.c @@ -66,6 +66,7 @@ #include "nodes/nodeFuncs.h" #include "storage/bufmgr.h" #include "utils/builtins.h" +#include "utils/datum.h" #include "utils/expandeddatum.h" #include "utils/lsyscache.h" #include "utils/typcache.h" @@ -1929,6 +1930,83 @@ ExecFetchSlotHeapTupleDatum(TupleTableSlot *slot) return ret; } +/* + * ExecCompareSlotAttrs + * + * Compare the subset of attributes in attrs bewtween TupleTableSlots to detect + * which attributes have changed. + * + * Returns a Bitmapset of attribute indices (using + * FirstLowInvalidHeapAttributeNumber convention) that differ between the two + * slots. + */ +Bitmapset * +ExecCompareSlotAttrs(TupleDesc tupdesc, const Bitmapset *attrs, + TupleTableSlot *s1, TupleTableSlot *s2) +{ + int attidx = -1; + Bitmapset *modified = NULL; + + /* XXX what if slots don't share the same tupleDescriptor... */ + /* Assert(s1->tts_tupleDescriptor == s2->tts_tupleDescriptor); */ + + while ((attidx = bms_next_member(attrs, attidx)) >= 0) + { + /* attidx is zero-based, attrnum is the normal attribute number */ + AttrNumber attrnum = attidx + FirstLowInvalidHeapAttributeNumber; + Datum value1, + value2; + bool null1, + null2; + CompactAttribute *att; + + /* + * If it's a whole-tuple reference, say "not equal". It's not really + * worth supporting this case, since it could only succeed after a + * no-op update, which is hardly a case worth optimizing for. + */ + if (attrnum == 0) + { + modified = bms_add_member(modified, attidx); + continue; + } + + /* + * Likewise, automatically say "not equal" for any system attribute + * other than tableOID; we cannot expect these to be consistent in a + * HOT chain, or even to be set correctly yet in the new tuple. + */ + if (attrnum < 0) + { + if (attrnum != TableOidAttributeNumber) + { + modified = bms_add_member(modified, attidx); + continue; + } + } + + att = TupleDescCompactAttr(tupdesc, attrnum - 1); + value1 = slot_getattr(s1, attrnum, &null1); + value2 = slot_getattr(s2, attrnum, &null2); + + /* A change to/from NULL, so not equal */ + if (null1 != null2) + { + modified = bms_add_member(modified, attidx); + continue; + } + + /* Both NULL, no change/unmodified */ + if (null2) + continue; + + if (!datum_image_eq(value1, value2, att->attbyval, att->attlen)) + modified = bms_add_member(modified, attidx); + } + + return modified; +} + /* ---------------------------------------------------------------- * convenience initialization routines * ---------------------------------------------------------------- diff --git a/src/include/executor/executor.h b/src/include/executor/executor.h index d46ba59895d..5dcfaa2027f 100644 --- a/src/include/executor/executor.h +++ b/src/include/executor/executor.h @@ -17,6 +17,7 @@ #include "datatype/timestamp.h" #include "executor/execdesc.h" #include "fmgr.h" +#include "nodes/execnodes.h" #include "nodes/lockoptions.h" #include "nodes/parsenodes.h" #include "utils/memutils.h" @@ -606,6 +607,10 @@ extern TupleDesc ExecCleanTypeFromTL(List *targetList); extern TupleDesc ExecTypeFromExprList(List *exprList); extern void ExecTypeSetColNames(TupleDesc typeInfo, List *namesList); extern void UpdateChangedParamSet(PlanState *node, Bitmapset *newchg); +extern Bitmapset *ExecCompareSlotAttrs(TupleDesc tupdesc, + const Bitmapset *attrs, + TupleTableSlot *old_tts, + TupleTableSlot *new_tts); typedef struct TupOutputState { -- 2.51.2