Disable concurrent index build entirely

The patch removes gp_create_index_concurrently GUC.  There is no use setting
this GUC, concurrent index building is not supported in Greenplum.  Greenplum
needs additional work for this feature to behave properly.  Also undone is an
incorrect (and dangerous) attempt to make this feature work in Greenplum.  The
change was dangerous because IndexStmt was dispatched to QEs outside of a
distributed transaction/2PC, which is a recipe for inconsistency.

GitHub issue #5293 is created to track the work needed for this feature.

src/backend/cdb/cdbvars.c

@@ -266,9 +266,6 @@ bool		gp_setwith_alter_storage = FALSE;
 
 bool		gp_enable_tablespace_auto_mkdir = FALSE;
 
-/* MPP-9772, MPP-9773: remove support for CREATE INDEX CONCURRENTLY */
-bool		gp_create_index_concurrently = FALSE;
-
 /* Enable check for compatibility of encoding and locale in createdb */
 bool		gp_encoding_check_locale_compatibility = true;
 

src/backend/commands/indexcmds.c

@@ -259,7 +259,6 @@ DefineIndex(RangeVar *heapRelation,
 	LOCKMODE	heap_lockmode;
 	bool		need_longlock = true;
 	bool		shouldDispatch;
-	List	   *dispatch_oids;
 	char	   *altconname = stmt ? stmt->altconname : NULL;
 	int			i;
 
@@ -601,30 +600,24 @@ DefineIndex(RangeVar *heapRelation,
 					 concurrent,
 					 altconname);
 
-	if (!concurrent)
+	if (shouldDispatch)
 	{
-		/*
-		 * Dispatch the command to all primary and mirror segment dbs.
-		 * Start a global transaction and reconfigure cluster if needed.
-		 * Wait for QEs to finish.  Exit via ereport(ERROR,...) if error.
-		 * (For a concurrent build, we do this later, see below.)
-		 */
-		if (shouldDispatch)
-		{
-			/* make sure the QE uses the same index name that we chose */
-			stmt->idxname = indexRelationName;
-			CdbDispatchUtilityStatement((Node *) stmt,
-										DF_CANCEL_ON_ERROR |
-										DF_WITH_SNAPSHOT |
-										DF_NEED_TWO_PHASE,
-										GetAssignedOidsForDispatch(),
-										NULL);
-
-			/* Set indcheckxmin in the master, if it was set on any segment */
-			if (!indexInfo->ii_BrokenHotChain)
-				cdb_sync_indcheckxmin_with_segments(indexRelationId);
-		}
+		/* make sure the QE uses the same index name that we chose */
+		stmt->idxname = indexRelationName;
+		CdbDispatchUtilityStatement((Node *) stmt,
+									DF_CANCEL_ON_ERROR |
+									DF_WITH_SNAPSHOT |
+									DF_NEED_TWO_PHASE,
+									GetAssignedOidsForDispatch(),
+									NULL);
 
+		/* Set indcheckxmin in the master, if it was set on any segment */
+		if (!indexInfo->ii_BrokenHotChain)
+			cdb_sync_indcheckxmin_with_segments(indexRelationId);
+	}
+
+	if (!concurrent)
+	{
 		/* Close the heap and we're done, in the non-concurrent case */
 		if (need_longlock)
 			heap_close(rel, NoLock);
@@ -633,6 +626,17 @@ DefineIndex(RangeVar *heapRelation,
 		return;
 	}
 
+	/*
+	 * FIXME: concurrent index build needs additional work in Greenplum.  The
+	 * feature is disabled in Greenplum until this work is done.  In upstream,
+	 * concurrent index build is accomplished in three steps.  Each step is
+	 * performed in its own transaction.  In GPDB, each step must be performed
+	 * in its own distributed transaction.  Today, we only support dispatching
+	 * one IndexStmt.  QEs cannot distinguish the steps within a concurrent
+	 * index build.  May be, augment IndexStmt with a variable indicating which
+	 * step of concurrent index build a QE should perform?
+	 */
+
 	/* save lockrelid and locktag for below, then close rel */
 	heaprelid = rel->rd_lockInfo.lockRelId;
 	SET_LOCKTAG_RELATION(heaplocktag, heaprelid.dbId, heaprelid.relId);
@@ -664,48 +668,9 @@ DefineIndex(RangeVar *heapRelation,
 	 */
 	LockRelationIdForSession(&heaprelid, ShareUpdateExclusiveLock);
 
-	/*
-	 * CommitTransactionCommand will throw an error, if we haven't dispatched
-	 * the assigned oids to the segments, so pick them up first. We will
-	 * dispatch them below, right after committing the transaction.
-	 *
-	 * FIXME: this has to be copied into TopMemoryContext, because the commit
-	 * releases anything else. It is currently leaked!
-	 */
-	{
-		MemoryContext old_context = MemoryContextSwitchTo(TopMemoryContext);
-
-		dispatch_oids = copyObject(GetAssignedOidsForDispatch());
-
-		MemoryContextSwitchTo(old_context);
-	}
-
 	PopActiveSnapshot();
 	CommitTransactionCommand();
 
-	/*
-	 * We dispatch the command to QEs after we've committed the creation of
-	 * the empty index in the master, but before we proceed to fill it.
-	 * This ensures that if something goes wrong, we don't end up in
-	 * a state where the index exists on some segments but not the master.
-	 * It also ensures that the index is only marked as valid on the
-	 * master, after it's been successfully built and marked as valid on
-	 * all the segments.
-	 */
-	if (shouldDispatch)
-	{
-		/*
-		 * Note: We don't use a snapshot. Each QE will create their own
-		 * transactions and take their own snapshots. We will wait later
-		 * later for all the current distributed transactions to finish, so
-		 * it's not important what exact snapshot is used here.
-		 */
-		CdbDispatchUtilityStatement((Node *)stmt,
-									DF_CANCEL_ON_ERROR,
-									dispatch_oids,
-									NULL);
-	}
-
 	StartTransactionCommand();
 
 	/*

src/backend/parser/gram.y

@@ -8111,10 +8111,8 @@ IndexStmt:	CREATE opt_unique INDEX opt_concurrently opt_index_name
 					n->whereClause = $14;
 					n->indexOid = InvalidOid;
 
-                    if (n->concurrent && !gp_create_index_concurrently)
+                    if (n->concurrent)
 					{
-						/* MPP-9772, MPP-9773: remove support for
-						   CREATE INDEX CONCURRENTLY */
 						ereport(ERROR,
 								(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
 								 errmsg("CREATE INDEX CONCURRENTLY is not supported")));

src/backend/utils/misc/guc_gp.c

@@ -770,18 +770,6 @@ struct config_bool ConfigureNamesBool_gp[] =
 		check_dispatch_log_stats, NULL, NULL
 	},
 
-	{
-		/* MPP-9772, MPP-9773: remove support for CREATE INDEX CONCURRENTLY */
-		{"gp_create_index_concurrently", PGC_USERSET, DEVELOPER_OPTIONS,
-			gettext_noop("Allow concurrent index creation."),
-			NULL,
-			GUC_NO_SHOW_ALL | GUC_NOT_IN_SAMPLE
-		},
-		&gp_create_index_concurrently,
-		false,
-		NULL, NULL, NULL
-	},
-
 	{
 		{"gp_enable_minmax_optimization", PGC_USERSET, QUERY_TUNING_METHOD,
 			gettext_noop("Enables the planner's use of index scans with limit to implement MIN/MAX."),

src/include/cdb/cdbvars.h

@@ -787,9 +787,6 @@ extern bool gp_cte_sharing;
 
 extern bool	gp_setwith_alter_storage;
 
-/* MPP-9772, MPP-9773: remove support for CREATE INDEX CONCURRENTLY */
-extern bool	gp_create_index_concurrently;
-
 /* Priority for the segworkers relative to the postmaster's priority */
 extern int gp_segworker_relative_priority;
 

src/test/regress/expected/create_index.out

@@ -1233,13 +1233,12 @@ create index hash_f8_index_3 on hash_f8_heap(random) where seqno > 1000;
 --
 -- Unfortunately this only tests about half the code paths because there are
 -- no concurrent updates happening to the table at the same time.
+--
+-- Concurrent index building is not currently supported in Greenplum.
+--start_ignore
 CREATE TABLE concur_heap (f1 text, f2 text, dk text) distributed by (dk);
 -- empty table
 CREATE INDEX CONCURRENTLY concur_index1 ON concur_heap(f2,f1);
-ERROR:  CREATE INDEX CONCURRENTLY is not supported
--- MPP-9772, MPP-9773: re-enable CREATE INDEX CONCURRENTLY (off by default)
-set gp_create_index_concurrently=true;
-CREATE INDEX CONCURRENTLY concur_index1 ON concur_heap(f2,f1);
 INSERT INTO concur_heap VALUES  ('a','b', '1');
 INSERT INTO concur_heap VALUES  ('b','b', '1');
 INSERT INTO concur_heap VALUES  ('c','c', '2');
@@ -1252,8 +1251,6 @@ ERROR:  duplicate key value violates unique constraint "concur_index2"
 DETAIL:  Key (dk, f1)=(1, b) already exists.
 -- check if constraint is enforced properly at build time
 CREATE UNIQUE INDEX CONCURRENTLY concur_index3 ON concur_heap(dk, f2);
-ERROR:  could not create unique index "concur_index3"
-DETAIL:  Key (dk, f2)=(1, b) is duplicated.
 -- test that expression indexes and partial indexes work concurrently
 CREATE INDEX CONCURRENTLY concur_index4 on concur_heap(f2) WHERE f1='a';
 CREATE INDEX CONCURRENTLY concur_index5 on concur_heap(f2) WHERE f1='x';
@@ -1288,6 +1285,7 @@ Indexes:
     "std_index" btree (f2)
 Distributed by: (dk)
 
+--end_ignore
 DROP TABLE concur_heap;
 --
 -- Test ADD CONSTRAINT USING INDEX

src/test/regress/sql/create_index.sql

@@ -377,13 +377,12 @@ create index hash_f8_index_3 on hash_f8_heap(random) where seqno > 1000;
 --
 -- Unfortunately this only tests about half the code paths because there are
 -- no concurrent updates happening to the table at the same time.
-
+--
+-- Concurrent index building is not currently supported in Greenplum.
+--start_ignore
 CREATE TABLE concur_heap (f1 text, f2 text, dk text) distributed by (dk);
 -- empty table
 CREATE INDEX CONCURRENTLY concur_index1 ON concur_heap(f2,f1);
--- MPP-9772, MPP-9773: re-enable CREATE INDEX CONCURRENTLY (off by default)
-set gp_create_index_concurrently=true;
-CREATE INDEX CONCURRENTLY concur_index1 ON concur_heap(f2,f1);
 INSERT INTO concur_heap VALUES  ('a','b', '1');
 INSERT INTO concur_heap VALUES  ('b','b', '1');
 INSERT INTO concur_heap VALUES  ('c','c', '2');
@@ -415,6 +414,7 @@ COMMIT;
 -- "invalid" flag set.
 
 \d concur_heap
+--end_ignore
 
 DROP TABLE concur_heap;