Date: Fri, 19 Mar 2021 10:57:37 -0300
From: Alvaro Herrera <alvherre@alvh.no-ip.org>
To: Justin Pryzby <pryzby@telsasoft.com>
Cc: Amit Langote <amitlangote09@gmail.com>,
	pgsql-hackers@lists.postgresql.org
Subject: Re: ALTER TABLE .. DETACH PARTITION CONCURRENTLY
Message-ID: <20210319135737.GA28771@alvherre.pgsql>
MIME-Version: 1.0
Content-Type: multipart/mixed; boundary="envbJBWh7q8WU6mo"
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <20210317184534.GB11765@telsasoft.com>
User-Agent: Mutt/1.10.1 (2018-07-13)
Archived-At: 
 <https://www.postgresql.org/message-id/20210319135737.GA28771%40alvherre.pgsql>
Precedence: bulk


--envbJBWh7q8WU6mo
Content-Type: text/plain; charset=iso-8859-1
Content-Disposition: inline
Content-Transfer-Encoding: 8bit

On 2021-Mar-17, Justin Pryzby wrote:

> The v8 patch has the "broken constraint" problem.

Yeah, I had misunderstood what the problem was.  I think a good solution
to this is to have get_partition_parent return the true parent even when
a detach is pending, for one particular callsite.  (This means adjusting
all other callsites.)  Notpatch attached (applies on top of v8).

> Also, it "fails to avoid" adding duplicate constraints:
> 
> Check constraints:
>     "c" CHECK (i IS NOT NULL AND i > 1 AND i < 2)
>     "cc" CHECK (i IS NOT NULL AND i >= 1 AND i < 2)
>     "p1_check" CHECK (true)
>     "p1_i_check" CHECK (i IS NOT NULL AND i >= 1 AND i < 2)

Do you mean the "cc" and "p1_i_check" one?  I mean, if you already have
a constraint in the partition that duplicates the partition constraint,
then during attach we still create our new constraint?  I guess a
solution to this would be to scan all constraints and see if any equals
the expression that the new one would have.  Sounds easy enough now that
write it out loud.

Thanks

-- 
Álvaro Herrera                            39°49'30"S 73°17'W
"Java is clearly an example of money oriented programming"  (A. Stepanov)

--envbJBWh7q8WU6mo
Content-Type: text/plain; charset=us-ascii
Content-Disposition: attachment;
 filename="0001-Fix-for-generation-of-invalid-constraints.notpatch"

From ecec91fc4744ddd9a231754afdf10ec09742f8d9 Mon Sep 17 00:00:00 2001
From: Alvaro Herrera <alvherre@alvh.no-ip.org>
Date: Fri, 19 Mar 2021 10:52:26 -0300
Subject: [PATCH] Fix for generation of invalid constraints

per report from Justin Pryzby
---
 src/backend/catalog/heap.c          |  2 +-
 src/backend/catalog/partition.c     | 32 ++++++++++++++---------------
 src/backend/commands/tablecmds.c    | 12 +++++------
 src/backend/utils/cache/partcache.c | 11 ++--------
 src/include/catalog/partition.h     |  2 +-
 5 files changed, 26 insertions(+), 33 deletions(-)

