On Mon, Feb 16, 2026, at 2:36 PM, Jeff Davis wrote:
> On Sun, 2026-02-15 at 15:39 -0500, Greg Burd wrote:
>
>> (2) Types don't influence this decision today. Their equality
>> operators are not used, attributes are memcmp(). This is a
>> requirement for index-only scans. I think it's insufficient for
>> types like JSONB which have internal structure that can be extracted
>> to form index keys, but that's for a later commit.
>
> That's an interesting path but would require more infrastructure, and
> to justify going down that path we should look for other opportunities
> to use that type infra beyond just HOT. Brainstorming: maybe something
> in the planner can make use of intelligence around expressions that are
> effectively setter/accessor methods on complex types? (Obviously work
> for later.)
Agreed, but this is my best idea at the moment to re-introduce $subjet into
this patch set. I'm open to other ideas, but fundamentally to allow $subjet
requires that somewhere we discover that while portions of the JSONB attribute
changed during the update the resulting index key attributes extracted from the
JSONB did not.
In all patches v1-v29 that was discovered by evaluating the index expressions
on the old and new tuples and comparing the index key datum that resulted for
equality. When equal, while the attribute changed the indexes need not opening
the door for a HOT update. The overhead of that wasn't horrible, but it does
change some expectations about how often expressions are evaluated and that
might be confusing.
>> I can see how this might be confusing, you're asking a good
>> question. Why not just add the mix_attrs as an argument to the table
>> AM update call and be done?
>>
>> 1. Bitmapsets that are NULL mean empty, so how would
>> simple_heap_update() signal to heap_update() that it needs to
>> determine the modified indexed attributes? We'd have to add a bool
>> along with the mix_attrs Bitmapset to indicate: "we've not calculated
>> the set yet, you need to do that."
>
> Maybe an extra bool is not ideal, but it's better than moving code onto
> the wrong side of an API boundary.
I'm not sure this is an "API boundary", but I'm open to debate on that. The
attached patch does add a bool to heap_update() in addition to the Bitmapset of
modified/indexed attributes (modified_attrs) and IMO is fine although I don't
like that it further complicates (muddies?) an already huge and highly
complicated function, heap_update().
>> 2. After fetching the exclusive buffer lock there is the test
>> `!ItemIdIsNormal(lp)` to cover the case where a simple_heap_update()
>> the otid origin is the syscache, there is no pin or snapshot, and so
>> there might be LP_* states other than LP_NORMAL due to concurrent
>> pruning. This only happens when updating catalog tuples, so this
>> logic need not be present at all in the heapam_tuple_update(). Yes,
>> the if() branch will be fast (frequently predicted by the CPU) but
>> this feels like logic specific to the update of catalog tuples.
>
> If convenient I'm fine with moving that branch out, but I think it
> needs to be done with the buffer locked (right?), so heap_update()
> looks like the right place for that test for now.
>
>> 3. HeapDetermineColumnsInfo() actually does more than find the
>> modified indexed attributes, it also performs half of the check for
>> the requirement to WAL log the replica identity attributes. The
>> replacement function in the executor doesn't do this work, so that is
>> coded into heapam_tuple_update() but not simple_heap_update(). The
>> second half is in ExtractReplicaIdentity() that happens later in the
>> heap_update() function after determining if HOT is a possibility or
>> not.
>
> IIUC you are saying that the decision is too heap-specific to expose to
> the executor. I think that's true today (as ExtractReplicaIdentity() is
> in heapam.c), but perhaps that's not fundamental: TOAST is not heap-
> specific, replica IDs are not heap-specific, and if you are WAL-logging
> a replica identity key it seems like you need to know whether it's
> external or not regardless of the AM.
>
> I'm not asking for a change here, just trying to understand the API
> boundaries.
I'm on the fence on this one, maybe all table AMs will need this same logic but
at the moment it feels very heap-specific to me. For now it lives in
heap_update() and HeapDetermineColumnsInfo(). I think I called this out only
to say that if you look at the new vs old method for computing modified_attrs
you'll see that's missing and done later in heap_update().
>> I have moved these changes back into heap_update(), add the mix_attrs
>> and mix_attrs_valid to see how things look, that's the attached
>> patch.
>
> Thank you -- that's easier to understand.
Excellent.
I've muddied the review again a bit by abstracting out a function
ExecCompareSlots() which can loosely be thought of as a replacement for
heap_attr_equal(). Doing that let's me vastly simplify the replication worker
changes and reuse it after ExecBRUpdateTriggers().
> Why does simple_heap_update() need to do the HeapDetermineColumnsInfo()
> inside heap_update()? It seems like you're trying to avoid doing the
> same work the executor is doing to determine the modified_attrs bitmap,
> but either (a) the work is cheap; or (b) the work to make the bitmap is
> expensive.
simple_heap_update() is exclusively called during catalog tuple updates and
does not involve the executor at all, these are direct calls into heap to store
catalog tuples. These updates don't preserve the set of updated attributes
(see cf-6221 [0]) and so for them to potentially use the HOT path in
heap_update() we need to identify which attributes changed. This requires
comparing the new heap tuple vs the one read from the buffer page. This is the
same logic as before the patch. If I could come up with a simple method to
retain the set of modified attributes during catalog tuple updates then we
could excise HeapDetermineColumnsInfo() entirely.
> If (a), then just construct the correct bitmap in simple_heap_update()
> and simplify the code. If (b), then optimizing the simple_heap_update()
> case isn't good enough, we need to find ways of avoiding the work in
> the most common cases in the executor, as well.
Construct the correct bitmap how? That function is called with the otid and
the updated HeapTuple, not enough to build the bitmap of modified indexed
attributes.
I said in an earlier email that the results for the modified/indexed bitmap and
the replica identity are identical. I added some test code (messy, but
attached for your amusement should you want) that looks for differences between
the two. It turns out that for replica identity the results are identical, for
modified/indexed attributes there are a few differences.
In brin.sql at 409:
UPDATE brintest SET int8col = int8col * int4col;
This updates the indexed value of the int8col (attribute 4) which is correctly
detected in the new code, but isn't in HeapDetermineColumnsInfo(). The
"interesting_cols" are identical. I'm still investigating.
I'm also working on a good benchmark that hopefully can show that under heavy
concurrent read/update mixed load the reduced exclusive lock time will allow
more work to be performed. Some contrived benchmarks have shown 15%
improvement, but I need to zero in on this before publishing results.
> Regards,
> Jeff Davis
Attached is a new version of the patch. The file heap-check.c contains the
code I had been using to test for equal results across methods. That code
is... well, it's just for me but I include it to show that I'm doing due
diligence to ensure this stuff is either the same, or different for a good
reason. Thanks for your continued interest in this work.
best.
-greg
[0] https://commitfest.postgresql.org/patch/6221/
From b7cf5928cf400cc66ac8f6ecddd37362f46da386 Mon Sep 17 00:00:00 2001
From: Greg Burd <[email protected]>
Date: Sun, 2 Nov 2025 11:36:20 -0500
Subject: [PATCH v20260217] Idenfity modified indexed attributes in the
executor on UPDATE
Refactor executor update logic to determine which indexed columns have
actually changed during an UPDATE operation rather than leaving this up
to HeapDetermineColumnsInfo() in heap_update().
ExecCheckIndexedAttrsForChanges() replaces HeapDeterminesColumnsInfo()
and is called before table_tuple_update() crucially without the need
for an exclusive buffer lock on the page that holds the tuple being
updated. This reduces the time the lock is held later within
heapam_tuple_update() and heap_update().
ExecCheckIndexedAttrsForChanges() in turn uses ExecCompareSlots() to
identify which attributes have changed and then intersects that with the
set of indexed attributes to identify the modified indexed set.
Besides identifying the set of modified indexed attributes
HeapDetermineColumnsInfo() was also responsible for part of the logic
involed in the decision to include the replica identity key or not.
This now happens in heap_update() when modified_attrs_valid is false.
Catalog tuple updates use simple_heap_update() and don't pass a
modified_attrs Bitmapset into heap_update() indicated by the
modified_attrs_valid bool set to false.
Updates stemming from logical replication also use the new
ExecCheckIndexedAttrsForChanges() in ExecSimpleRelationUpdate().
Before row triggers on UPDATE may use heap_modify_tuple() to update
attributes not identified by ExecGetAllUpdatedCols() as is the case in
tsvector_update_trigger(). ExecBRUpdateTriggers() now identifies this
case and adds such attributes to ri_extraUpdatedCols. See tsearch.sql
tests with this trigger for an example of this.
---
src/backend/access/heap/heapam.c | 80 +++++++++++++++++++++---
src/backend/access/heap/heapam_handler.c | 7 +--
src/backend/access/table/tableam.c | 5 +-
src/backend/commands/trigger.c | 15 ++++-
src/backend/executor/execReplication.c | 12 +++-
src/backend/executor/execTuples.c | 74 ++++++++++++++++++++++
src/backend/executor/nodeModifyTable.c | 72 ++++++++++++++++++---
src/backend/replication/logical/worker.c | 15 +++--
src/backend/utils/cache/relcache.c | 15 +++++
src/include/access/heapam.h | 1 +
src/include/access/tableam.h | 8 ++-
src/include/catalog/index.h | 1 +
src/include/executor/executor.h | 10 +++
src/include/nodes/execnodes.h | 6 ++
src/include/utils/rel.h | 1 +
src/include/utils/relcache.h | 1 +
16 files changed, 287 insertions(+), 36 deletions(-)
diff --git a/src/backend/access/heap/heapam.c b/src/backend/access/heap/heapam.c
index 98d53caeea8..ab8b6ddb8de 100644
--- a/src/backend/access/heap/heapam.c
+++ b/src/backend/access/heap/heapam.c
@@ -3311,6 +3311,7 @@ TM_Result
heap_update(Relation relation, const ItemPointerData *otid, HeapTuple newtup,
CommandId cid, Snapshot crosscheck, bool wait,
TM_FailureData *tmfd, LockTupleMode *lockmode,
+ const Bitmapset *modified_attrs, bool modified_attrs_valid,
TU_UpdateIndexes *update_indexes)
{
TM_Result result;
@@ -3320,7 +3321,6 @@ heap_update(Relation relation, const ItemPointerData *otid, HeapTuple newtup,
Bitmapset *key_attrs;
Bitmapset *id_attrs;
Bitmapset *interesting_attrs;
- Bitmapset *modified_attrs;
ItemId lp;
HeapTupleData oldtup;
HeapTuple heaptup;
@@ -3345,7 +3345,7 @@ heap_update(Relation relation, const ItemPointerData *otid, HeapTuple newtup,
bool all_visible_cleared_new = false;
bool checked_lockers;
bool locker_remains;
- bool id_has_external = false;
+ bool rep_id_key_required = false;
TransactionId xmax_new_tuple,
xmax_old_tuple;
uint16 infomask_old_tuple,
@@ -3487,9 +3487,69 @@ heap_update(Relation relation, const ItemPointerData *otid, HeapTuple newtup,
* new tuple so we must include it as part of the old_key_tuple. See
* ExtractReplicaIdentity.
*/
- modified_attrs = HeapDetermineColumnsInfo(relation, interesting_attrs,
- id_attrs, &oldtup,
- newtup, &id_has_external);
+ if (!modified_attrs_valid)
+ {
+ bool id_has_external = false;
+
+ modified_attrs = HeapDetermineColumnsInfo(relation, interesting_attrs,
+ id_attrs, &oldtup,
+ newtup, &id_has_external);
+ rep_id_key_required = id_has_external ||
+ bms_overlap(modified_attrs, id_attrs);
+ }
+ else
+ {
+ /*
+ * ExtractReplicatIdentity() needs to know if a modified attrbute is
+ * used as a replica indentity or if any of the unmodified indexed
+ * attributes in the old tuple are stored externally and used as a
+ * replica identity.
+ */
+ rep_id_key_required = bms_overlap(modified_attrs, id_attrs);
+ if (!rep_id_key_required)
+ {
+ Bitmapset *attrs;
+ TupleDesc tupdesc = RelationGetDescr(relation);
+ int attidx = -1;
+
+ /* Check all unmodified indexed replica identity key attributes */
+ attrs = bms_difference(interesting_attrs, modified_attrs);
+ attrs = bms_int_members(attrs, id_attrs);
+
+ while ((attidx = bms_next_member(attrs, attidx)) >= 0)
+ {
+ /*
+ * attidx is zero-based, attrnum is the normal attribute
+ * number
+ */
+ AttrNumber attrnum = attidx + FirstLowInvalidHeapAttributeNumber;
+ Datum value;
+ bool isnull;
+
+ /*
+ * System attributes are not added into interesting_attrs in
+ * relcache.
+ */
+ Assert(attrnum > 0);
+
+ value = heap_getattr(&oldtup, attrnum, tupdesc, &isnull);
+
+ /* No need to check attributes that can't be stored externally */
+ if (isnull ||
+ TupleDescCompactAttr(tupdesc, attrnum - 1)->attlen != -1)
+ continue;
+
+ /* Check if the old tuple's attribute is stored externally */
+ if (VARATT_IS_EXTERNAL((struct varlena *) DatumGetPointer(value)))
+ {
+ rep_id_key_required = true;
+ break;
+ }
+ }
+
+ bms_free(attrs);
+ }
+ }
/*
* If we're not updating any "key" column, we can grab a weaker lock type.
@@ -3763,7 +3823,7 @@ l2:
bms_free(sum_attrs);
bms_free(key_attrs);
bms_free(id_attrs);
- bms_free(modified_attrs);
+ /* modified attrs is passed in and free'd by the caller, or NULL */
bms_free(interesting_attrs);
return result;
}
@@ -4111,8 +4171,7 @@ l2:
* columns are modified or it has external data.
*/
old_key_tuple = ExtractReplicaIdentity(relation, &oldtup,
- bms_overlap(modified_attrs, id_attrs) ||
- id_has_external,
+ rep_id_key_required,
&old_key_copied);
/* NO EREPORT(ERROR) from here till changes are logged */
@@ -4278,7 +4337,7 @@ l2:
bms_free(sum_attrs);
bms_free(key_attrs);
bms_free(id_attrs);
- bms_free(modified_attrs);
+ /* modified attrs is passed in and free'd by the caller, or NULL */
bms_free(interesting_attrs);
return TM_Ok;
@@ -4562,7 +4621,8 @@ simple_heap_update(Relation relation, const ItemPointerData *otid, HeapTuple tup
result = heap_update(relation, otid, tup,
GetCurrentCommandId(true), InvalidSnapshot,
true /* wait for commit */ ,
- &tmfd, &lockmode, update_indexes);
+ &tmfd, &lockmode,
+ NULL, false, update_indexes);
switch (result)
{
case TM_SelfModified:
diff --git a/src/backend/access/heap/heapam_handler.c b/src/backend/access/heap/heapam_handler.c
index cbef73e5d4b..2d74fa90c7f 100644
--- a/src/backend/access/heap/heapam_handler.c
+++ b/src/backend/access/heap/heapam_handler.c
@@ -312,12 +312,11 @@ heapam_tuple_delete(Relation relation, ItemPointer tid, CommandId cid,
return heap_delete(relation, tid, cid, crosscheck, wait, tmfd, changingPart);
}
-
static TM_Result
heapam_tuple_update(Relation relation, ItemPointer otid, TupleTableSlot *slot,
CommandId cid, Snapshot snapshot, Snapshot crosscheck,
- bool wait, TM_FailureData *tmfd,
- LockTupleMode *lockmode, TU_UpdateIndexes *update_indexes)
+ bool wait, TM_FailureData *tmfd, LockTupleMode *lockmode,
+ const Bitmapset *modified_attrs, TU_UpdateIndexes *update_indexes)
{
bool shouldFree = true;
HeapTuple tuple = ExecFetchSlotHeapTuple(slot, true, &shouldFree);
@@ -328,7 +327,7 @@ heapam_tuple_update(Relation relation, ItemPointer otid, TupleTableSlot *slot,
tuple->t_tableOid = slot->tts_tableOid;
result = heap_update(relation, otid, tuple, cid, crosscheck, wait,
- tmfd, lockmode, update_indexes);
+ tmfd, lockmode, modified_attrs, true, update_indexes);
ItemPointerCopy(&tuple->t_self, &slot->tts_tid);
/*
diff --git a/src/backend/access/table/tableam.c b/src/backend/access/table/tableam.c
index dfda1af412e..42acd5b17a9 100644
--- a/src/backend/access/table/tableam.c
+++ b/src/backend/access/table/tableam.c
@@ -359,6 +359,7 @@ void
simple_table_tuple_update(Relation rel, ItemPointer otid,
TupleTableSlot *slot,
Snapshot snapshot,
+ const Bitmapset *mix_attrs,
TU_UpdateIndexes *update_indexes)
{
TM_Result result;
@@ -369,7 +370,9 @@ simple_table_tuple_update(Relation rel, ItemPointer otid,
GetCurrentCommandId(true),
snapshot, InvalidSnapshot,
true /* wait for commit */ ,
- &tmfd, &lockmode, update_indexes);
+ &tmfd, &lockmode,
+ mix_attrs,
+ update_indexes);
switch (result)
{
diff --git a/src/backend/commands/trigger.c b/src/backend/commands/trigger.c
index 8df915f63fb..78c063939b0 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,8 @@ ExecBRUpdateTriggers(EState *estate, EPQState *epqstate,
bool should_free_new = false;
TriggerData LocTriggerData = {0};
int i;
- Bitmapset *updatedCols;
+ Bitmapset *updatedCols = NULL;
+ Bitmapset *modifiedCols;
LockTupleMode lockmode;
/* Determine lock mode to use */
@@ -3127,6 +3129,17 @@ 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.
+ */
+ modifiedCols = ExecCompareSlots(relinfo, tupdesc, updatedCols,
+ NULL, oldslot, newslot);
+ relinfo->ri_extraUpdatedCols =
+ bms_add_members(relinfo->ri_extraUpdatedCols, modifiedCols);
+ bms_free(modifiedCols);
+
return true;
}
diff --git a/src/backend/executor/execReplication.c b/src/backend/executor/execReplication.c
index 743b1ee2b28..2122fc33554 100644
--- a/src/backend/executor/execReplication.c
+++ b/src/backend/executor/execReplication.c
@@ -33,6 +33,7 @@
#include "utils/builtins.h"
#include "utils/lsyscache.h"
#include "utils/rel.h"
+#include "utils/relcache.h"
#include "utils/snapmgr.h"
#include "utils/syscache.h"
#include "utils/typcache.h"
@@ -899,6 +900,7 @@ ExecSimpleRelationUpdate(ResultRelInfo *resultRelInfo,
bool skip_tuple = false;
Relation rel = resultRelInfo->ri_RelationDesc;
ItemPointer tid = &(searchslot->tts_tid);
+ Bitmapset *mix_attrs;
/*
* We support only non-system tables, with
@@ -937,8 +939,16 @@ ExecSimpleRelationUpdate(ResultRelInfo *resultRelInfo,
if (rel->rd_rel->relispartition)
ExecPartitionCheck(resultRelInfo, slot, estate, true);
+ mix_attrs = ExecCheckIndexedAttrsForChanges(resultRelInfo,
+ estate, searchslot, slot);
+
+ /*
+ * We're not going to call ExecCheckIndexedAttrsForChanges here
+ * because we've already identified the changes earlier on thanks to
+ * slot_modify_data.
+ */
simple_table_tuple_update(rel, tid, slot, estate->es_snapshot,
- &update_indexes);
+ mix_attrs, &update_indexes);
conflictindexes = resultRelInfo->ri_onConflictArbiterIndexes;
diff --git a/src/backend/executor/execTuples.c b/src/backend/executor/execTuples.c
index b768eae9e53..9ad08b24c2f 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,79 @@ ExecFetchSlotHeapTupleDatum(TupleTableSlot *slot)
return ret;
}
+/*
+ * ExecCompareSlots
+ *
+ * Compare old and new TupleTableSlots to detect which attributes have changed.
+ *
+ * This function serves two purposes:
+ * 1) After trigger execution: detect trigger-modified columns
+ * (pass excluding=explicitly SET columns, including=NULL)
+ * 2) Index maintenance: detect changes in indexed columns
+ * (pass including=indexed AND explicitly SET columns, excluding=NULL)
+ *
+ * Parameters:
+ * resultRelInfo - relation information
+ * excluding - bitmapset of attributes to skip
+ * including - bitmapset of attributes to check
+ * tupdesc - RelationGetDescr(relation)
+ * old_tts - old tuple slot
+ * new_tts - new tuple slot
+ *
+ * If including is NULL, check all attributes EXCEPT those in excluding
+ * If excluding is NULL, check ONLY attributes in including
+ * If both are NULL, check all attributes
+ *
+ * Returns a Bitmapset of attribute indices (using FirstLowInvalidHeapAttributeNumber
+ * convention) that differ between the two slots.
+ */
+Bitmapset *
+ExecCompareSlots(ResultRelInfo *resultRelInfo,
+ TupleDesc tupdesc,
+ const Bitmapset *excluding,
+ const Bitmapset *including,
+ TupleTableSlot *old_tts,
+ TupleTableSlot *new_tts)
+{
+ Bitmapset *modified_attrs = NULL;
+
+ for (int i = 0; i < tupdesc->natts; i++)
+ {
+ AttrNumber attnum = i + 1;
+ AttrNumber attidx = attnum - FirstLowInvalidHeapAttributeNumber;
+ Datum old_value,
+ new_value;
+ bool old_null,
+ new_null;
+ CompactAttribute *att;
+
+ /* Determine whether to check this attribute */
+ if ((!bms_is_empty(including) && !bms_is_member(attidx, including)) ||
+ bms_is_member(attidx, excluding))
+ continue;
+
+ att = TupleDescCompactAttr(tupdesc, attnum - 1);
+ old_value = slot_getattr(old_tts, attnum, &old_null);
+ new_value = slot_getattr(new_tts, attnum, &new_null);
+
+ /* A change to/from NULL, so not equal */
+ if (old_null != new_null)
+ {
+ modified_attrs = bms_add_member(modified_attrs, attidx);
+ continue;
+ }
+
+ /* Both NULL, no change/unmodified */
+ if (old_null)
+ continue;
+
+ if (!datum_image_eq(old_value, new_value, att->attbyval, att->attlen))
+ modified_attrs = bms_add_member(modified_attrs, attidx);
+ }
+
+ return modified_attrs;
+}
+
/* ----------------------------------------------------------------
* convenience initialization routines
* ----------------------------------------------------------------
diff --git a/src/backend/executor/nodeModifyTable.c b/src/backend/executor/nodeModifyTable.c
index 6802fc13e95..afcc3da2835 100644
--- a/src/backend/executor/nodeModifyTable.c
+++ b/src/backend/executor/nodeModifyTable.c
@@ -17,6 +17,7 @@
* ExecModifyTable - retrieve the next tuple from the node
* ExecEndModifyTable - shut down the ModifyTable node
* ExecReScanModifyTable - rescan the ModifyTable node
+ * ExecCheckIndexedAttrsForChanges - find set of updated indexed columns
*
* NOTES
* The ModifyTable node receives input from its outerPlan, which is
@@ -54,6 +55,7 @@
#include "access/htup_details.h"
#include "access/tableam.h"
+#include "access/tupdesc.h"
#include "access/xact.h"
#include "commands/trigger.h"
#include "executor/execPartition.h"
@@ -188,6 +190,44 @@ static TupleTableSlot *ExecMergeNotMatched(ModifyTableContext *context,
ResultRelInfo *resultRelInfo,
bool canSetTag);
+/*
+ * ExecCheckIndexedAttrsForChanges
+ *
+ * Determine which indexes need updating by finding the set of modified indexed
+ * attributes.
+ *
+ * The goal is for the executor to know, ahead of calling into the table AM to
+ * process the update and before calling into the index AM for inserting new
+ * index tuples, which attributes in the new TupleTableSlot, if any, truely
+ * necessitate a new index tuple.
+ *
+ * Returns a Bitmapset of attributes that intersects with indexes which require
+ * a new index tuple.
+ */
+Bitmapset *
+ExecCheckIndexedAttrsForChanges(ResultRelInfo *resultRelInfo,
+ EState *estate,
+ TupleTableSlot *old_tts,
+ TupleTableSlot *new_tts)
+{
+ Relation relation = resultRelInfo->ri_RelationDesc;
+ TupleDesc tupdesc = RelationGetDescr(relation);
+ Bitmapset *upd_attrs = NULL; /* attributes set in UPDATE statement */
+ Bitmapset *mix_attrs;
+
+ /* If no indexes, we're done */
+ if (resultRelInfo->ri_NumIndices == 0)
+ return NULL;
+
+ /* Fetch the set of attributes explicity SET in the UPDATE statement */
+ upd_attrs = ExecGetAllUpdatedCols(resultRelInfo, estate);
+
+ /* Find out if any changed */
+ mix_attrs = ExecCompareSlots(resultRelInfo, tupdesc,
+ NULL, upd_attrs, old_tts, new_tts);
+
+ return mix_attrs;
+}
/*
* Verify that the tuples to be produced by INSERT match the
@@ -2197,14 +2237,17 @@ ExecUpdatePrepareSlot(ResultRelInfo *resultRelInfo,
*/
static TM_Result
ExecUpdateAct(ModifyTableContext *context, ResultRelInfo *resultRelInfo,
- ItemPointer tupleid, HeapTuple oldtuple, TupleTableSlot *slot,
- bool canSetTag, UpdateContext *updateCxt)
+ ItemPointer tupleid, HeapTuple oldtuple, TupleTableSlot *oldSlot,
+ TupleTableSlot *slot, bool canSetTag, UpdateContext *updateCxt)
{
EState *estate = context->estate;
Relation resultRelationDesc = resultRelInfo->ri_RelationDesc;
bool partition_constraint_failed;
TM_Result result;
+ /* The set of modified indexed attributes that trigger new index entries */
+ Bitmapset *mix_attrs = NULL;
+
updateCxt->crossPartUpdate = false;
/*
@@ -2321,7 +2364,16 @@ lreplace:
ExecConstraints(resultRelInfo, slot, estate);
/*
- * replace the heap tuple
+ * Next up we need to find out the set of indexed attributes that have
+ * changed in value and should trigger a new index tuple. We could start
+ * with the set of updated columns via ExecGetUpdatedCols(), but if we do
+ * we will overlook attributes directly modified by heap_modify_tuple()
+ * which are not known to ExecGetUpdatedCols().
+ */
+ mix_attrs = ExecCheckIndexedAttrsForChanges(resultRelInfo, estate, oldSlot, slot);
+
+ /*
+ * Call into the table AM to update the heap tuple.
*
* Note: if es_crosscheck_snapshot isn't InvalidSnapshot, we check that
* the row to be updated is visible to that snapshot, and throw a
@@ -2335,6 +2387,7 @@ lreplace:
estate->es_crosscheck_snapshot,
true /* wait for commit */ ,
&context->tmfd, &updateCxt->lockmode,
+ mix_attrs,
&updateCxt->updateIndexes);
return result;
@@ -2553,8 +2606,9 @@ ExecUpdate(ModifyTableContext *context, ResultRelInfo *resultRelInfo,
*/
redo_act:
lockedtid = *tupleid;
- result = ExecUpdateAct(context, resultRelInfo, tupleid, oldtuple, slot,
- canSetTag, &updateCxt);
+
+ result = ExecUpdateAct(context, resultRelInfo, tupleid, oldtuple, oldSlot,
+ slot, canSetTag, &updateCxt);
/*
* If ExecUpdateAct reports that a cross-partition update was done,
@@ -3404,8 +3458,8 @@ lmerge_matched:
Assert(oldtuple == NULL);
result = ExecUpdateAct(context, resultRelInfo, tupleid,
- NULL, newslot, canSetTag,
- &updateCxt);
+ NULL, resultRelInfo->ri_oldTupleSlot,
+ newslot, canSetTag, &updateCxt);
/*
* As in ExecUpdate(), if ExecUpdateAct() reports that a
@@ -3430,6 +3484,7 @@ lmerge_matched:
tupleid, NULL, newslot);
mtstate->mt_merge_updated += 1;
}
+
break;
case CMD_DELETE:
@@ -4537,7 +4592,7 @@ ExecModifyTable(PlanState *pstate)
* For UPDATE/DELETE/MERGE, fetch the row identity info for the tuple
* to be updated/deleted/merged. For a heap relation, that's a TID;
* otherwise we may have a wholerow junk attr that carries the old
- * tuple in toto. Keep this in step with the part of
+ * tuple in total. Keep this in step with the part of
* ExecInitModifyTable that sets up ri_RowIdAttNo.
*/
if (operation == CMD_UPDATE || operation == CMD_DELETE ||
@@ -4717,6 +4772,7 @@ ExecModifyTable(PlanState *pstate)
/* Now apply the update. */
slot = ExecUpdate(&context, resultRelInfo, tupleid, oldtuple,
oldSlot, slot, node->canSetTag);
+
if (tuplock)
UnlockTuple(resultRelInfo->ri_RelationDesc, tupleid,
InplaceUpdateTupleLock);
diff --git a/src/backend/replication/logical/worker.c b/src/backend/replication/logical/worker.c
index 32725c48623..2d6d9d76f87 100644
--- a/src/backend/replication/logical/worker.c
+++ b/src/backend/replication/logical/worker.c
@@ -285,12 +285,14 @@
#include "storage/procarray.h"
#include "tcop/tcopprot.h"
#include "utils/acl.h"
+#include "utils/datum.h"
#include "utils/guc.h"
#include "utils/inval.h"
#include "utils/lsyscache.h"
#include "utils/memutils.h"
#include "utils/pg_lsn.h"
#include "utils/rel.h"
+#include "utils/relcache.h"
#include "utils/rls.h"
#include "utils/snapmgr.h"
#include "utils/syscache.h"
@@ -2917,7 +2919,6 @@ apply_handle_update_internal(ApplyExecutionData *edata,
TupleTableSlot *localslot = NULL;
ConflictTupleInfo conflicttuple = {0};
bool found;
- MemoryContext oldctx;
EvalPlanQualInit(&epqstate, estate, NULL, NIL, -1, NIL);
ExecOpenIndices(relinfo, false);
@@ -2956,16 +2957,16 @@ apply_handle_update_internal(ApplyExecutionData *edata,
}
/* Process and store remote tuple in the slot */
- oldctx = MemoryContextSwitchTo(GetPerTupleMemoryContext(estate));
slot_modify_data(remoteslot, localslot, relmapentry, newtup);
- MemoryContextSwitchTo(oldctx);
EvalPlanQualSetSlot(&epqstate, remoteslot);
InitConflictIndexes(relinfo);
- /* Do the actual update. */
+ /* First check privileges */
TargetPrivilegesCheck(relinfo->ri_RelationDesc, ACL_UPDATE);
+
+ /* Then do the actual update. */
ExecSimpleRelationUpdate(relinfo, estate, &epqstate, localslot,
remoteslot);
}
@@ -3522,10 +3523,7 @@ apply_handle_tuple_routing(ApplyExecutionData *edata,
* Apply the update to the local tuple, putting the result in
* remoteslot_part.
*/
- oldctx = MemoryContextSwitchTo(GetPerTupleMemoryContext(estate));
- slot_modify_data(remoteslot_part, localslot, part_entry,
- newtup);
- MemoryContextSwitchTo(oldctx);
+ slot_modify_data(remoteslot_part, localslot, part_entry, newtup);
EvalPlanQualInit(&epqstate, estate, NULL, NIL, -1, NIL);
@@ -3549,6 +3547,7 @@ apply_handle_tuple_routing(ApplyExecutionData *edata,
EvalPlanQualSetSlot(&epqstate, remoteslot_part);
TargetPrivilegesCheck(partrelinfo->ri_RelationDesc,
ACL_UPDATE);
+
ExecSimpleRelationUpdate(partrelinfo, estate, &epqstate,
localslot, remoteslot_part);
}
diff --git a/src/backend/utils/cache/relcache.c b/src/backend/utils/cache/relcache.c
index 6b634c9fff1..547cf1d054d 100644
--- a/src/backend/utils/cache/relcache.c
+++ b/src/backend/utils/cache/relcache.c
@@ -2477,6 +2477,7 @@ RelationDestroyRelation(Relation relation, bool remember_tupdesc)
bms_free(relation->rd_idattr);
bms_free(relation->rd_hotblockingattr);
bms_free(relation->rd_summarizedattr);
+ bms_free(relation->rd_indexedattr);
if (relation->rd_pubdesc)
pfree(relation->rd_pubdesc);
if (relation->rd_options)
@@ -5278,6 +5279,7 @@ RelationGetIndexPredicate(Relation relation)
* index (empty if FULL)
* INDEX_ATTR_BITMAP_HOT_BLOCKING Columns that block updates from being HOT
* INDEX_ATTR_BITMAP_SUMMARIZED Columns included in summarizing indexes
+ * INDEX_ATTR_BITMAP_INDEXED Columns referenced by indexes
*
* Attribute numbers are offset by FirstLowInvalidHeapAttributeNumber so that
* we can include system attributes (e.g., OID) in the bitmap representation.
@@ -5301,6 +5303,7 @@ RelationGetIndexAttrBitmap(Relation relation, IndexAttrBitmapKind attrKind)
Bitmapset *pkindexattrs; /* columns in the primary index */
Bitmapset *idindexattrs; /* columns in the replica identity */
Bitmapset *hotblockingattrs; /* columns with HOT blocking indexes */
+ Bitmapset *indexedattrs; /* columns referenced by indexes */
Bitmapset *summarizedattrs; /* columns with summarizing indexes */
List *indexoidlist;
List *newindexoidlist;
@@ -5324,6 +5327,8 @@ RelationGetIndexAttrBitmap(Relation relation, IndexAttrBitmapKind attrKind)
return bms_copy(relation->rd_hotblockingattr);
case INDEX_ATTR_BITMAP_SUMMARIZED:
return bms_copy(relation->rd_summarizedattr);
+ case INDEX_ATTR_BITMAP_INDEXED:
+ return bms_copy(relation->rd_indexedattr);
default:
elog(ERROR, "unknown attrKind %u", attrKind);
}
@@ -5368,6 +5373,7 @@ restart:
idindexattrs = NULL;
hotblockingattrs = NULL;
summarizedattrs = NULL;
+ indexedattrs = NULL;
foreach(l, indexoidlist)
{
Oid indexOid = lfirst_oid(l);
@@ -5500,10 +5506,14 @@ restart:
bms_free(idindexattrs);
bms_free(hotblockingattrs);
bms_free(summarizedattrs);
+ /* indexedattrs not yet initialized */
goto restart;
}
+ /* Set indexed attributes to track all referenced attributes */
+ indexedattrs = bms_union(hotblockingattrs, summarizedattrs);
+
/* Don't leak the old values of these bitmaps, if any */
relation->rd_attrsvalid = false;
bms_free(relation->rd_keyattr);
@@ -5516,6 +5526,8 @@ restart:
relation->rd_hotblockingattr = NULL;
bms_free(relation->rd_summarizedattr);
relation->rd_summarizedattr = NULL;
+ bms_free(relation->rd_indexedattr);
+ relation->rd_indexedattr = NULL;
/*
* Now save copies of the bitmaps in the relcache entry. We intentionally
@@ -5530,6 +5542,7 @@ restart:
relation->rd_idattr = bms_copy(idindexattrs);
relation->rd_hotblockingattr = bms_copy(hotblockingattrs);
relation->rd_summarizedattr = bms_copy(summarizedattrs);
+ relation->rd_indexedattr = bms_copy(indexedattrs);
relation->rd_attrsvalid = true;
MemoryContextSwitchTo(oldcxt);
@@ -5546,6 +5559,8 @@ restart:
return hotblockingattrs;
case INDEX_ATTR_BITMAP_SUMMARIZED:
return summarizedattrs;
+ case INDEX_ATTR_BITMAP_INDEXED:
+ return indexedattrs;
default:
elog(ERROR, "unknown attrKind %u", attrKind);
return NULL;
diff --git a/src/include/access/heapam.h b/src/include/access/heapam.h
index 3c0961ab36b..a56f3d1f378 100644
--- a/src/include/access/heapam.h
+++ b/src/include/access/heapam.h
@@ -368,6 +368,7 @@ extern TM_Result heap_update(Relation relation, const ItemPointerData *otid,
HeapTuple newtup,
CommandId cid, Snapshot crosscheck, bool wait,
TM_FailureData *tmfd, LockTupleMode *lockmode,
+ const Bitmapset *mix_attrs, bool mix_attrs_valid,
TU_UpdateIndexes *update_indexes);
extern TM_Result heap_lock_tuple(Relation relation, HeapTuple tuple,
CommandId cid, LockTupleMode mode, LockWaitPolicy wait_policy,
diff --git a/src/include/access/tableam.h b/src/include/access/tableam.h
index 251379016b0..3b080aa3711 100644
--- a/src/include/access/tableam.h
+++ b/src/include/access/tableam.h
@@ -549,6 +549,7 @@ typedef struct TableAmRoutine
bool wait,
TM_FailureData *tmfd,
LockTupleMode *lockmode,
+ const Bitmapset *updated_cols,
TU_UpdateIndexes *update_indexes);
/* see table_tuple_lock() for reference about parameters */
@@ -1524,12 +1525,12 @@ static inline TM_Result
table_tuple_update(Relation rel, ItemPointer otid, TupleTableSlot *slot,
CommandId cid, Snapshot snapshot, Snapshot crosscheck,
bool wait, TM_FailureData *tmfd, LockTupleMode *lockmode,
- TU_UpdateIndexes *update_indexes)
+ const Bitmapset *mix_cols, TU_UpdateIndexes *update_indexes)
{
return rel->rd_tableam->tuple_update(rel, otid, slot,
cid, snapshot, crosscheck,
- wait, tmfd,
- lockmode, update_indexes);
+ wait, tmfd, lockmode,
+ mix_cols, update_indexes);
}
/*
@@ -2010,6 +2011,7 @@ extern void simple_table_tuple_delete(Relation rel, ItemPointer tid,
Snapshot snapshot);
extern void simple_table_tuple_update(Relation rel, ItemPointer otid,
TupleTableSlot *slot, Snapshot snapshot,
+ const Bitmapset *mix_attrs,
TU_UpdateIndexes *update_indexes);
diff --git a/src/include/catalog/index.h b/src/include/catalog/index.h
index b259c4141ed..14a39beab6e 100644
--- a/src/include/catalog/index.h
+++ b/src/include/catalog/index.h
@@ -132,6 +132,7 @@ extern bool CompareIndexInfo(const IndexInfo *info1, const IndexInfo *info2,
const AttrMap *attmap);
extern void BuildSpeculativeIndexInfo(Relation index, IndexInfo *ii);
+extern void BuildUpdateIndexInfo(ResultRelInfo *resultRelInfo);
extern void FormIndexDatum(IndexInfo *indexInfo,
TupleTableSlot *slot,
diff --git a/src/include/executor/executor.h b/src/include/executor/executor.h
index 55a7d930d26..6e263278a4e 100644
--- a/src/include/executor/executor.h
+++ b/src/include/executor/executor.h
@@ -606,6 +606,12 @@ 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 *ExecCompareSlots(ResultRelInfo *resultRelInfo,
+ TupleDesc tupdesc,
+ const Bitmapset *excluding,
+ const Bitmapset *including,
+ TupleTableSlot *old_tts,
+ TupleTableSlot *new_tts);
typedef struct TupOutputState
{
@@ -800,5 +806,9 @@ extern ResultRelInfo *ExecLookupResultRelByOid(ModifyTableState *node,
Oid resultoid,
bool missing_ok,
bool update_cache);
+extern Bitmapset *ExecCheckIndexedAttrsForChanges(ResultRelInfo *relinfo,
+ EState *estate,
+ TupleTableSlot *old_tts,
+ TupleTableSlot *new_tts);
#endif /* EXECUTOR_H */
diff --git a/src/include/nodes/execnodes.h b/src/include/nodes/execnodes.h
index 63c067d5aae..13284dbd70b 100644
--- a/src/include/nodes/execnodes.h
+++ b/src/include/nodes/execnodes.h
@@ -502,6 +502,12 @@ typedef struct ResultRelInfo
/* true if the above has been computed */
bool ri_extraUpdatedCols_valid;
+ /*
+ * For UPDATE a Bitmapset of the attributes that are both indexed and have
+ * changed in value.
+ */
+ Bitmapset *ri_ChangedIndexedCols;
+
/* Projection to generate new tuple in an INSERT/UPDATE */
ProjectionInfo *ri_projectNew;
/* Slot to hold that tuple */
diff --git a/src/include/utils/rel.h b/src/include/utils/rel.h
index 236830f6b93..df5426fd7fb 100644
--- a/src/include/utils/rel.h
+++ b/src/include/utils/rel.h
@@ -164,6 +164,7 @@ typedef struct RelationData
Bitmapset *rd_idattr; /* included in replica identity index */
Bitmapset *rd_hotblockingattr; /* cols blocking HOT update */
Bitmapset *rd_summarizedattr; /* cols indexed by summarizing indexes */
+ Bitmapset *rd_indexedattr; /* all cols referenced by indexes */
PublicationDesc *rd_pubdesc; /* publication descriptor, or NULL */
diff --git a/src/include/utils/relcache.h b/src/include/utils/relcache.h
index 2700224939a..5834ab7b903 100644
--- a/src/include/utils/relcache.h
+++ b/src/include/utils/relcache.h
@@ -71,6 +71,7 @@ typedef enum IndexAttrBitmapKind
INDEX_ATTR_BITMAP_IDENTITY_KEY,
INDEX_ATTR_BITMAP_HOT_BLOCKING,
INDEX_ATTR_BITMAP_SUMMARIZED,
+ INDEX_ATTR_BITMAP_INDEXED,
} IndexAttrBitmapKind;
extern Bitmapset *RelationGetIndexAttrBitmap(Relation relation,
--
2.51.2
#if 0
if (modified_attrs_valid)
{
bool id_key = false;
bool hdci_id_key_req;
Bitmapset *hdci_attrs = HeapDetermineColumnsInfo(relation, hot_attrs, id_attrs,
&oldtup, newtup, &id_key);
/* hdci_id_key_req = bms_overlap(modified_attrs, hdci_attrs) || id_key; */
/* Assert(bms_equal(modified_attrs, hdci_attrs)); */
/* Assert(rep_id_key_required == hdci_id_key_req); */
Assert(id_key == id_has_external);
}
#endif
#if 0
if (modified_attrs_valid)
{
bool id_key = false;
bool hdci_id_key_req;
Bitmapset *hdci_attrs = HeapDetermineColumnsInfo(relation, hot_attrs, id_attrs,
&oldtup, newtup, &id_key);
hdci_id_key_req = bms_overlap(modified_attrs, hdci_attrs) || id_key;
/* Compare bitmapsets and log differences */
if (!bms_equal(modified_attrs, hdci_attrs))
{
Bitmapset *only_in_modified = bms_difference(modified_attrs, hdci_attrs);
Bitmapset *only_in_hdci = bms_difference(hdci_attrs, modified_attrs);
int attidx = -1;
TupleDesc tupdesc = RelationGetDescr(relation);
elog(WARNING, "Bitmapset mismatch in HeapDetermineColumnsInfo for relation %s",
RelationGetRelationName(relation));
elog(WARNING, " ExecCheckIndexedAttrsForChanges: %s", bmsToString(modified_attrs));
elog(WARNING, " HeapDetermineColumnsInfo: %s", bmsToString(hdci_attrs));
/* Log and compare attributes only in modified_attrs */
attidx = -1;
if (bms_num_members(only_in_modified) > 0)
{
elog(WARNING, " Attributes reported changed by ExecCheckIndexedAttrsForChanges but not HeapDetermineColumnsInfo:");
while ((attidx = bms_next_member(only_in_modified, attidx)) >= 0)
{
AttrNumber attnum = attidx + FirstLowInvalidHeapAttributeNumber;
if (attnum > 0 && attnum <= tupdesc->natts)
{
Form_pg_attribute att = TupleDescAttr(tupdesc, attnum - 1);
Datum old_val,
new_val;
bool old_isnull,
new_isnull;
old_val = heap_getattr(&oldtup, attnum, tupdesc, &old_isnull);
new_val = heap_getattr(newtup, attnum, tupdesc, &new_isnull);
if (old_isnull != new_isnull)
{
elog(WARNING, " %s (attnum %d): NULL status differs (old=%s, new=%s) - CORRECT DETECTION",
NameStr(att->attname), attnum,
old_isnull ? "NULL" : "NOT NULL",
new_isnull ? "NULL" : "NOT NULL");
}
else if (!old_isnull && !new_isnull)
{
if (!datum_image_eq(old_val, new_val, att->attbyval, att->attlen))
{
elog(WARNING, " %s (attnum %d): binary values differ - CORRECT DETECTION",
NameStr(att->attname), attnum);
}
else
{
elog(WARNING, " %s (attnum %d): binary values are IDENTICAL - FALSE POSITIVE (HeapDetermineColumnsInfo is correct)",
NameStr(att->attname), attnum);
}
}
else
{
elog(WARNING, " %s (attnum %d): both NULL - FALSE POSITIVE (HeapDetermineColumnsInfo is correct)",
NameStr(att->attname), attnum);
}
}
}
}
/* Log and compare attributes only in hdci_attrs */
attidx = -1;
if (bms_num_members(only_in_hdci) > 0)
{
elog(WARNING, " Attributes reported changed by HeapDetermineColumnsInfo but not ExecCheckIndexedAttrsForChanges:");
while ((attidx = bms_next_member(only_in_hdci, attidx)) >= 0)
{
AttrNumber attnum = attidx + FirstLowInvalidHeapAttributeNumber;
if (attnum > 0 && attnum <= tupdesc->natts)
{
Form_pg_attribute att = TupleDescAttr(tupdesc, attnum - 1);
Datum old_val,
new_val;
bool old_isnull,
new_isnull;
old_val = heap_getattr(&oldtup, attnum, tupdesc, &old_isnull);
new_val = heap_getattr(newtup, attnum, tupdesc, &new_isnull);
if (old_isnull != new_isnull)
{
elog(WARNING, " %s (attnum %d): NULL status differs (old=%s, new=%s) - CORRECT DETECTION",
NameStr(att->attname), attnum,
old_isnull ? "NULL" : "NOT NULL",
new_isnull ? "NULL" : "NOT NULL");
}
else if (!old_isnull && !new_isnull)
{
if (!datum_image_eq(old_val, new_val, att->attbyval, att->attlen))
{
elog(WARNING, " %s (attnum %d): binary values differ - CORRECT DETECTION",
NameStr(att->attname), attnum);
}
else
{
elog(WARNING, " %s (attnum %d): binary values are IDENTICAL - FALSE NEGATIVE (ExecCheckIndexedAttrsForChanges is correct)",
NameStr(att->attname), attnum);
}
}
else
{
elog(WARNING, " %s (attnum %d): both NULL - FALSE NEGATIVE (ExecCheckIndexedAttrsForChanges is correct)",
NameStr(att->attname), attnum);
}
}
}
}
bms_free(only_in_modified);
bms_free(only_in_hdci);
}
/* Compare replica identity logic */
if (rep_id_key_required != hdci_id_key_req)
{
elog(WARNING, "Replica identity logic mismatch for relation %s",
RelationGetRelationName(relation));
elog(WARNING, " ExecCheckIndexedAttrsForChanges (rep_id_key_required): %s",
rep_id_key_required ? "TRUE" : "FALSE");
elog(WARNING, " HeapDetermineColumnsInfo (hdci_id_key_req): %s",
hdci_id_key_req ? "TRUE" : "FALSE");
}
}
#endif
