From 944e318a1d00b1875d068b18b4b45743f2888e11 Mon Sep 17 00:00:00 2001 From: Tomas Vondra Date: Tue, 14 Mar 2023 14:36:30 +0100 Subject: [PATCH v5 2/2] review comments and tweaks --- doc/src/sgml/indexam.sgml | 4 ++-- src/backend/access/heap/heapam_handler.c | 4 ++-- src/backend/catalog/indexing.c | 15 +++++++++++++-- src/backend/executor/execReplication.c | 4 ++-- src/backend/executor/nodeModifyTable.c | 4 ++-- src/backend/nodes/makefuncs.c | 2 +- src/backend/utils/cache/relcache.c | 12 +++++++++--- src/include/access/tableam.h | 8 +++++++- src/include/nodes/execnodes.h | 2 +- src/test/regress/sql/stats.sql | 1 + 10 files changed, 40 insertions(+), 16 deletions(-) diff --git a/doc/src/sgml/indexam.sgml b/doc/src/sgml/indexam.sgml index 897419ec95..29ece7c42e 100644 --- a/doc/src/sgml/indexam.sgml +++ b/doc/src/sgml/indexam.sgml @@ -254,8 +254,8 @@ typedef struct IndexAmRoutine The amsummarizing flag indicates whether the access method summarizes the indexed tuples, with summarizing granularity of at least per block. - Access methods that do not point to individual tuples, but to (like - BRIN), may allow the HOT optimization + Access methods that do not point to individual tuples, but to block ranges + (like BRIN), may allow the HOT optimization to continue. This does not apply to attributes referenced in index predicates, an update of such attribute always disables HOT. diff --git a/src/backend/access/heap/heapam_handler.c b/src/backend/access/heap/heapam_handler.c index a1d7d91ff7..1ce7c6b971 100644 --- a/src/backend/access/heap/heapam_handler.c +++ b/src/backend/access/heap/heapam_handler.c @@ -346,8 +346,8 @@ heapam_tuple_update(Relation relation, ItemPointer otid, TupleTableSlot *slot, else if (!HeapTupleIsHeapOnly(tuple)) Assert(*update_indexes == TU_All); else - Assert(*update_indexes == TU_Summarizing || - *update_indexes == TU_None); + Assert((*update_indexes == TU_Summarizing) || + (*update_indexes == TU_None)); if (shouldFree) pfree(tuple); diff --git a/src/backend/catalog/indexing.c b/src/backend/catalog/indexing.c index a387eccdc4..f00e077cc2 100644 --- a/src/backend/catalog/indexing.c +++ b/src/backend/catalog/indexing.c @@ -83,7 +83,7 @@ CatalogIndexInsert(CatalogIndexState indstate, HeapTuple heapTuple, IndexInfo **indexInfoArray; Datum values[INDEX_MAX_KEYS]; bool isnull[INDEX_MAX_KEYS]; - bool onlySummarized = updateIndexes == TU_Summarizing; + bool onlySummarized = (updateIndexes == TU_Summarizing); /* * HOT update does not require index inserts. But with asserts enabled we @@ -95,6 +95,14 @@ CatalogIndexInsert(CatalogIndexState indstate, HeapTuple heapTuple, return; #endif + /* XXX This is a bit weird way to make a conditional assert. Maybe it'd be + * better to write it like this: + * + * Assert(!(onlySummarized && !HeapTupleIsHeapOnly(heapTuple))); + * + * With a comment "When only updating summarized indexes, it has to be + * a HOT-only tuple" as explanation. + */ if (onlySummarized) Assert(HeapTupleIsHeapOnly(heapTuple)); @@ -149,7 +157,10 @@ CatalogIndexInsert(CatalogIndexState indstate, HeapTuple heapTuple, /* * Skip insertions into non-summarizing indexes if we only need - * to update summarizing indexes + * to update summarizing indexes. + * + * XXX I wonder if we could create a BRIN index on a catalog, so + * that this actually triggers during testing. */ if (onlySummarized && !indexInfo->ii_Summarizing) continue; diff --git a/src/backend/executor/execReplication.c b/src/backend/executor/execReplication.c index 36196e4d94..c0a20a015b 100644 --- a/src/backend/executor/execReplication.c +++ b/src/backend/executor/execReplication.c @@ -510,11 +510,11 @@ ExecSimpleRelationUpdate(ResultRelInfo *resultRelInfo, simple_table_tuple_update(rel, tid, slot, estate->es_snapshot, &update_indexes); - if (resultRelInfo->ri_NumIndices > 0 && update_indexes != TU_None) + if (resultRelInfo->ri_NumIndices > 0 && (update_indexes != TU_None)) recheckIndexes = ExecInsertIndexTuples(resultRelInfo, slot, estate, true, false, NULL, NIL, - update_indexes == TU_Summarizing); + (update_indexes == TU_Summarizing)); /* AFTER ROW UPDATE Triggers */ ExecARUpdateTriggers(estate, resultRelInfo, diff --git a/src/backend/executor/nodeModifyTable.c b/src/backend/executor/nodeModifyTable.c index 3d0efebacc..3a67389508 100644 --- a/src/backend/executor/nodeModifyTable.c +++ b/src/backend/executor/nodeModifyTable.c @@ -2120,12 +2120,12 @@ ExecUpdateEpilogue(ModifyTableContext *context, UpdateContext *updateCxt, List *recheckIndexes = NIL; /* insert index entries for tuple if necessary */ - if (resultRelInfo->ri_NumIndices > 0 && updateCxt->updateIndexes != TU_None) + if (resultRelInfo->ri_NumIndices > 0 && (updateCxt->updateIndexes != TU_None)) recheckIndexes = ExecInsertIndexTuples(resultRelInfo, slot, context->estate, true, false, NULL, NIL, - updateCxt->updateIndexes == TU_Summarizing); + (updateCxt->updateIndexes == TU_Summarizing)); /* AFTER ROW UPDATE Triggers */ ExecARUpdateTriggers(context->estate, resultRelInfo, diff --git a/src/backend/nodes/makefuncs.c b/src/backend/nodes/makefuncs.c index f23f8b7349..216383ca23 100644 --- a/src/backend/nodes/makefuncs.c +++ b/src/backend/nodes/makefuncs.c @@ -761,7 +761,7 @@ makeIndexInfo(int numattrs, int numkeyattrs, Oid amoid, List *expressions, n->ii_Summarizing = summarizing; /* summarizing indexes cannot contain non-key attributes */ - Assert(!summarizing || numkeyattrs == numattrs); + Assert(!summarizing || (numkeyattrs == numattrs)); /* expressions */ n->ii_Expressions = expressions; diff --git a/src/backend/utils/cache/relcache.c b/src/backend/utils/cache/relcache.c index cd0f6e2a5e..fc8cc5b5de 100644 --- a/src/backend/utils/cache/relcache.c +++ b/src/backend/utils/cache/relcache.c @@ -5289,6 +5289,11 @@ restart: /* Is this index the configured (or default) replica identity? */ isIDKey = (indexOid == relreplindex); + /* + * If the index is summarizing, it doesn't block HOT updates, but we + * may still need to update it (if the attributes were modified). So + * decide which bitmap we'll update in the following loop. + */ if (indexDesc->rd_indam->amsummarizing) attrs = &summarizedattrs; else @@ -5305,6 +5310,9 @@ restart: * hotblockingattrs, since they are in index, and HOT-update * shouldn't miss them. * + * XXX This is misleading, because it talks about hotblockingattrs + * but then it adds stuff into attrs. + * * Summarizing indexes do not block HOT, but do need to be updated * when the column value changes, thus require a separate * attribute bitmapset. @@ -5335,9 +5343,7 @@ restart: /* Collect all attributes used in expressions, too */ pull_varattnos(indexExpressions, 1, attrs); - /* - * Collect all attributes in the index predicate, too. - */ + /* Collect all attributes in the index predicate, too */ pull_varattnos(indexPredicate, 1, attrs); index_close(indexDesc, AccessShareLock); diff --git a/src/include/access/tableam.h b/src/include/access/tableam.h index f31d7693ec..36c5835628 100644 --- a/src/include/access/tableam.h +++ b/src/include/access/tableam.h @@ -105,12 +105,18 @@ typedef enum TM_Result /* * Result codes for table_update(..., update_indexes*..). * Used to determine which indexes to update. + * + * XXX Why do we assign explicit values to the members, instead of just letting + * it up to the enum (just like for TM_Result)? */ -typedef enum TU_UpdateIndexes { +typedef enum TU_UpdateIndexes +{ /* No indexed columns were updated (incl. TID addressing of tuple) */ TU_None = 0, + /* A non-summarizing indexed column was updated, or the TID has changed */ TU_All = 1, + /* Only summarized columns were updated, TID is unchanged */ TU_Summarizing = 2 } TU_UpdateIndexes; diff --git a/src/include/nodes/execnodes.h b/src/include/nodes/execnodes.h index ae3608cd93..d97f5a8e7d 100644 --- a/src/include/nodes/execnodes.h +++ b/src/include/nodes/execnodes.h @@ -161,7 +161,7 @@ typedef struct ExprState * IndexUnchanged aminsert hint, cached for retail inserts * Concurrent are we doing a concurrent index build? * BrokenHotChain did we detect any broken HOT chains? - * Summarizing is it summarizing? + * Summarizing is it a summarizing index? * ParallelWorkers # of workers requested (excludes leader) * Am Oid of index AM * AmCache private cache area for index AM diff --git a/src/test/regress/sql/stats.sql b/src/test/regress/sql/stats.sql index c8caa4db38..8b946d05cc 100644 --- a/src/test/regress/sql/stats.sql +++ b/src/test/regress/sql/stats.sql @@ -535,6 +535,7 @@ SET enable_seqscan TO on; SELECT pg_stat_get_replication_slot(NULL); SELECT pg_stat_get_subscription_stats(NULL); + -- Test that the following operations are tracked in pg_stat_io: -- - reads of target blocks into shared buffers -- - writes of shared buffers to permanent storage -- 2.39.2