Set indcheckxmin in master, if it was set on any segment.

Fixes github issue #1774.

src/backend/commands/indexcmds.c

@@ -68,6 +68,7 @@
 #include "cdb/cdbcat.h"
 #include "cdb/cdbrelsize.h"
 #include "cdb/cdboidsync.h"
+#include "gp-libpq-fe.h"
 
 /* non-export function prototypes */
 static void CheckPredicate(Expr *predicate);
@@ -85,6 +86,96 @@ static bool relationHasPrimaryKey(Relation rel);
 static bool relationHasUniqueIndex(Relation rel);
 
 
+/*
+ * Helper function, to check indcheckxmin for an index on all segments, and
+ * set it on the master if it was set on any segment.
+ *
+ * If CREATE INDEX creates a "broken" HOT chain, the new index must not be
+ * used by new queries, with an old snapshot, that would need to see the old
+ * values. See src/backend/access/heap/README.HOT. This is enforced by
+ * setting indcheckxmin in the pg_index row. In GPDB, we use the pg_index
+ * row in the master for planning, but all the data is stored in the
+ * segments, so indcheckxmin must be set in the master, if it's set in any
+ * of the segments.
+ */
+static void
+cdb_sync_indcheckxmin_with_segments(Oid indexRelationId)
+{
+	CdbPgResults cdb_pgresults = {NULL, 0};
+	int			i;
+	char		cmd[100];
+	bool		indcheckxmin_set_in_any_segment;
+
+	Assert(Gp_role == GP_ROLE_DISPATCH && !IsBootstrapProcessingMode());
+
+	/*
+	 * Query all the segments, for their indcheckxmin value for this index.
+	 */
+	snprintf(cmd, sizeof(cmd),
+			 "select indcheckxmin from pg_catalog.pg_index where indexrelid = '%u'",
+			 indexRelationId);
+
+	CdbDispatchCommand(cmd, DF_WITH_SNAPSHOT, &cdb_pgresults);
+
+	indcheckxmin_set_in_any_segment = false;
+	for (i = 0; i < cdb_pgresults.numResults; i++)
+	{
+		char	   *val;
+
+		if (PQresultStatus(cdb_pgresults.pg_results[i]) != PGRES_TUPLES_OK)
+		{
+			cdbdisp_clearCdbPgResults(&cdb_pgresults);
+			elog(ERROR, "could not fetch indcheckxmin from segment");
+		}
+
+		if (PQntuples(cdb_pgresults.pg_results[i]) != 1 ||
+			PQnfields(cdb_pgresults.pg_results[i]) != 1 ||
+			PQgetisnull(cdb_pgresults.pg_results[i], 0, 0))
+			elog(ERROR, "unexpected shape of result set for indcheckxmin query");
+
+		val = PQgetvalue(cdb_pgresults.pg_results[i], 0, 0);
+		if (val[0] == 't')
+		{
+			indcheckxmin_set_in_any_segment = true;
+			break;
+		}
+		else if (val[0] != 'f')
+			elog(ERROR, "invalid boolean value received from segment: %s", val);
+	}
+
+	cdbdisp_clearCdbPgResults(&cdb_pgresults);
+
+	/*
+	 * If indcheckxmin was set on any segment, also set it in the master.
+	 */
+	if (indcheckxmin_set_in_any_segment)
+	{
+		Relation	pg_index;
+		HeapTuple	indexTuple;
+		Form_pg_index indexForm;
+
+		pg_index = heap_open(IndexRelationId, RowExclusiveLock);
+
+		indexTuple = SearchSysCacheCopy(INDEXRELID,
+										ObjectIdGetDatum(indexRelationId),
+										0, 0, 0);
+		if (!HeapTupleIsValid(indexTuple))
+			elog(ERROR, "cache lookup failed for index %u", indexRelationId);
+		indexForm = (Form_pg_index) GETSTRUCT(indexTuple);
+
+		if (!indexForm->indcheckxmin)
+		{
+			indexForm->indcheckxmin = true;
+			simple_heap_update(pg_index, &indexTuple->t_self, indexTuple);
+			CatalogUpdateIndexes(pg_index, indexTuple);
+		}
+
+		heap_freetuple(indexTuple);
+		heap_close(pg_index, RowExclusiveLock);
+	}
+}
+
+
 /*
  * DefineIndex
  *		Creates a new index.
@@ -530,7 +621,6 @@ DefineIndex(RangeVar *heapRelation,
 	else
 		heap_close(rel, heap_lockmode);
 
-	/* create the index on the QEs first, so we can get their stats when we create on the QD */
    	if (shouldDispatch)
 	{
 		cdb_sync_oid_to_segments();
@@ -559,6 +649,7 @@ DefineIndex(RangeVar *heapRelation,
 		 * (For a concurrent build, we do this later, see below.)
 		 */
 		if (shouldDispatch)
+		{
 			CdbDispatchUtilityStatement((Node *) stmt,
 										DF_CANCEL_ON_ERROR |
 										DF_WITH_SNAPSHOT |
@@ -566,6 +657,11 @@ DefineIndex(RangeVar *heapRelation,
 										GetAssignedOidsForDispatch(),
 										NULL);
 
+			/* Set indcheckxmin in the master, if it was set on any segment */
+			if (!indexInfo->ii_BrokenHotChain)
+				cdb_sync_indcheckxmin_with_segments(indexRelationId);
+		}
+
 		return;					/* We're done, in the standard case */
 	}
 

src/test/isolation/expected/create_index_hot.out

+Parsed test spec with 2 sessions
+
+starting permutation: s2begin s2select s1update s1createindexonc s2select s2forceindexscan s2select
+step s2begin: BEGIN ISOLATION LEVEL SERIALIZABLE;
+step s2select: select '#' as expected, c from hot where c = '#'
+                  union all
+                  select '$', c from hot where c = '$';
+expected       c              
+
+#              #              
+step s1update: update hot set c = '$' where c = '#';
+step s1createindexonc: create index idx_c on hot (c);
+step s2select: select '#' as expected, c from hot where c = '#'
+                  union all
+                  select '$', c from hot where c = '$';
+expected       c              
+
+#              #              
+step s2forceindexscan: set enable_indexscan=on; set enable_seqscan=off;
+step s2select: select '#' as expected, c from hot where c = '#'
+                  union all
+                  select '$', c from hot where c = '$';
+expected       c              
+
+#              #              

src/test/isolation/isolation_schedule

@@ -5,3 +5,4 @@ test: heap-serializable-vacuum
 test: ao-serializable-read
 test: ao-serializable-vacuum
 test: ao-insert-eof
+test: create_index_hot

src/test/isolation/specs/create_index_hot.spec

+#
+# Test behavior with HOT and CREATE INDEX.
+#
+# If CREATE INDEX creates a "broken" HOT chain, the new index must not be
+# used by new queries, with an old snapshot, that would need to see the
+# old values. See src/backend/access/heap/README.HOT.
+#
+# This is enforced by setting indcheckxmin in the pg_index row. In GPDB,
+# we use the pg_index row in the master for planning, but all the data is
+# stored in the segments, so indcheckxmin must be set in the master, if
+# it's set in any of the segments.
+setup
+{
+    create table hot (dummy int, a int, b char, c char);
+    create index idx_a on hot (a);
+    insert into hot values (1,1,'A','#');
+}
+
+teardown
+{
+    drop table hot;
+}
+
+# Update a row, and create an index on the updated column. This produces
+# a broken HOT chain.
+session "s1"
+step "s1update" { update hot set c = '$' where c = '#'; }
+step "s1createindexonc" { create index idx_c on hot (c); }
+
+session "s2"
+step "s2begin"  { BEGIN ISOLATION LEVEL SERIALIZABLE; }
+step "s2select" { select '#' as expected, c from hot where c = '#'
+                  union all
+                  select '$', c from hot where c = '$'; }
+step "s2forceindexscan" { set enable_indexscan=on; set enable_seqscan=off; }
+teardown        { abort; }
+
+permutation
+  "s2begin"
+  "s2select"
+
+  "s1update"
+  "s1createindexonc"
+
+  "s2select"
+  "s2forceindexscan"
+  "s2select"