From d955f7c59318057d4e37f61b6ed646571ec02a30 Mon Sep 17 00:00:00 2001 From: shihao zhong Date: Sat, 7 Nov 2026 22:08:46 -0500 Subject: [PATCH 3/3] Let DDL see partitions pending detach Only queries and publications now omit such partitions, so DDL on the parent reaches them and FINALIZE works. --- src/backend/catalog/heap.c | 2 +- src/backend/catalog/pg_inherits.c | 26 ++++++++--- src/backend/catalog/pg_publication.c | 8 +++- src/backend/commands/analyze.c | 3 +- src/backend/commands/copyto.c | 3 +- src/backend/commands/indexcmds.c | 4 +- src/backend/commands/tablecmds.c | 27 +++++++----- src/backend/commands/trigger.c | 9 ++-- src/backend/partitioning/partbounds.c | 6 +-- src/backend/partitioning/partdesc.c | 15 ++++++- src/backend/replication/logical/worker.c | 6 +-- src/backend/utils/adt/partitionfuncs.c | 3 +- src/include/catalog/pg_inherits.h | 2 + src/include/partitioning/partdesc.h | 2 +- .../detach-partition-concurrently-3.out | 44 +++++++++++++++++++ .../detach-partition-concurrently-3.spec | 14 ++++++ 16 files changed, 134 insertions(+), 40 deletions(-) diff --git a/src/backend/catalog/heap.c b/src/backend/catalog/heap.c index a2c76b22fb0..2fb13a8e2a0 100644 --- a/src/backend/catalog/heap.c +++ b/src/backend/catalog/heap.c @@ -4455,7 +4455,7 @@ StorePartitionBound(Relation rel, Relation parent, PartitionBoundSpec *bound) * removed. */ defaultPartOid = - get_default_oid_from_partdesc(RelationGetPartitionDesc(parent, true)); + get_default_oid_from_partdesc(RelationGetPartitionDesc(parent)); if (OidIsValid(defaultPartOid)) CacheInvalidateRelcacheByRelid(defaultPartOid); diff --git a/src/backend/catalog/pg_inherits.c b/src/backend/catalog/pg_inherits.c index 23fb7886cfb..51c5c3f0e3e 100644 --- a/src/backend/catalog/pg_inherits.c +++ b/src/backend/catalog/pg_inherits.c @@ -52,13 +52,13 @@ typedef struct SeenRelsEntry * then no locks are acquired, but caller must beware of race conditions * against possible DROPs of child relations. * - * Partitions marked as being detached are omitted; see - * find_inheritance_children_extended for details. + * Partitions marked as being detached are included. Only queries omit + * them; see find_inheritance_children_extended. */ List * find_inheritance_children(Oid parentrelId, LOCKMODE lockmode) { - return find_inheritance_children_extended(parentrelId, true, lockmode, + return find_inheritance_children_extended(parentrelId, false, lockmode, NULL, NULL); } @@ -249,11 +249,22 @@ find_inheritance_children_extended(Oid parentrelId, bool omit_detached, * then no locks are acquired, but caller must beware of race conditions * against possible DROPs of child relations. * - * NB - No current callers of this routine are interested in children being - * concurrently detached, so there's no provision to include them. + * Partitions pending detach are included, as in find_inheritance_children. */ List * find_all_inheritors(Oid parentrelId, LOCKMODE lockmode, List **numparents) +{ + return find_all_inheritors_extended(parentrelId, false, lockmode, + numparents); +} + +/* + * find_all_inheritors_extended - + * As find_all_inheritors, optionally omitting partitions pending detach. + */ +List * +find_all_inheritors_extended(Oid parentrelId, bool omit_detached, + LOCKMODE lockmode, List **numparents) { /* hash table for O(1) rel_oid -> rel_numparents cell lookup */ HTAB *seen_rels; @@ -290,7 +301,10 @@ find_all_inheritors(Oid parentrelId, LOCKMODE lockmode, List **numparents) ListCell *lc; /* Get the direct children of this rel */ - currentchildren = find_inheritance_children(currentrel, lockmode); + currentchildren = find_inheritance_children_extended(currentrel, + omit_detached, + lockmode, + NULL, NULL); /* * Add to the queue only those children not already seen. This avoids diff --git a/src/backend/catalog/pg_publication.c b/src/backend/catalog/pg_publication.c index f11ea8839ca..9f732cc6dcb 100644 --- a/src/backend/catalog/pg_publication.c +++ b/src/backend/catalog/pg_publication.c @@ -434,8 +434,12 @@ GetPubPartitionOptionRelations(List *result, PublicationPartOpt pub_partopt, if (get_rel_relkind(relid) == RELKIND_PARTITIONED_TABLE && pub_partopt != PUBLICATION_PART_ROOT) { - List *all_parts = find_all_inheritors(relid, NoLock, - NULL); + /* + * A partition pending detach is not published through its parent, so + * leave it out here. + */ + List *all_parts = find_all_inheritors_extended(relid, true, + NoLock, NULL); if (pub_partopt == PUBLICATION_PART_ALL) result = list_concat(result, all_parts); diff --git a/src/backend/commands/analyze.c b/src/backend/commands/analyze.c index 518d7526255..d01e12a9159 100644 --- a/src/backend/commands/analyze.c +++ b/src/backend/commands/analyze.c @@ -1497,7 +1497,8 @@ acquire_inherited_sample_rows(Relation onerel, int elevel, * the children. */ tableOIDs = - find_all_inheritors(RelationGetRelid(onerel), AccessShareLock, NULL); + find_all_inheritors_extended(RelationGetRelid(onerel), true, + AccessShareLock, NULL); /* * Check that there's at least one descendant, else fail. This could diff --git a/src/backend/commands/copyto.c b/src/backend/commands/copyto.c index 9ca06ed3e18..cf0fc69c300 100644 --- a/src/backend/commands/copyto.c +++ b/src/backend/commands/copyto.c @@ -860,7 +860,8 @@ BeginCopyTo(ParseState *pstate, * Collect OIDs of relation containing data, so that later * DoCopyTo can copy the data from them. */ - children = find_all_inheritors(RelationGetRelid(rel), AccessShareLock, NULL); + children = find_all_inheritors_extended(RelationGetRelid(rel), true, + AccessShareLock, NULL); foreach_oid(child, children) { diff --git a/src/backend/commands/indexcmds.c b/src/backend/commands/indexcmds.c index 892abff4db5..572b487c94d 100644 --- a/src/backend/commands/indexcmds.c +++ b/src/backend/commands/indexcmds.c @@ -1267,7 +1267,7 @@ DefineIndex(ParseState *pstate, */ if (partitioned && stmt->relation && !stmt->relation->inh) { - PartitionDesc pd = RelationGetPartitionDesc(rel, true); + PartitionDesc pd = RelationGetPartitionDesc(rel); if (pd->nparts != 0) flags |= INDEX_CREATE_INVALID; @@ -1337,7 +1337,7 @@ DefineIndex(ParseState *pstate, * * If we're called internally (no stmt->relation), recurse always. */ - partdesc = RelationGetPartitionDesc(rel, true); + partdesc = RelationGetPartitionDesc(rel); if ((!stmt->relation || stmt->relation->inh) && partdesc->nparts > 0) { int nparts = partdesc->nparts; diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c index 45171356ad1..9f07c05e583 100644 --- a/src/backend/commands/tablecmds.c +++ b/src/backend/commands/tablecmds.c @@ -1195,8 +1195,7 @@ DefineRelation(CreateStmt *stmt, char relkind, Oid ownerId, * lock the partition so as to avoid a deadlock. */ defaultPartOid = - get_default_oid_from_partdesc(RelationGetPartitionDesc(parent, - true)); + get_default_oid_from_partdesc(RelationGetPartitionDesc(parent)); if (OidIsValid(defaultPartOid)) defaultRel = table_open(defaultPartOid, AccessExclusiveLock); @@ -1949,7 +1948,8 @@ ExecuteTruncate(TruncateStmt *stmt) ListCell *child; List *children; - children = find_all_inheritors(myrelid, lockmode, NULL); + children = find_all_inheritors_extended(myrelid, true, lockmode, + NULL); foreach(child, children) { @@ -4986,9 +4986,12 @@ ATPrepCmd(List **wqueue, Relation rel, AlterTableCmd *cmd, /* * Disallow any ALTER TABLE other than ALTER TABLE DETACH FINALIZE on - * partitions that are pending detach. + * partitions that are pending detach. A command recursing from the + * parent must still reach such a partition, since it remains a partition + * until the detach is finalized. */ - if (rel->rd_rel->relispartition && + if (!recursing && + rel->rd_rel->relispartition && cmd->subtype != AT_DetachPartitionFinalize && PartitionHasPendingDetach(RelationGetRelid(rel))) ereport(ERROR, @@ -11643,7 +11646,7 @@ addFkRecurseReferenced(Constraint *fkconstraint, Relation rel, */ if (pkrel->rd_rel->relkind == RELKIND_PARTITIONED_TABLE) { - PartitionDesc pd = RelationGetPartitionDesc(pkrel, true); + PartitionDesc pd = RelationGetPartitionDesc(pkrel); for (int i = 0; i < pd->nparts; i++) { @@ -11814,7 +11817,7 @@ addFkRecurseReferencing(List **wqueue, Constraint *fkconstraint, Relation rel, } else if (rel->rd_rel->relkind == RELKIND_PARTITIONED_TABLE) { - PartitionDesc pd = RelationGetPartitionDesc(rel, true); + PartitionDesc pd = RelationGetPartitionDesc(rel); Relation trigrel; /* @@ -21615,7 +21618,7 @@ QueuePartitionConstraintValidation(List **wqueue, Relation scanrel, } else if (scanrel->rd_rel->relkind == RELKIND_PARTITIONED_TABLE) { - PartitionDesc partdesc = RelationGetPartitionDesc(scanrel, true); + PartitionDesc partdesc = RelationGetPartitionDesc(scanrel); int i; for (i = 0; i < partdesc->nparts; i++) @@ -21676,7 +21679,7 @@ ATExecAttachPartition(List **wqueue, Relation rel, PartitionCmd *cmd, * new partition will change its partition constraint. */ defaultPartOid = - get_default_oid_from_partdesc(RelationGetPartitionDesc(rel, true)); + get_default_oid_from_partdesc(RelationGetPartitionDesc(rel)); if (OidIsValid(defaultPartOid)) LockRelationOid(defaultPartOid, AccessExclusiveLock); @@ -22361,7 +22364,7 @@ ATExecDetachPartition(List **wqueue, AlteredTableInfo *tab, Relation rel, * will change its partition constraint. */ defaultPartOid = - get_default_oid_from_partdesc(RelationGetPartitionDesc(rel, true)); + get_default_oid_from_partdesc(RelationGetPartitionDesc(rel)); if (OidIsValid(defaultPartOid)) { /* @@ -23089,7 +23092,7 @@ ATExecAttachPartitionIdx(List **wqueue, Relation parentIdx, RangeVar *name) RelationGetRelationName(partIdx)))); /* Make sure it indexes a partition of the other index's table */ - partDesc = RelationGetPartitionDesc(parentTbl, true); + partDesc = RelationGetPartitionDesc(parentTbl); found = false; for (i = 0; i < partDesc->nparts; i++) { @@ -23263,7 +23266,7 @@ validatePartitionedIndex(Relation partedIdx, Relation partedTbl) * Partitions pending detach must be counted too, because their indexes * remain attached to this index until the detach is finalized. */ - if (tuples == RelationGetPartitionDesc(partedTbl, false)->nparts) + if (tuples == RelationGetPartitionDesc(partedTbl)->nparts) { Relation idxRel; HeapTuple indTup; diff --git a/src/backend/commands/trigger.c b/src/backend/commands/trigger.c index 838eae31660..01beb028ab9 100644 --- a/src/backend/commands/trigger.c +++ b/src/backend/commands/trigger.c @@ -1171,7 +1171,7 @@ CreateTriggerFiringOn(const CreateTrigStmt *stmt, const char *queryString, */ if (partition_recurse) { - PartitionDesc partdesc = RelationGetPartitionDesc(rel, true); + PartitionDesc partdesc = RelationGetPartitionDesc(rel); int i; MemoryContext oldcxt, perChildCxt; @@ -1580,7 +1580,7 @@ renametrig(RenameStmt *stmt) /* ... and if it is partitioned, recurse to its partitions */ if (targetrel->rd_rel->relkind == RELKIND_PARTITIONED_TABLE) { - PartitionDesc partdesc = RelationGetPartitionDesc(targetrel, true); + PartitionDesc partdesc = RelationGetPartitionDesc(targetrel); for (int i = 0; i < partdesc->nparts; i++) { @@ -1726,8 +1726,7 @@ renametrig_partition(Relation tgrel, Oid partitionId, Oid parentTriggerOid, /* And if this relation is partitioned, recurse to its partitions */ if (partitionRel->rd_rel->relkind == RELKIND_PARTITIONED_TABLE) { - PartitionDesc partdesc = RelationGetPartitionDesc(partitionRel, - true); + PartitionDesc partdesc = RelationGetPartitionDesc(partitionRel); for (int i = 0; i < partdesc->nparts; i++) { @@ -1849,7 +1848,7 @@ EnableDisableTrigger(Relation rel, const char *tgname, Oid tgparent, rel->rd_rel->relkind == RELKIND_PARTITIONED_TABLE && (TRIGGER_FOR_ROW(oldtrig->tgtype))) { - PartitionDesc partdesc = RelationGetPartitionDesc(rel, true); + PartitionDesc partdesc = RelationGetPartitionDesc(rel); int i; for (i = 0; i < partdesc->nparts; i++) diff --git a/src/backend/partitioning/partbounds.c b/src/backend/partitioning/partbounds.c index f618f59f7a8..ff3f5988032 100644 --- a/src/backend/partitioning/partbounds.c +++ b/src/backend/partitioning/partbounds.c @@ -2885,7 +2885,7 @@ check_new_partition_bound(char *relname, Relation parent, PartitionBoundSpec *spec, ParseState *pstate) { PartitionKey key = RelationGetPartitionKey(parent); - PartitionDesc partdesc = RelationGetPartitionDesc(parent, false); + PartitionDesc partdesc = RelationGetPartitionDesc(parent); PartitionBoundInfo boundinfo = partdesc->boundinfo; int with = -1; bool overlap = false; @@ -4088,7 +4088,7 @@ get_qual_for_list(Relation parent, PartitionBoundSpec *spec) { int i; int ndatums = 0; - PartitionDesc pdesc = RelationGetPartitionDesc(parent, false); + PartitionDesc pdesc = RelationGetPartitionDesc(parent); PartitionBoundInfo boundinfo = pdesc->boundinfo; if (boundinfo) @@ -4288,7 +4288,7 @@ get_qual_for_range(Relation parent, PartitionBoundSpec *spec, if (spec->is_default) { List *or_expr_args = NIL; - PartitionDesc pdesc = RelationGetPartitionDesc(parent, false); + PartitionDesc pdesc = RelationGetPartitionDesc(parent); Oid *inhoids = pdesc->oids; int nparts = pdesc->nparts, k; diff --git a/src/backend/partitioning/partdesc.c b/src/backend/partitioning/partdesc.c index 4399fe9a3a0..64f45f1549a 100644 --- a/src/backend/partitioning/partdesc.c +++ b/src/backend/partitioning/partdesc.c @@ -46,6 +46,7 @@ typedef struct PartitionDirectoryEntry PartitionDesc pd; } PartitionDirectoryEntry; +static PartitionDesc get_partition_desc(Relation rel, bool omit_detached); static PartitionDesc RelationBuildPartitionDesc(Relation rel, bool omit_detached); @@ -68,7 +69,17 @@ static PartitionDesc RelationBuildPartitionDesc(Relation rel, * that the data doesn't become stale. */ PartitionDesc -RelationGetPartitionDesc(Relation rel, bool omit_detached) +RelationGetPartitionDesc(Relation rel) +{ + return get_partition_desc(rel, false); +} + +/* + * get_partition_desc -- workhorse for RelationGetPartitionDesc and + * PartitionDirectoryLookup. Only queries omit partitions pending detach. + */ +static PartitionDesc +get_partition_desc(Relation rel, bool omit_detached) { Assert(rel->rd_rel->relkind == RELKIND_PARTITIONED_TABLE); @@ -467,7 +478,7 @@ PartitionDirectoryLookup(PartitionDirectory pdir, Relation rel) */ RelationIncrementReferenceCount(rel); pde->rel = rel; - pde->pd = RelationGetPartitionDesc(rel, pdir->omit_detached); + pde->pd = get_partition_desc(rel, pdir->omit_detached); Assert(pde->pd != NULL); } return pde->pd; diff --git a/src/backend/replication/logical/worker.c b/src/backend/replication/logical/worker.c index 95044620e68..ddbd5335b03 100644 --- a/src/backend/replication/logical/worker.c +++ b/src/backend/replication/logical/worker.c @@ -3769,9 +3769,9 @@ apply_handle_truncate(StringInfo s) if (rel->localrel->rd_rel->relkind == RELKIND_PARTITIONED_TABLE) { ListCell *child; - List *children = find_all_inheritors(rel->localreloid, - lockmode, - NULL); + List *children = find_all_inheritors_extended(rel->localreloid, + true, lockmode, + NULL); foreach(child, children) { diff --git a/src/backend/utils/adt/partitionfuncs.c b/src/backend/utils/adt/partitionfuncs.c index e9db027aa2e..5ce62388db0 100644 --- a/src/backend/utils/adt/partitionfuncs.c +++ b/src/backend/utils/adt/partitionfuncs.c @@ -85,7 +85,8 @@ pg_partition_tree(PG_FUNCTION_ARGS) * Find all members of inheritance set. We only need AccessShareLock * on the children for the partition information lookup. */ - partitions = find_all_inheritors(rootrelid, AccessShareLock, NULL); + partitions = find_all_inheritors_extended(rootrelid, true, + AccessShareLock, NULL); if (get_call_result_type(fcinfo, NULL, &tupdesc) != TYPEFUNC_COMPOSITE) elog(ERROR, "return type must be a row type"); diff --git a/src/include/catalog/pg_inherits.h b/src/include/catalog/pg_inherits.h index cc874abaabb..694d39a1f22 100644 --- a/src/include/catalog/pg_inherits.h +++ b/src/include/catalog/pg_inherits.h @@ -58,6 +58,8 @@ extern List *find_inheritance_children_extended(Oid parentrelId, bool omit_detac extern List *find_all_inheritors(Oid parentrelId, LOCKMODE lockmode, List **numparents); +extern List *find_all_inheritors_extended(Oid parentrelId, bool omit_detached, + LOCKMODE lockmode, List **numparents); extern bool has_subclass(Oid relationId); extern bool has_superclass(Oid relationId); extern bool typeInheritsFrom(Oid subclassTypeId, Oid superclassTypeId); diff --git a/src/include/partitioning/partdesc.h b/src/include/partitioning/partdesc.h index f8698e87455..0f2ff8f663d 100644 --- a/src/include/partitioning/partdesc.h +++ b/src/include/partitioning/partdesc.h @@ -58,7 +58,7 @@ typedef struct PartitionDescData } PartitionDescData; -extern PartitionDesc RelationGetPartitionDesc(Relation rel, bool omit_detached); +extern PartitionDesc RelationGetPartitionDesc(Relation rel); extern PartitionDirectory CreatePartitionDirectory(MemoryContext mcxt, bool omit_detached); extern PartitionDesc PartitionDirectoryLookup(PartitionDirectory, Relation); diff --git a/src/test/isolation/expected/detach-partition-concurrently-3.out b/src/test/isolation/expected/detach-partition-concurrently-3.out index 485068dbd81..78385da7da0 100644 --- a/src/test/isolation/expected/detach-partition-concurrently-3.out +++ b/src/test/isolation/expected/detach-partition-concurrently-3.out @@ -570,3 +570,47 @@ indisvalid t (1 row) + +starting permutation: s2snitch s1b s1s s2detach s1cancel s1c s1addck s1setdef s1alter s1pubtables s2detachfinal s1p1ddl +step s2snitch: INSERT INTO d3_pid SELECT pg_backend_pid(); +step s1b: BEGIN; +step s1s: SELECT * FROM d3_listp; +a +- +1 +(1 row) + +step s2detach: ALTER TABLE d3_listp DETACH PARTITION d3_listp1 CONCURRENTLY; +step s1cancel: SELECT pg_cancel_backend(pid) FROM d3_pid; +step s2detach: <... completed> +ERROR: canceling statement due to user request +step s1cancel: <... completed> +pg_cancel_backend +----------------- +t +(1 row) + +step s1c: COMMIT; +step s1addck: ALTER TABLE d3_listp ADD CHECK (a > 0); +step s1setdef: ALTER TABLE d3_listp ALTER a SET DEFAULT 1; +step s1alter: ALTER TABLE d3_listp1 ALTER a DROP NOT NULL; +ERROR: cannot alter partition "d3_listp1" with an incomplete detach +step s1pubtables: CREATE PUBLICATION pub_d3t FOR TABLE d3_listp; + SELECT tablename FROM pg_publication_tables + WHERE pubname = 'pub_d3t' ORDER BY 1; + DROP PUBLICATION pub_d3t; +tablename +--------- +d3_listp2 +(1 row) + +step s2detachfinal: ALTER TABLE d3_listp DETACH PARTITION d3_listp1 FINALIZE; +step s1p1ddl: SELECT (SELECT pg_get_expr(adbin, adrelid) FROM pg_attrdef + WHERE adrelid = 'd3_listp1'::regclass) AS def, + (SELECT count(*) FROM pg_constraint + WHERE conrelid = 'd3_listp1'::regclass AND contype = 'c') AS checks; +def|checks +---+------ + 1| 1 +(1 row) + diff --git a/src/test/isolation/specs/detach-partition-concurrently-3.spec b/src/test/isolation/specs/detach-partition-concurrently-3.spec index 7b7b6fee1d6..86efa5954b8 100644 --- a/src/test/isolation/specs/detach-partition-concurrently-3.spec +++ b/src/test/isolation/specs/detach-partition-concurrently-3.spec @@ -50,6 +50,16 @@ step s1attach2 { ALTER INDEX d3_idx ATTACH PARTITION d3_listp2_a; } step s1attach3 { CREATE UNIQUE INDEX d3_listp3_a ON d3_listp3 (a); ALTER INDEX d3_idx ATTACH PARTITION d3_listp3_a; } step s1valid { SELECT indisvalid FROM pg_index WHERE indexrelid = 'd3_idx'::regclass; } +step s1addck { ALTER TABLE d3_listp ADD CHECK (a > 0); } +step s1setdef { ALTER TABLE d3_listp ALTER a SET DEFAULT 1; } +step s1pubtables { CREATE PUBLICATION pub_d3t FOR TABLE d3_listp; + SELECT tablename FROM pg_publication_tables + WHERE pubname = 'pub_d3t' ORDER BY 1; + DROP PUBLICATION pub_d3t; } +step s1p1ddl { SELECT (SELECT pg_get_expr(adbin, adrelid) FROM pg_attrdef + WHERE adrelid = 'd3_listp1'::regclass) AS def, + (SELECT count(*) FROM pg_constraint + WHERE conrelid = 'd3_listp1'::regclass AND contype = 'c') AS checks; } session s2 step s2begin { BEGIN; } @@ -106,3 +116,7 @@ permutation s2snitch s1b s1s s2detach s1cancel(s2detach) s1c s2begin s2detachfin # The index of a partition pending detach must not count toward validity. permutation s2snitch s1idx s1b s1s s2detach s1cancel(s2detach) s1c s1attach2 s1valid s1attach3 s1valid + +# DDL on the parent must reach a partition pending detach, while DDL on +# the partition itself is still refused. Publications do not list it. +permutation s2snitch s1b s1s s2detach s1cancel(s2detach) s1c s1addck s1setdef s1alter s1pubtables s2detachfinal s1p1ddl -- 2.37.1 (Apple Git-137.1)