PartitionDesc-includes_detached-thinko-fix.patch

application/octet-stream

Filename: PartitionDesc-includes_detached-thinko-fix.patch
Type: application/octet-stream
Part: 0
Message: Re: ALTER TABLE .. DETACH PARTITION CONCURRENTLY

Patch

Format: unified
File+
src/backend/catalog/pg_inherits.c 16 8
src/backend/commands/tablecmds.c 9 9
src/backend/commands/trigger.c 1 1
src/backend/partitioning/partdesc.c 3 2
src/include/catalog/pg_inherits.h 1 1
src/test/isolation/expected/detach-partition-concurrently-4.out 1 0
diff --git a/src/backend/catalog/pg_inherits.c b/src/backend/catalog/pg_inherits.c
index bb8b2249b1..ed75468a56 100644
--- a/src/backend/catalog/pg_inherits.c
+++ b/src/backend/catalog/pg_inherits.c
@@ -54,11 +54,12 @@ typedef struct SeenRelsEntry
  *
  * include_detached says to include all partitions, even if they're marked
  * detached.  Passing it as false means they might or might not be included,
- * depending on the visibility of the pg_inherits row for the active snapshot.
+ * depending on the visibility of the pg_inherits row for the active snapshot;
+ * whether actually included or not is returned in *detached_included.
  */
 List *
 find_inheritance_children(Oid parentrelId, bool include_detached,
-						  LOCKMODE lockmode)
+						  bool *detached_included, LOCKMODE lockmode)
 {
 	List	   *list = NIL;
 	Relation	relation;
@@ -71,6 +72,9 @@ find_inheritance_children(Oid parentrelId, bool include_detached,
 				numoids,
 				i;
 
+	if (detached_included)
+		*detached_included = false;
+
 	/*
 	 * Can skip the scan if pg_class shows the relation has never had a
 	 * subclass.
@@ -104,11 +108,12 @@ find_inheritance_children(Oid parentrelId, bool include_detached,
 		 * if its pg_inherits tuple's xmin is still visible to the active
 		 * snapshot.
 		 *
-		 * The reason for this check is that we want to avoid seeing the
-		 * partition as alive in RI queries during REPEATABLE READ or
-		 * SERIALIZABLE transactions.  (If there's no active snapshot set,
-		 * that means we're not running a user query, so it's OK to always
-		 * include detached partitions in that case.)
+		 * The reason for this check is that we want the RI queries running in
+		 * REPEATABLE READ or SERIALIZABLE transactions to not see detach-
+		 * pending partitions that were detached after a given transaction
+		 * started.  (If there's no active snapshot set, that means we're not
+		 * running a user query, so it's OK to always include detached
+		 * partitions in that case.)
 		 */
 		if (((Form_pg_inherits) GETSTRUCT(inheritsTuple))->inhdetachpending &&
 			!include_detached &&
@@ -122,6 +127,9 @@ find_inheritance_children(Oid parentrelId, bool include_detached,
 
 			if (!XidInMVCCSnapshot(xmin, snap))
 				continue;
+
+			if (detached_included)
+				*detached_included = true;
 		}
 
 		inhrelid = ((Form_pg_inherits) GETSTRUCT(inheritsTuple))->inhrelid;
@@ -236,7 +244,7 @@ find_all_inheritors(Oid parentrelId, LOCKMODE lockmode, List **numparents)
 
 		/* Get the direct children of this rel */
 		currentchildren = find_inheritance_children(currentrel, false,
-													lockmode);
+													NULL, lockmode);
 
 		/*
 		 * Add to the queue only those children not already seen. This avoids
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index 096a6f2891..291ebbd3be 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -3507,7 +3507,7 @@ renameatt_internal(Oid myrelid,
 		 * expected_parents will only be 0 if we are not already recursing.
 		 */
 		if (expected_parents == 0 &&
-			find_inheritance_children(myrelid, false, NoLock) != NIL)
+			find_inheritance_children(myrelid, false, NULL, NoLock) != NIL)
 			ereport(ERROR,
 					(errcode(ERRCODE_INVALID_TABLE_DEFINITION),
 					 errmsg("inherited column \"%s\" must be renamed in child tables too",
@@ -3706,7 +3706,7 @@ rename_constraint_internal(Oid myrelid,
 		else
 		{
 			if (expected_parents == 0 &&
-				find_inheritance_children(myrelid, false, NoLock) != NIL)
+				find_inheritance_children(myrelid, false, NULL, NoLock) != NIL)
 				ereport(ERROR,
 						(errcode(ERRCODE_INVALID_TABLE_DEFINITION),
 						 errmsg("inherited constraint \"%s\" must be renamed in child tables too",
@@ -6580,7 +6580,7 @@ ATExecAddColumn(List **wqueue, AlteredTableInfo *tab, Relation rel,
 	 */
 	if (colDef->identity &&
 		recurse &&
-		find_inheritance_children(myrelid, false, NoLock) != NIL)
+		find_inheritance_children(myrelid, false, NULL, NoLock) != NIL)
 		ereport(ERROR,
 				(errcode(ERRCODE_INVALID_TABLE_DEFINITION),
 				 errmsg("cannot recursively add identity column to table that has child tables")));
@@ -6826,7 +6826,7 @@ ATExecAddColumn(List **wqueue, AlteredTableInfo *tab, Relation rel,
 	 * use find_all_inheritors to do it in one pass.
 	 */
 	children =
-		find_inheritance_children(RelationGetRelid(rel), false, lockmode);
+		find_inheritance_children(RelationGetRelid(rel), false, NULL, lockmode);
 
 	/*
 	 * If we are told not to recurse, there had better not be any child
@@ -7689,7 +7689,7 @@ ATPrepDropExpression(Relation rel, AlterTableCmd *cmd, bool recurse, bool recurs
 	 * resulting state can be properly dumped and restored.
 	 */
 	if (!recurse &&
-		find_inheritance_children(RelationGetRelid(rel), false, lockmode))
+		find_inheritance_children(RelationGetRelid(rel), false, NULL, lockmode))
 		ereport(ERROR,
 				(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
 				 errmsg("ALTER TABLE / DROP EXPRESSION must be applied to child tables too")));
@@ -8297,7 +8297,7 @@ ATExecDropColumn(List **wqueue, Relation rel, const char *colName,
 	 * use find_all_inheritors to do it in one pass.
 	 */
 	children =
-		find_inheritance_children(RelationGetRelid(rel), false, lockmode);
+		find_inheritance_children(RelationGetRelid(rel), false, NULL, lockmode);
 
 	if (children)
 	{
@@ -8785,7 +8785,7 @@ ATAddCheckConstraint(List **wqueue, AlteredTableInfo *tab, Relation rel,
 	 * use find_all_inheritors to do it in one pass.
 	 */
 	children =
-		find_inheritance_children(RelationGetRelid(rel), false, lockmode);
+		find_inheritance_children(RelationGetRelid(rel), false, NULL, lockmode);
 
 	/*
 	 * Check if ONLY was specified with ALTER TABLE.  If so, allow the
@@ -11319,7 +11319,7 @@ ATExecDropConstraint(Relation rel, const char *constrName,
 	 */
 	if (!is_no_inherit_constraint)
 		children =
-			find_inheritance_children(RelationGetRelid(rel), false, lockmode);
+			find_inheritance_children(RelationGetRelid(rel), false, NULL, lockmode);
 	else
 		children = NIL;
 
@@ -11704,7 +11704,7 @@ ATPrepAlterColumnType(List **wqueue,
 	}
 	else if (!recursing &&
 			 find_inheritance_children(RelationGetRelid(rel), false,
-									   NoLock) != NIL)
+									   NULL, NoLock) != NIL)
 		ereport(ERROR,
 				(errcode(ERRCODE_INVALID_TABLE_DEFINITION),
 				 errmsg("type of inherited column \"%s\" must be changed in child tables too",
diff --git a/src/backend/commands/trigger.c b/src/backend/commands/trigger.c
index 3421014e47..b216a0a9a9 100644
--- a/src/backend/commands/trigger.c
+++ b/src/backend/commands/trigger.c
@@ -1141,7 +1141,7 @@ CreateTrigger(CreateTrigStmt *stmt, const char *queryString,
 			ListCell   *l;
 			List	   *idxs = NIL;
 
-			idxs = find_inheritance_children(indexOid, false,
+			idxs = find_inheritance_children(indexOid, false, NULL,
 											 ShareRowExclusiveLock);
 			foreach(l, idxs)
 				childTbls = lappend_oid(childTbls,
diff --git a/src/backend/partitioning/partdesc.c b/src/backend/partitioning/partdesc.c
index 58570fecfd..35dd10c6b2 100644
--- a/src/backend/partitioning/partdesc.c
+++ b/src/backend/partitioning/partdesc.c
@@ -105,6 +105,7 @@ RelationBuildPartitionDesc(Relation rel, bool include_detached)
 	MemoryContext new_pdcxt;
 	MemoryContext oldcxt;
 	int		   *mapping;
+	bool		includes_detached;
 
 	/*
 	 * Get partition oids from pg_inherits.  This uses a single snapshot to
@@ -113,7 +114,7 @@ RelationBuildPartitionDesc(Relation rel, bool include_detached)
 	 * some well-defined point in time.
 	 */
 	inhoids = find_inheritance_children(RelationGetRelid(rel), include_detached,
-										NoLock);
+										&includes_detached, NoLock);
 	nparts = list_length(inhoids);
 
 	/* Allocate working arrays for OIDs, leaf flags, and boundspecs. */
@@ -241,7 +242,7 @@ RelationBuildPartitionDesc(Relation rel, bool include_detached)
 		partdesc->boundinfo = partition_bounds_copy(boundinfo, key);
 		partdesc->oids = (Oid *) palloc(nparts * sizeof(Oid));
 		partdesc->is_leaf = (bool *) palloc(nparts * sizeof(bool));
-		partdesc->includes_detached = include_detached;
+		partdesc->includes_detached = includes_detached;
 
 		/*
 		 * Assign OIDs from the original array into mapped indexes of the
diff --git a/src/include/catalog/pg_inherits.h b/src/include/catalog/pg_inherits.h
index 6d07e1b302..26f5489ac8 100644
--- a/src/include/catalog/pg_inherits.h
+++ b/src/include/catalog/pg_inherits.h
@@ -51,7 +51,7 @@ DECLARE_INDEX(pg_inherits_parent_index, 2187, on pg_inherits using btree(inhpare
 
 
 extern List *find_inheritance_children(Oid parentrelId, bool include_detached,
-									   LOCKMODE lockmode);
+									   bool *detached_included, LOCKMODE lockmode);
 extern List *find_all_inheritors(Oid parentrelId, LOCKMODE lockmode,
 								 List **parents);
 extern bool has_subclass(Oid relationId);
diff --git a/src/test/isolation/expected/detach-partition-concurrently-4.out b/src/test/isolation/expected/detach-partition-concurrently-4.out
index 90a75cb077..2167675374 100644
--- a/src/test/isolation/expected/detach-partition-concurrently-4.out
+++ b/src/test/isolation/expected/detach-partition-concurrently-4.out
@@ -324,6 +324,7 @@ a
 1              
 2              
 step s1insert: insert into d4_fk values (1);
+ERROR:  insert or update on table "d4_fk" violates foreign key constraint "d4_fk_a_fkey"
 step s1c: commit;
 
 starting permutation: s2snitch s1b s1s s2detach s1cancel s3vacfreeze s1s s1insert s1c