From dc412e6ece61f9fc610dd36ab1f2cd538821c338 Mon Sep 17 00:00:00 2001 From: Greg Burd Date: Tue, 3 Nov 2026 11:01:18 -0500 Subject: [PATCH v46 3/9] Give the indexUnchanged hint from the attributes an UPDATE changed When an UPDATE cannot be HOT, ExecInsertIndexTuples() passes indexUnchanged = true to index_insert() for each index the update leaves logically unchanged, a hint that nbtree uses to try bottom-up deletion before it splits a page. index_unchanged_by_update() worked that out from ExecGetUpdatedCols() and ExecGetExtraUpdatedCols(), the columns the statement targets, and cached the answer per statement in the IndexInfo. That is wrong both ways. A column the SET clause names but whose value is written back unchanged, as ORMs that send every column do, takes the hint away from every index on it, which is the version churn bottom-up deletion is for. A column a BEFORE ROW trigger changes without SET naming it gives the hint to an index whose key changed. REPACK (CONCURRENTLY) installed an updatedCols naming every column, so its indexes never got the hint. Take the hint from modified_idx_attrs instead, the indexed attributes whose values this row's update changed, which the caller already has. ExecInsertIndexTuples() gets it as a new argument, NULL for an insert. The rules are the ones index_unchanged_by_update() applied to the targeted columns: key columns count, INCLUDE columns and the predicate do not, and an expression is taken to have changed if any column it uses did. The hint is now decided per row; nbtree only acts on it when practically all inserts on a leaf page get it. ii_CheckedUnchanged and ii_IndexUnchanged are no longer used and go away, as does the updatedCols REPACK built for the hint and nothing else. ExecUpdateAct() keeps the set in the UpdateContext, allocated in the per-tuple memory context, until ExecUpdateEpilogue() has inserted the index entries. contrib/pageinspect's hot_updates test now shows the index whose column SET writes back with its own value staying as small as one SET doesn't name. Discussion: https://postgr.es/m/78574B24-BE0A-42C5-8075-3FA9FA63B8FC@amazon.com --- contrib/pageinspect/expected/hot_updates.out | 2 +- src/backend/catalog/toasting.c | 2 - src/backend/commands/copyfrom.c | 6 +- src/backend/commands/repack.c | 31 +----- src/backend/executor/execIndexing.c | 108 +++++-------------- src/backend/executor/execReplication.c | 8 +- src/backend/executor/nodeModifyTable.c | 32 ++++-- src/backend/nodes/makefuncs.c | 2 - src/include/executor/executor.h | 1 + src/include/nodes/execnodes.h | 4 - 10 files changed, 61 insertions(+), 135 deletions(-) diff --git a/contrib/pageinspect/expected/hot_updates.out b/contrib/pageinspect/expected/hot_updates.out index b7c26b1cb01..d1a05db73ce 100644 --- a/contrib/pageinspect/expected/hot_updates.out +++ b/contrib/pageinspect/expected/hot_updates.out @@ -146,7 +146,7 @@ SELECT pg_relation_size('hint_test_a') = pg_relation_size('hint_test_d') AS same_size; same_size ----------- - f + t (1 row) DROP TABLE hint_test; diff --git a/src/backend/catalog/toasting.c b/src/backend/catalog/toasting.c index 3058ec14223..efc694f9944 100644 --- a/src/backend/catalog/toasting.c +++ b/src/backend/catalog/toasting.c @@ -359,8 +359,6 @@ create_toast_table(Relation rel, Oid toastOid, Oid toastIndexOid, indexInfo->ii_Unique = true; indexInfo->ii_NullsNotDistinct = false; indexInfo->ii_ReadyForInserts = true; - indexInfo->ii_CheckedUnchanged = false; - indexInfo->ii_IndexUnchanged = false; indexInfo->ii_Concurrent = false; indexInfo->ii_BrokenHotChain = false; indexInfo->ii_ParallelWorkers = 0; diff --git a/src/backend/commands/copyfrom.c b/src/backend/commands/copyfrom.c index 89f48c8951b..b3eb6a9a400 100644 --- a/src/backend/commands/copyfrom.c +++ b/src/backend/commands/copyfrom.c @@ -578,7 +578,7 @@ CopyMultiInsertBufferFlush(CopyMultiInsertInfo *miinfo, recheckIndexes = ExecInsertIndexTuples(resultRelInfo, estate, 0, buffer->slots[i], - NIL, NULL); + NULL, NIL, NULL); ExecARInsertTriggers(estate, resultRelInfo, slots[i], NULL, recheckIndexes, cstate->transition_capture); @@ -1435,8 +1435,8 @@ CopyFrom(CopyFromState cstate) if (resultRelInfo->ri_NumIndices > 0) recheckIndexes = ExecInsertIndexTuples(resultRelInfo, estate, 0, - myslot, NIL, - NULL); + myslot, NULL, + NIL, NULL); } /* AFTER ROW INSERT Triggers */ diff --git a/src/backend/commands/repack.c b/src/backend/commands/repack.c index 21ec65e74cb..350e843ef02 100644 --- a/src/backend/commands/repack.c +++ b/src/backend/commands/repack.c @@ -2898,7 +2898,7 @@ apply_concurrent_insert(Relation rel, TupleTableSlot *slot, chgcxt->cc_estate, 0, slot, - NIL, NULL); + NULL, NIL, NULL); pgstat_progress_incr_param(PROGRESS_REPACK_HEAP_TUPLES_INSERTED, 1); } @@ -2932,7 +2932,6 @@ apply_concurrent_update(Relation rel, TupleTableSlot *spilled_tuple, false, &tmfd, &lockmode, modified_idx_attrs, &update_indexes); - bms_free(modified_idx_attrs); if (res != TM_Ok) ereport(ERROR, errcode(ERRCODE_T_R_SERIALIZATION_FAILURE), @@ -2949,8 +2948,10 @@ apply_concurrent_update(Relation rel, TupleTableSlot *spilled_tuple, chgcxt->cc_estate, flags, spilled_tuple, + modified_idx_attrs, NIL, NULL); } + bms_free(modified_idx_attrs); pgstat_progress_incr_param(PROGRESS_REPACK_HEAP_TUPLES_UPDATED, 1); } @@ -3283,10 +3284,7 @@ initialize_change_context(ChangeContext *chgcxt, */ { RangeTblEntry *rte; - TupleDesc desc = RelationGetDescr(tgt_relation); List *perminfos = NIL; - Bitmapset *updatedCols = NULL; - RTEPermissionInfo *perminfo; /* * For our use, the RTE only needs to have perminfoindex initialized, @@ -3300,29 +3298,6 @@ initialize_change_context(ChangeContext *chgcxt, /* Create the RTEPermissionInfo instance (and set ->perminfoindex). */ addRTEPermissionInfo(&perminfos, rte); - /* - * Initialize updatedCols to show that all columns are updated. This - * is of course not necessarily true, and we cannot know this early; - * but this is only used by ExecInsertIndexTuples to flag index - * updates with no logical value changes, so if it's wrong, nothing - * terribly bad happens. We may want to improve this someday though. - * - * Don't claim that dropped columns are changed though. - */ - for (int i = 0; i < desc->natts; i++) - { - CompactAttribute *attr = TupleDescCompactAttr(desc, i); - - if (attr->attisdropped) - continue; - updatedCols = bms_add_member(updatedCols, - i + 1 - FirstLowInvalidHeapAttributeNumber); - } - - /* install updatedCols in the right place */ - perminfo = getRTEPermissionInfo(perminfos, rte); - perminfo->updatedCols = updatedCols; - /* finally we can initialize the range table proper */ ExecInitRangeTable(chgcxt->cc_estate, list_make1(rte), perminfos, bms_make_singleton(1)); diff --git a/src/backend/executor/execIndexing.c b/src/backend/executor/execIndexing.c index 02ca46b5cb3..c5d215eb3e0 100644 --- a/src/backend/executor/execIndexing.c +++ b/src/backend/executor/execIndexing.c @@ -140,11 +140,10 @@ static bool check_exclusion_or_unique_constraint(Relation heap, Relation index, static bool index_recheck_constraint(Relation index, const Oid *constr_procs, const Datum *existing_values, const bool *existing_isnull, const Datum *new_values); -static bool index_unchanged_by_update(ResultRelInfo *resultRelInfo, - EState *estate, IndexInfo *indexInfo, - Relation indexRelation); +static bool index_unchanged_by_update(IndexInfo *indexInfo, + const Bitmapset *modified_idx_attrs); static bool index_expression_changed_walker(Node *node, - Bitmapset *allUpdatedCols); + Bitmapset *modified_idx_attrs); static void ExecWithoutOverlapsNotEmpty(Relation rel, NameData attname, Datum attval, char typtype, Oid atttypid); @@ -284,9 +283,11 @@ ExecCloseIndices(ResultRelInfo *resultRelInfo) * 'update_indexes' to TU_All). Receiving this hint makes * us consider if we should pass down the 'indexUnchanged' * hint in turn. That's something that we figure out for - * each index_insert() call iff EIIT_IS_UPDATE is set. - * (When that flag is not set we already know not to pass the - * hint to any index.) + * each index_insert() call iff EIIT_IS_UPDATE is set, from + * 'modified_idx_attrs', the indexed attributes the update + * changed (see ExecUpdateModifiedIdxAttrs()). (When that + * flag is not set we already know not to pass the hint to + * any index, and 'modified_idx_attrs' is ignored.) * * If EIIT_ONLY_SUMMARIZING is set, an equivalent optimization to * HOT has been applied and any updated columns are indexed @@ -312,6 +313,7 @@ ExecInsertIndexTuples(ResultRelInfo *resultRelInfo, EState *estate, uint32 flags, TupleTableSlot *slot, + const Bitmapset *modified_idx_attrs, List *arbiterIndexes, bool *specConflict) { @@ -441,10 +443,8 @@ ExecInsertIndexTuples(ResultRelInfo *resultRelInfo, * consider if the 'indexUnchanged' = true hint should be passed. */ indexUnchanged = ((flags & EIIT_IS_UPDATE) && - index_unchanged_by_update(resultRelInfo, - estate, - indexInfo, - indexRelation)); + index_unchanged_by_update(indexInfo, + modified_idx_attrs)); satisfiesConstraint = index_insert(indexRelation, /* index relation */ @@ -1039,38 +1039,23 @@ index_recheck_constraint(Relation index, const Oid *constr_procs, * * When the executor performs an UPDATE that requires a new round of index * tuples, determine if we should pass 'indexUnchanged' = true hint for one - * single index. + * single index: whether the update changed none of the attributes that the + * index's keys are formed from. modified_idx_attrs are the indexed + * attributes that this row's update changed. */ static bool -index_unchanged_by_update(ResultRelInfo *resultRelInfo, EState *estate, - IndexInfo *indexInfo, Relation indexRelation) +index_unchanged_by_update(IndexInfo *indexInfo, + const Bitmapset *modified_idx_attrs) { - Bitmapset *updatedCols; - Bitmapset *extraUpdatedCols; - Bitmapset *allUpdatedCols; bool hasexpression = false; - List *idxExprs; /* - * Check cache first - */ - if (indexInfo->ii_CheckedUnchanged) - return indexInfo->ii_IndexUnchanged; - indexInfo->ii_CheckedUnchanged = true; - - /* - * Check for indexed attribute overlap with updated columns. + * Check for indexed attribute overlap with modified attributes. * * Only do this for key columns. A change to a non-key column within an * INCLUDE index should not be counted here. Non-key column values are * opaque payload state to the index AM, a little like an extra table TID. - * - * Note that row-level BEFORE triggers won't affect our behavior, since - * they don't affect the updatedCols bitmaps generally. It doesn't seem - * worth the trouble of checking which attributes were changed directly. */ - updatedCols = ExecGetUpdatedCols(resultRelInfo, estate); - extraUpdatedCols = ExecGetExtraUpdatedCols(resultRelInfo, estate); for (int attr = 0; attr < indexInfo->ii_NumIndexKeyAttrs; attr++) { int keycol = indexInfo->ii_IndexAttrNumbers[attr]; @@ -1086,59 +1071,21 @@ index_unchanged_by_update(ResultRelInfo *resultRelInfo, EState *estate, } if (bms_is_member(keycol - FirstLowInvalidHeapAttributeNumber, - updatedCols) || - bms_is_member(keycol - FirstLowInvalidHeapAttributeNumber, - extraUpdatedCols)) + modified_idx_attrs)) { /* Changed key column -- don't hint for this index */ - indexInfo->ii_IndexUnchanged = false; return false; } } /* - * When we get this far and index has no expressions, return true so that - * index_insert() call will go on to pass 'indexUnchanged' = true hint. - * - * The _absence_ of an indexed key attribute that overlaps with updated - * attributes (in addition to the total absence of indexed expressions) - * shows that the index as a whole is logically unchanged by UPDATE. + * An indexed expression is taken to have changed if any column it uses + * changed. (INCLUDE columns cannot be expressions.) */ - if (!hasexpression) - { - indexInfo->ii_IndexUnchanged = true; - return true; - } - - /* - * Need to pass only one bms to expression_tree_walker helper function. - * Avoid allocating memory in common case where there are no extra cols. - */ - if (!extraUpdatedCols) - allUpdatedCols = updatedCols; - else - allUpdatedCols = bms_union(updatedCols, extraUpdatedCols); - - /* - * We have to work slightly harder in the event of indexed expressions, - * but the principle is the same as before: try to find columns (Vars, - * actually) that overlap with known-updated columns. - * - * If we find any matching Vars, don't pass hint for index. Otherwise - * pass hint. - */ - idxExprs = RelationGetIndexExpressions(indexRelation); - hasexpression = index_expression_changed_walker((Node *) idxExprs, - allUpdatedCols); - list_free(idxExprs); - if (extraUpdatedCols) - bms_free(allUpdatedCols); - - if (hasexpression) - { - indexInfo->ii_IndexUnchanged = false; + if (hasexpression && + index_expression_changed_walker((Node *) indexInfo->ii_Expressions, + (Bitmapset *) modified_idx_attrs)) return false; - } /* * Deliberately don't consider index predicates. We should even give the @@ -1146,17 +1093,16 @@ index_unchanged_by_update(ResultRelInfo *resultRelInfo, EState *estate, * tuple, which is possible with a partial index (provided the usual * conditions are met). */ - indexInfo->ii_IndexUnchanged = true; return true; } /* * Indexed expression helper for index_unchanged_by_update(). * - * Returns true when Var that appears within allUpdatedCols located. + * Returns true when Var that appears within modified_idx_attrs located. */ static bool -index_expression_changed_walker(Node *node, Bitmapset *allUpdatedCols) +index_expression_changed_walker(Node *node, Bitmapset *modified_idx_attrs) { if (node == NULL) return false; @@ -1166,7 +1112,7 @@ index_expression_changed_walker(Node *node, Bitmapset *allUpdatedCols) Var *var = (Var *) node; if (bms_is_member(var->varattno - FirstLowInvalidHeapAttributeNumber, - allUpdatedCols)) + modified_idx_attrs)) { /* Var was updated -- indicates that we should not hint */ return true; @@ -1177,7 +1123,7 @@ index_expression_changed_walker(Node *node, Bitmapset *allUpdatedCols) } return expression_tree_walker(node, index_expression_changed_walker, - allUpdatedCols); + modified_idx_attrs); } /* diff --git a/src/backend/executor/execReplication.c b/src/backend/executor/execReplication.c index bd3ea224e3c..7a69febe55a 100644 --- a/src/backend/executor/execReplication.c +++ b/src/backend/executor/execReplication.c @@ -939,7 +939,8 @@ ExecSimpleRelationInsert(ResultRelInfo *resultRelInfo, flags = 0; recheckIndexes = ExecInsertIndexTuples(resultRelInfo, estate, flags, - slot, conflictindexes, + slot, NULL, + conflictindexes, &conflict); } @@ -1034,7 +1035,6 @@ ExecSimpleRelationUpdate(ResultRelInfo *resultRelInfo, simple_table_tuple_update_ext(rel, tid, slot, estate->es_snapshot, modified_idx_attrs, &update_indexes); - bms_free(modified_idx_attrs); conflictindexes = resultRelInfo->ri_onConflictArbiterIndexes; @@ -1048,9 +1048,11 @@ ExecSimpleRelationUpdate(ResultRelInfo *resultRelInfo, flags |= EIIT_ONLY_SUMMARIZING; recheckIndexes = ExecInsertIndexTuples(resultRelInfo, estate, flags, - slot, conflictindexes, + slot, modified_idx_attrs, + conflictindexes, &conflict); } + bms_free(modified_idx_attrs); /* * Refer to the comments above the call to CheckAndReportConflict() in diff --git a/src/backend/executor/nodeModifyTable.c b/src/backend/executor/nodeModifyTable.c index c0d1ece7fd2..33ba9be20db 100644 --- a/src/backend/executor/nodeModifyTable.c +++ b/src/backend/executor/nodeModifyTable.c @@ -131,6 +131,13 @@ typedef struct UpdateContext bool crossPartUpdate; /* was it a cross-partition update? */ TU_UpdateIndexes updateIndexes; /* Which index updates are required? */ + /* + * The indexed attributes the update changed, for the indexUnchanged hint + * when inserting the new version's index entries. In the per-tuple + * memory context. + */ + Bitmapset *modifiedIdxAttrs; + /* * Lock mode to acquire on the latest tuple version before performing * EvalPlanQual on it @@ -200,7 +207,8 @@ static TupleTableSlot *ExecMergeNotMatched(ModifyTableContext *context, * * Return the set of indexed attributes whose values differ between old_tts * and new_tts, using the FirstLowInvalidHeapAttributeNumber convention. The - * result describes this one pair of tuples; the caller frees it. + * result describes this one pair of tuples, and is allocated in the caller's + * memory context. * * All indexed attributes are compared, not just those the UPDATE targets. * ExecGetAllUpdatedCols() names the columns an UPDATE targets, not the values @@ -1265,7 +1273,7 @@ ExecInsert(ModifyTableContext *context, /* insert index entries for tuple */ recheckIndexes = ExecInsertIndexTuples(resultRelInfo, estate, EIIT_NO_DUPE_ERROR, - slot, arbiterIndexes, + slot, NULL, arbiterIndexes, &specConflict); /* adjust the tuple's state accordingly */ @@ -1304,7 +1312,7 @@ ExecInsert(ModifyTableContext *context, /* insert index entries for tuple */ if (resultRelInfo->ri_NumIndices > 0) recheckIndexes = ExecInsertIndexTuples(resultRelInfo, estate, - 0, slot, NIL, + 0, slot, NULL, NIL, NULL); } } @@ -2246,9 +2254,7 @@ ExecUpdateAct(ModifyTableContext *context, ResultRelInfo *resultRelInfo, Relation resultRelationDesc = resultRelInfo->ri_RelationDesc; bool partition_constraint_failed; TM_Result result; - - /* The set of modified indexed attributes that trigger new index entries */ - Bitmapset *modified_idx_attrs = NULL; + MemoryContext oldcxt; updateCxt->crossPartUpdate = false; @@ -2365,7 +2371,11 @@ lreplace: if (resultRelationDesc->rd_att->constr) ExecConstraints(resultRelInfo, slot, estate); - modified_idx_attrs = ExecUpdateModifiedIdxAttrs(resultRelInfo, oldSlot, slot); + /* kept until ExecUpdateEpilogue() has inserted the index entries */ + oldcxt = MemoryContextSwitchTo(GetPerTupleMemoryContext(estate)); + updateCxt->modifiedIdxAttrs = ExecUpdateModifiedIdxAttrs(resultRelInfo, + oldSlot, slot); + MemoryContextSwitchTo(oldcxt); /* * Call into the table AM to update the heap tuple. @@ -2383,9 +2393,8 @@ lreplace: estate->es_crosscheck_snapshot, true /* wait for commit */ , &context->tmfd, &updateCxt->lockmode, - modified_idx_attrs, + updateCxt->modifiedIdxAttrs, &updateCxt->updateIndexes); - bms_free(modified_idx_attrs); return result; } @@ -2412,8 +2421,9 @@ ExecUpdateEpilogue(ModifyTableContext *context, UpdateContext *updateCxt, if (updateCxt->updateIndexes == TU_Summarizing) flags |= EIIT_ONLY_SUMMARIZING; recheckIndexes = ExecInsertIndexTuples(resultRelInfo, context->estate, - flags, slot, NIL, - NULL); + flags, slot, + updateCxt->modifiedIdxAttrs, + NIL, NULL); } /* AFTER ROW UPDATE Triggers */ diff --git a/src/backend/nodes/makefuncs.c b/src/backend/nodes/makefuncs.c index cdc02b274ff..80d1fd17cfa 100644 --- a/src/backend/nodes/makefuncs.c +++ b/src/backend/nodes/makefuncs.c @@ -845,8 +845,6 @@ makeIndexInfo(int numattrs, int numkeyattrs, Oid amoid, List *expressions, n->ii_Unique = unique; n->ii_NullsNotDistinct = nulls_not_distinct; n->ii_ReadyForInserts = isready; - n->ii_CheckedUnchanged = false; - n->ii_IndexUnchanged = false; n->ii_Concurrent = concurrent; n->ii_Summarizing = summarizing; n->ii_WithoutOverlaps = withoutoverlaps; diff --git a/src/include/executor/executor.h b/src/include/executor/executor.h index a249c8261ed..eab946f7ec0 100644 --- a/src/include/executor/executor.h +++ b/src/include/executor/executor.h @@ -758,6 +758,7 @@ extern void ExecCloseIndices(ResultRelInfo *resultRelInfo); #define EIIT_ONLY_SUMMARIZING (1<<2) extern List *ExecInsertIndexTuples(ResultRelInfo *resultRelInfo, EState *estate, uint32 flags, TupleTableSlot *slot, + const Bitmapset *modified_idx_attrs, List *arbiterIndexes, bool *specConflict); extern bool ExecCheckIndexConstraints(ResultRelInfo *resultRelInfo, diff --git a/src/include/nodes/execnodes.h b/src/include/nodes/execnodes.h index 2800eec067a..892948da9a0 100644 --- a/src/include/nodes/execnodes.h +++ b/src/include/nodes/execnodes.h @@ -215,10 +215,6 @@ typedef struct IndexInfo bool ii_NullsNotDistinct; /* is it valid for inserts? */ bool ii_ReadyForInserts; - /* IndexUnchanged status determined yet? */ - bool ii_CheckedUnchanged; - /* aminsert hint, cached for retail inserts */ - bool ii_IndexUnchanged; /* are we doing a concurrent index build? */ bool ii_Concurrent; /* did we detect any broken HOT chains? */ -- 2.50.1