Re-allow testing of GiST buffered builds.

Commit 16fa9b2b3 broke the ability to reliably test GiST buffered builds,
because it caused sorted builds to be done instead if sortsupport is
available, regardless of any attempt to override that.  While a would-be
test case could try to work around that by choosing an opclass that has
no sortsupport function, coverage would be silently lost the moment
someone decides it'd be a good idea to add a sortsupport function.

Hence, rearrange the logic in gistbuild() so that if "buffering = on"
is specified in CREATE INDEX, we will use that method, sortsupport or no.

Also document the interaction between sorting and the buffering
parameter, as 16fa9b2b3 failed to do.

(Note that in fact we still lack any test coverage of buffered builds,
but this is a prerequisite to adding a non-fragile test.)

Discussion: https://postgr.es/m/3249980.1602532990@sss.pgh.pa.us

1 files changed, 32 insertions, 28 deletions

diff --git a/src/backend/access/gist/gistbuild.c b/src/backend/access/gist/gistbuild.c
index 28bc5855ad9..9d3fa9c3b75 100644
--- a/src/backend/access/gist/gistbuild.c
+++ b/src/backend/access/gist/gistbuild.c
@@ -180,9 +180,7 @@ gistbuild(Relation heap, Relation index, IndexInfo *indexInfo)
 	MemoryContext oldcxt = CurrentMemoryContext;
 	int			fillfactor;
 	Oid			SortSupportFnOids[INDEX_MAX_KEYS];
-	bool		hasallsortsupports;
-	int			keyscount = IndexRelationGetNumberOfKeyAttributes(index);
-	GiSTOptions *options = NULL;
+	GiSTOptions *options = (GiSTOptions *) index->rd_options;
 
 	/*
 	 * We expect to be called exactly once for any index relation. If that's
@@ -192,9 +190,6 @@ gistbuild(Relation heap, Relation index, IndexInfo *indexInfo)
 		elog(ERROR, "index \"%s\" already contains data",
 			 RelationGetRelationName(index));
 
-	if (index->rd_options)
-		options = (GiSTOptions *) index->rd_options;
-
 	buildstate.indexrel = index;
 	buildstate.heaprel = heap;
 	buildstate.sortstate = NULL;
@@ -208,33 +203,17 @@ gistbuild(Relation heap, Relation index, IndexInfo *indexInfo)
 	buildstate.giststate->tempCxt = createTempGistContext();
 
 	/*
-	 * Choose build strategy. If all keys support sorting, do that. Otherwise
-	 * the default strategy is switch to buffering mode when the index grows
-	 * too large to fit in cache.
+	 * Choose build strategy.  First check whether the user specified to use
+	 * buffering mode.  (The use-case for that in the field is somewhat
+	 * questionable perhaps, but it's important for testing purposes.)
 	 */
-	hasallsortsupports = true;
-	for (int i = 0; i < keyscount; i++)
-	{
-		SortSupportFnOids[i] = index_getprocid(index, i + 1,
-											   GIST_SORTSUPPORT_PROC);
-		if (!OidIsValid(SortSupportFnOids[i]))
-		{
-			hasallsortsupports = false;
-			break;
-		}
-	}
-
-	if (hasallsortsupports)
-	{
-		buildstate.buildMode = GIST_SORTED_BUILD;
-	}
-	else if (options)
+	if (options)
 	{
 		if (options->buffering_mode == GIST_OPTION_BUFFERING_ON)
 			buildstate.buildMode = GIST_BUFFERING_STATS;
 		else if (options->buffering_mode == GIST_OPTION_BUFFERING_OFF)
 			buildstate.buildMode = GIST_BUFFERING_DISABLED;
-		else
+		else					/* must be "auto" */
 			buildstate.buildMode = GIST_BUFFERING_AUTO;
 	}
 	else
@@ -243,6 +222,28 @@ gistbuild(Relation heap, Relation index, IndexInfo *indexInfo)
 	}
 
 	/*
+	 * Unless buffering mode was forced, see if we can use sorting instead.
+	 */
+	if (buildstate.buildMode != GIST_BUFFERING_STATS)
+	{
+		bool		hasallsortsupports = true;
+		int			keyscount = IndexRelationGetNumberOfKeyAttributes(index);
+
+		for (int i = 0; i < keyscount; i++)
+		{
+			SortSupportFnOids[i] = index_getprocid(index, i + 1,
+												   GIST_SORTSUPPORT_PROC);
+			if (!OidIsValid(SortSupportFnOids[i]))
+			{
+				hasallsortsupports = false;
+				break;
+			}
+		}
+		if (hasallsortsupports)
+			buildstate.buildMode = GIST_SORTED_BUILD;
+	}
+
+	/*
 	 * Calculate target amount of free space to leave on pages.
 	 */
 	fillfactor = options ? options->fillfactor : GIST_DEFAULT_FILLFACTOR;
@@ -852,7 +853,10 @@ gistBuildCallback(Relation index,
 	 * and switch to buffering mode if it has.
 	 *
 	 * To avoid excessive calls to smgrnblocks(), only check this every
-	 * BUFFERING_MODE_SWITCH_CHECK_STEP index tuples
+	 * BUFFERING_MODE_SWITCH_CHECK_STEP index tuples.
+	 *
+	 * In 'stats' state, switch as soon as we have seen enough tuples to have
+	 * some idea of the average tuple size.
 	 */
 	if ((buildstate->buildMode == GIST_BUFFERING_AUTO &&
 		 buildstate->indtuples % BUFFERING_MODE_SWITCH_CHECK_STEP == 0 &&