From: Alvaro Herrera <alvherre@alvh.no-ip.org>
To: Justin Pryzby <pryzby@telsasoft.com>
Cc: Amit Langote <amitlangote09@gmail.com>
Cc: pgsql-hackers@lists.postgresql.org
Subject: Re: ALTER TABLE .. DETACH PARTITION CONCURRENTLY
Date: Fri, 19 Mar 2021 10:57:37 -0300
Message-ID: <20210319135737.GA28771@alvherre.pgsql> (raw)
In-Reply-To: <20210317184534.GB11765@telsasoft.com>
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 <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.cindex 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.cindex 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.cindex 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.cindex 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.hindex 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
Attachments:
[text/plain] 0001-Fix-for-generation-of-invalid-constraints.notpatch (7.9K, ../20210319135737.GA28771@alvherre.pgsql/2-0001-Fix-for-generation-of-invalid-constraints.notpatch)
download | inline diff:
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.cindex 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.cindex 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.cindex 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.cindex 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.hindex 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
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: pgsql-hackers@postgresql.org
Cc: alvherre@alvh.no-ip.org, pryzby@telsasoft.com, amitlangote09@gmail.com, pgsql-hackers@lists.postgresql.org
Subject: Re: ALTER TABLE .. DETACH PARTITION CONCURRENTLY
In-Reply-To: <20210319135737.GA28771@alvherre.pgsql>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox