v5-0002-review-comments-and-tweaks.patch

From 944e318a1d00b1875d068b18b4b45743f2888e11 Mon Sep 17 00:00:00 2001
From: Tomas Vondra <tomas@2ndquadrant.com>
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 <structfield>amsummarizing</structfield> 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
-   <acronym>BRIN</acronym>), may allow the <acronym>HOT</acronym> optimization
+   Access methods that do not point to individual tuples, but to block ranges
+   (like <acronym>BRIN</acronym>), may allow the <acronym>HOT</acronym> optimization
    to continue. This does not apply to attributes referenced in index
    predicates, an update of such attribute always disables <acronym>HOT</acronym>.
   </para>
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