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)
>From ecec91fc4744ddd9a231754afdf10ec09742f8d9 Mon Sep 17 00:00:00 2001
From: Alvaro Herrera <alvhe...@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