diff --git a/src/backend/catalog/heap.c b/src/backend/catalog/heap.c
index 1a05f2a2fe..c6c5e40a39 100644
--- a/src/backend/catalog/heap.c
+++ b/src/backend/catalog/heap.c
@@ -1919,7 +1919,7 @@ heap_drop_with_catalog(Oid relid)
 		elog(ERROR, "cache lookup failed for relation %u", relid);
 	if (((Form_pg_class) GETSTRUCT(tuple))->relispartition)
 	{
-		parentOid = get_partition_parent(relid);
+		parentOid = get_partition_parent(relid, true);
 		LockRelationOid(parentOid, AccessExclusiveLock);
 
 		/*
diff --git a/src/backend/catalog/partition.c b/src/backend/catalog/partition.c
index 797a7384ad..a7cf3d9e86 100644
--- a/src/backend/catalog/partition.c
+++ b/src/backend/catalog/partition.c
@@ -42,15 +42,15 @@ static void get_partition_ancestors_worker(Relation inhRel, Oid relid,
  *
  * Returns inheritance parent of a partition by scanning pg_inherits
  *
- * If the partition is in the process of being detached, InvalidOid is
- * returned and no error is thrown.
+ * If the partition is in the process of being detached, we raise the same
+ * error unless even_if_detached is passed.
  *
  * Note: Because this function assumes that the relation whose OID is passed
  * as an argument will have precisely one parent, it should only be called
  * when it is known that the relation is a partition.
  */
 Oid
-get_partition_parent(Oid relid)
+get_partition_parent(Oid relid, bool even_if_detached)
 {
 	Relation	catalogRelation;
 	Oid			result;
@@ -60,10 +60,10 @@ get_partition_parent(Oid relid)
 
 	result = get_partition_parent_worker(catalogRelation, relid, &detached);
 
-	if (detached)
-		result = InvalidOid;
-	else if (!OidIsValid(result))
+	if (!OidIsValid(result))
 		elog(ERROR, "could not find tuple for parent of relation %u", relid);
+	if (detached && !even_if_detached)
+		elog(ERROR, "relation %u has no parent because it's being detached", relid);
 
 	table_close(catalogRelation, AccessShareLock);
 
@@ -75,8 +75,8 @@ get_partition_parent(Oid relid)
  *		Scan the pg_inherits relation to return the OID of the parent of the
  *		given relation
  *
- * If the partition is being detached, InvalidOid is returned, and also
- * *detached is set true.
+ * If the partition is being detached, *detached is set true (but the original
+ * parent is still returned.)
  */
 static Oid
 get_partition_parent_worker(Relation inhRel, Oid relid, bool *detached)
@@ -106,12 +106,9 @@ get_partition_parent_worker(Relation inhRel, Oid relid, bool *detached)
 
 		/* A partition being detached has no parent */
 		if (form->inhdetachpending)
-		{
 			*detached = true;
-			result = InvalidOid;
-		}
-		else
-			result = form->inhparent;
+
+		result = form->inhparent;
 	}
 
 	systable_endscan(scan);
@@ -154,9 +151,12 @@ get_partition_ancestors_worker(Relation inhRel, Oid relid, List **ancestors)
 	Oid			parentOid;
 	bool		detached;
 
-	/* Recursion ends at the topmost level, ie., when there's no parent */
+	/*
+	 * Recursion ends at the topmost level, ie., when there's no parent; also
+	 * when the partition is being detached.
+	 */
 	parentOid = get_partition_parent_worker(inhRel, relid, &detached);
-	if (parentOid == InvalidOid)
+	if (parentOid == InvalidOid || detached)
 		return;
 
 	*ancestors = lappend_oid(*ancestors, parentOid);
@@ -189,7 +189,7 @@ index_get_partition(Relation partition, Oid indexId)
 		ReleaseSysCache(tup);
 		if (!ispartition)
 			continue;
-		if (get_partition_parent(partIdx) == indexId)
+		if (get_partition_parent(partIdx, false) == indexId)
 		{
 			list_free(idxlist);
 			return partIdx;
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index 21dc869b0b..853dc8d257 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -1547,7 +1547,7 @@ RangeVarCallbackForDropRelation(const RangeVar *rel, Oid relOid, Oid oldRelOid,
 	 */
 	if (is_partition && relOid != oldRelOid)
 	{
-		state->partParentOid = get_partition_parent(relOid);
+		state->partParentOid = get_partition_parent(relOid, false);
 		if (OidIsValid(state->partParentOid))
 			LockRelationOid(state->partParentOid, AccessExclusiveLock);
 	}
@@ -6816,7 +6816,7 @@ ATExecDropNotNull(Relation rel, const char *colName, LOCKMODE lockmode)
 	/* If rel is partition, shouldn't drop NOT NULL if parent has the same */
 	if (rel->rd_rel->relispartition)
 	{
-		Oid			parentId = get_partition_parent(RelationGetRelid(rel));
+		Oid			parentId = get_partition_parent(RelationGetRelid(rel), false);
 		Relation	parent = table_open(parentId, AccessShareLock);
 		TupleDesc	tupDesc = RelationGetDescr(parent);
 		AttrNumber	parent_attnum;
@@ -17361,7 +17361,7 @@ DetachPartitionFinalize(Relation rel, Relation partRel, bool concurrent,
 		if (!has_superclass(idxid))
 			continue;
 
-		Assert((IndexGetRelation(get_partition_parent(idxid), false) ==
+		Assert((IndexGetRelation(get_partition_parent(idxid, false), false) ==
 				RelationGetRelid(rel)));
 
 		idx = index_open(idxid, AccessExclusiveLock);
@@ -17660,7 +17660,7 @@ ATExecAttachPartitionIdx(List **wqueue, Relation parentIdx, RangeVar *name)
 
 	/* Silently do nothing if already in the right state */
 	currParent = partIdx->rd_rel->relispartition ?
-		get_partition_parent(partIdxId) : InvalidOid;
+		get_partition_parent(partIdxId, false) : InvalidOid;
 	if (currParent != RelationGetRelid(parentIdx))
 	{
 		IndexInfo  *childInfo;
@@ -17872,8 +17872,8 @@ validatePartitionedIndex(Relation partedIdx, Relation partedTbl)
 		/* make sure we see the validation we just did */
 		CommandCounterIncrement();
 
-		parentIdxId = get_partition_parent(RelationGetRelid(partedIdx));
-		parentTblId = get_partition_parent(RelationGetRelid(partedTbl));
+		parentIdxId = get_partition_parent(RelationGetRelid(partedIdx), false);
+		parentTblId = get_partition_parent(RelationGetRelid(partedTbl), false);
 		parentIdx = relation_open(parentIdxId, AccessExclusiveLock);
 		parentTbl = relation_open(parentTblId, AccessExclusiveLock);
 		Assert(!parentIdx->rd_index->indisvalid);
diff --git a/src/backend/utils/cache/partcache.c b/src/backend/utils/cache/partcache.c
index 0fe4f55b04..6dfa3fb4a8 100644
--- a/src/backend/utils/cache/partcache.c
+++ b/src/backend/utils/cache/partcache.c
@@ -352,16 +352,9 @@ generate_partition_qual(Relation rel)
 		return copyObject(rel->rd_partcheck);
 
 	/*
-	 * Obtain parent relid; if it's invalid, then the partition is being
-	 * detached.  The constraint is NIL in that case, and let's cache that.
+	 * Obtain parent relid.  XXX explain why we need this
 	 */
-	parentrelid = get_partition_parent(RelationGetRelid(rel));
-	if (parentrelid == InvalidOid)
-	{
-		rel->rd_partcheckvalid = true;
-		rel->rd_partcheck = NIL;
-		return NIL;
-	}
+	parentrelid = get_partition_parent(RelationGetRelid(rel), true);
 
 	/* Grab at least an AccessShareLock on the parent table */
 	parent = relation_open(parentrelid, AccessShareLock);
diff --git a/src/include/catalog/partition.h b/src/include/catalog/partition.h
index fe3f66befa..c8c7bc1d99 100644
--- a/src/include/catalog/partition.h
+++ b/src/include/catalog/partition.h
@@ -19,7 +19,7 @@
 /* Seed for the extended hash function */
 #define HASH_PARTITION_SEED UINT64CONST(0x7A5B22367996DCFD)
 
-extern Oid	get_partition_parent(Oid relid);
+extern Oid	get_partition_parent(Oid relid, bool even_if_detached);
 extern List *get_partition_ancestors(Oid relid);
 extern Oid	index_get_partition(Relation partition, Oid indexId);
 extern List *map_partition_varattnos(List *expr, int fromrel_varno,
-- 
2.20.1


--envbJBWh7q8WU6mo--