On 2021-Apr-26, Alvaro Herrera wrote: > > Please allow me to study the patch a bit more closely and get back tomorrow. > > Sure, thanks!
Here's a more polished version. After trying the version with the elog(ERROR) when two detached partitions are present, I decided against it; it is unhelpful because it doesn't let you build partition descriptors for anything. So I made it an elog(WARNING) (not an ereport, note), and keep the most recent pg_inherits.xmin value. This is not great, but it lets you out of the situation by finalizing one of the detaches. The other check (at ALTER TABLE .. DETACH time) is an ereport(ERROR) and should make the first one unreachable. -- Álvaro Herrera 39°49'30"S 73°17'W
>From 89e8a7d170fc0c6701e536b9c5830e3a58f303cc Mon Sep 17 00:00:00 2001 From: Alvaro Herrera <alvhe...@alvh.no-ip.org> Date: Mon, 26 Apr 2021 14:53:04 -0400 Subject: [PATCH] Allow a partdesc-with-omitted-partitions to be cached Makes partdesc acquisition faster during the transient period during which a partition is in the process of being dropped. Per gripe from Amit Langote Discussion: https://postgr.es/m/ca+hiwqfgpp1lxjzobygt9rpvtjxxkg5qg2+xch2z1q7krqz...@mail.gmail.com --- src/backend/catalog/pg_inherits.c | 52 +++++++++++-- src/backend/commands/tablecmds.c | 66 +++++++++------- src/backend/commands/trigger.c | 3 +- src/backend/partitioning/partdesc.c | 75 ++++++++++++------- src/include/catalog/pg_inherits.h | 6 +- src/include/utils/rel.h | 3 + .../detach-partition-concurrently-3.out | 57 +++++++++++++- .../detach-partition-concurrently-3.spec | 9 ++- 8 files changed, 205 insertions(+), 66 deletions(-) diff --git a/src/backend/catalog/pg_inherits.c b/src/backend/catalog/pg_inherits.c index 6447b52854..c373faf2d6 100644 --- a/src/backend/catalog/pg_inherits.c +++ b/src/backend/catalog/pg_inherits.c @@ -52,6 +52,22 @@ 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. + */ +List * +find_inheritance_children(Oid parentrelId, LOCKMODE lockmode) +{ + return find_inheritance_children_extended(parentrelId, true, lockmode, + NULL, NULL); +} + +/* + * find_inheritance_children_extended + * + * As find_inheritance_children, with more options regarding detached + * partitions. + * * If a partition's pg_inherits row is marked "detach pending", * *detached_exist (if not null) is set true. * @@ -60,11 +76,13 @@ typedef struct SeenRelsEntry * marked "detach pending" is visible to that snapshot, then that partition is * omitted from the output list. This makes partitions invisible depending on * whether the transaction that marked those partitions as detached appears - * committed to the active snapshot. + * committed to the active snapshot. In addition, *detached_xmin (if not null) + * is set to the xmin of the row of the detached partition. */ List * -find_inheritance_children(Oid parentrelId, bool omit_detached, - LOCKMODE lockmode, bool *detached_exist) +find_inheritance_children_extended(Oid parentrelId, bool omit_detached, + LOCKMODE lockmode, bool *detached_exist, + TransactionId *detached_xmin) { List *list = NIL; Relation relation; @@ -132,7 +150,32 @@ find_inheritance_children(Oid parentrelId, bool omit_detached, snap = GetActiveSnapshot(); if (!XidInMVCCSnapshot(xmin, snap)) + { + if (detached_xmin) + { + /* + * Two detached partitions should not occur (see + * checks in MarkInheritDetached), but if they do, + * track the newer of the two. Make sure to warn the + * user, so that they can clean up. Since this is + * just a cross-check against potentially corrupt + * catalogs, we don't make it a full-fledged error + * message. + */ + if (*detached_xmin != InvalidTransactionId) + { + elog(WARNING, "more than one partition pending detach found for table with OID %u", + parentrelId); + if (TransactionIdFollows(xmin, *detached_xmin)) + *detached_xmin = xmin; + } + else + *detached_xmin = xmin; + } + + /* Don't add the partition to the output list */ continue; + } } } @@ -247,8 +290,7 @@ find_all_inheritors(Oid parentrelId, LOCKMODE lockmode, List **numparents) ListCell *lc; /* Get the direct children of this rel */ - currentchildren = find_inheritance_children(currentrel, true, - lockmode, NULL); + currentchildren = find_inheritance_children(currentrel, 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 7d00f4eb25..ab3c61898c 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, true, NoLock, NULL) != NIL) + find_inheritance_children(myrelid, 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, true, NoLock, NULL) != NIL) + find_inheritance_children(myrelid, 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, true, NoLock, NULL) != NIL) + find_inheritance_children(myrelid, 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), true, lockmode, NULL); + find_inheritance_children(RelationGetRelid(rel), 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), true, lockmode, NULL)) + find_inheritance_children(RelationGetRelid(rel), 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), true, lockmode, NULL); + find_inheritance_children(RelationGetRelid(rel), 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), true, lockmode, NULL); + find_inheritance_children(RelationGetRelid(rel), lockmode); /* * Check if ONLY was specified with ALTER TABLE. If so, allow the @@ -11318,8 +11318,7 @@ ATExecDropConstraint(Relation rel, const char *constrName, * use find_all_inheritors to do it in one pass. */ if (!is_no_inherit_constraint) - children = find_inheritance_children(RelationGetRelid(rel), true, - lockmode, NULL); + children = find_inheritance_children(RelationGetRelid(rel), lockmode); else children = NIL; @@ -11703,8 +11702,7 @@ ATPrepAlterColumnType(List **wqueue, } } else if (!recursing && - find_inheritance_children(RelationGetRelid(rel), true, - NoLock, NULL) != NIL) + find_inheritance_children(RelationGetRelid(rel), NoLock) != NIL) ereport(ERROR, (errcode(ERRCODE_INVALID_TABLE_DEFINITION), errmsg("type of inherited column \"%s\" must be changed in child tables too", @@ -14616,7 +14614,8 @@ ATExecDropInherit(Relation rel, RangeVar *parent, LOCKMODE lockmode) * MarkInheritDetached * * Set inhdetachpending for a partition, for ATExecDetachPartition - * in concurrent mode. + * in concurrent mode. While at it, verify that no other partition is + * already pending detach. */ static void MarkInheritDetached(Relation child_rel, Relation parent_rel) @@ -14632,32 +14631,45 @@ MarkInheritDetached(Relation child_rel, Relation parent_rel) Assert(parent_rel->rd_rel->relkind == RELKIND_PARTITIONED_TABLE); /* - * Find pg_inherits entries by inhrelid. + * Find pg_inherits entries by inhparent. (We need to scan them all in + * order to verify that no other partition is pending detach.) */ catalogRelation = table_open(InheritsRelationId, RowExclusiveLock); ScanKeyInit(&key, - Anum_pg_inherits_inhrelid, + Anum_pg_inherits_inhparent, BTEqualStrategyNumber, F_OIDEQ, - ObjectIdGetDatum(RelationGetRelid(child_rel))); - scan = systable_beginscan(catalogRelation, InheritsRelidSeqnoIndexId, + ObjectIdGetDatum(RelationGetRelid(parent_rel))); + scan = systable_beginscan(catalogRelation, InheritsParentIndexId, true, NULL, 1, &key); while (HeapTupleIsValid(inheritsTuple = systable_getnext(scan))) { - HeapTuple newtup; + Form_pg_inherits inhForm; - if (((Form_pg_inherits) GETSTRUCT(inheritsTuple))->inhparent != - RelationGetRelid(parent_rel)) - elog(ERROR, "bad parent tuple found for partition %u", - RelationGetRelid(child_rel)); + inhForm = (Form_pg_inherits) GETSTRUCT(inheritsTuple); + if (inhForm->inhdetachpending) + ereport(ERROR, + errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE), + errmsg("partition \"%s\" already pending detach in partitioned table \"%s.%s\"", + get_rel_name(inhForm->inhrelid), + get_namespace_name(parent_rel->rd_rel->relnamespace), + RelationGetRelationName(parent_rel)), + errhint("Use ALTER TABLE ... DETACH PARTITION ... FINALIZE to complete the detach operation.")); - newtup = heap_copytuple(inheritsTuple); - ((Form_pg_inherits) GETSTRUCT(newtup))->inhdetachpending = true; + if (inhForm->inhrelid == RelationGetRelid(child_rel)) + { + HeapTuple newtup; - CatalogTupleUpdate(catalogRelation, - &inheritsTuple->t_self, - newtup); - found = true; + newtup = heap_copytuple(inheritsTuple); + ((Form_pg_inherits) GETSTRUCT(newtup))->inhdetachpending = true; + + CatalogTupleUpdate(catalogRelation, + &inheritsTuple->t_self, + newtup); + found = true; + heap_freetuple(newtup); + /* keep looking, to ensure we catch others pending detach */ + } } /* Done */ diff --git a/src/backend/commands/trigger.c b/src/backend/commands/trigger.c index d8393aa4de..f305f8bc0f 100644 --- a/src/backend/commands/trigger.c +++ b/src/backend/commands/trigger.c @@ -1141,8 +1141,7 @@ CreateTrigger(CreateTrigStmt *stmt, const char *queryString, ListCell *l; List *idxs = NIL; - idxs = find_inheritance_children(indexOid, true, - ShareRowExclusiveLock, NULL); + idxs = find_inheritance_children(indexOid, ShareRowExclusiveLock); foreach(l, idxs) childTbls = lappend_oid(childTbls, IndexGetRelation(lfirst_oid(l), diff --git a/src/backend/partitioning/partdesc.c b/src/backend/partitioning/partdesc.c index 2305dff407..9af740f6e1 100644 --- a/src/backend/partitioning/partdesc.c +++ b/src/backend/partitioning/partdesc.c @@ -78,11 +78,33 @@ RelationGetPartitionDesc(Relation rel, bool omit_detached) * If relcache has a partition descriptor, use that. However, we can only * do so when we are asked to include all partitions including detached; * and also when we know that there are no detached partitions. + * + * If there is no active snapshot, detached partitions aren't omitted + * either, so we can use the cached descriptor too in that case. */ if (likely(rel->rd_partdesc && - (!rel->rd_partdesc->detached_exist || !omit_detached))) + (!rel->rd_partdesc->detached_exist || !omit_detached || + !ActiveSnapshotSet()))) return rel->rd_partdesc; + /* + * If we're asked to omit detached partitions, we may be able to use a + * cached version of it, if it was built with the same conditions that we + * see now. + */ + if (omit_detached && + rel->rd_partdesc_nodetached && + TransactionIdIsValid(rel->rd_partdesc_nodetached_xmin) && + ActiveSnapshotSet()) + { + Snapshot activesnap; + + activesnap = GetActiveSnapshot(); + + if (!XidInMVCCSnapshot(rel->rd_partdesc_nodetached_xmin, activesnap)) + return rel->rd_partdesc_nodetached; + } + return RelationBuildPartitionDesc(rel, omit_detached); } @@ -117,6 +139,7 @@ RelationBuildPartitionDesc(Relation rel, bool omit_detached) Oid *oids = NULL; bool *is_leaf = NULL; bool detached_exist; + TransactionId detached_xmin; ListCell *cell; int i, nparts; @@ -132,8 +155,11 @@ RelationBuildPartitionDesc(Relation rel, bool omit_detached) * some well-defined point in time. */ detached_exist = false; - inhoids = find_inheritance_children(RelationGetRelid(rel), omit_detached, - NoLock, &detached_exist); + detached_xmin = InvalidTransactionId; + inhoids = find_inheritance_children_extended(RelationGetRelid(rel), + omit_detached, NoLock, + &detached_exist, + &detached_xmin); nparts = list_length(inhoids); /* Allocate working arrays for OIDs, leaf flags, and boundspecs. */ @@ -282,36 +308,29 @@ RelationBuildPartitionDesc(Relation rel, bool omit_detached) /* * We have a fully valid partdesc. Reparent it so that it has the right - * lifespan, and if appropriate put it into the relation's relcache entry. + * lifespan. */ - if (omit_detached && detached_exist) + MemoryContextSetParent(new_pdcxt, CacheMemoryContext); + + /* + * But first, a kluge: if there's an old rd_pdcxt, it contains an old + * partition descriptor that may still be referenced somewhere. + * Preserve it, while not leaking it, by reattaching it as a child + * context of the new rd_pdcxt. Eventually it will get dropped by + * either RelationClose or RelationClearRelation. + */ + if (rel->rd_pdcxt != NULL) + MemoryContextSetParent(rel->rd_pdcxt, new_pdcxt); + rel->rd_pdcxt = new_pdcxt; + + /* Store it into relcache */ + if (omit_detached && detached_exist && ActiveSnapshotSet()) { - /* - * A transient partition descriptor is only good for the current - * statement, so make it a child of the current portal's context. - */ - MemoryContextSetParent(new_pdcxt, PortalContext); + rel->rd_partdesc_nodetached = partdesc; + rel->rd_partdesc_nodetached_xmin = detached_xmin; } else { - /* - * This partdesc goes into relcache. - */ - - MemoryContextSetParent(new_pdcxt, CacheMemoryContext); - - /* - * But first, a kluge: if there's an old rd_pdcxt, it contains an old - * partition descriptor that may still be referenced somewhere. - * Preserve it, while not leaking it, by reattaching it as a child - * context of the new rd_pdcxt. Eventually it will get dropped by - * either RelationClose or RelationClearRelation. - */ - if (rel->rd_pdcxt != NULL) - MemoryContextSetParent(rel->rd_pdcxt, new_pdcxt); - rel->rd_pdcxt = new_pdcxt; - - /* Store it into relcache */ rel->rd_partdesc = partdesc; } diff --git a/src/include/catalog/pg_inherits.h b/src/include/catalog/pg_inherits.h index 4d28ede5a6..f47925588a 100644 --- a/src/include/catalog/pg_inherits.h +++ b/src/include/catalog/pg_inherits.h @@ -50,8 +50,10 @@ DECLARE_INDEX(pg_inherits_parent_index, 2187, on pg_inherits using btree(inhpare #define InheritsParentIndexId 2187 -extern List *find_inheritance_children(Oid parentrelId, bool omit_detached, - LOCKMODE lockmode, bool *detached_exist); +extern List *find_inheritance_children(Oid parentrelId, LOCKMODE lockmode); +extern List *find_inheritance_children_extended(Oid parentrelId, bool omit_detached, + LOCKMODE lockmode, bool *detached_exist, TransactionId *detached_xmin); + extern List *find_all_inheritors(Oid parentrelId, LOCKMODE lockmode, List **parents); extern bool has_subclass(Oid relationId); diff --git a/src/include/utils/rel.h b/src/include/utils/rel.h index 34e25eb597..6e5a5bf4a7 100644 --- a/src/include/utils/rel.h +++ b/src/include/utils/rel.h @@ -127,6 +127,9 @@ typedef struct RelationData /* data managed by RelationGetPartitionDesc: */ PartitionDesc rd_partdesc; /* partition descriptor, or NULL */ + PartitionDesc rd_partdesc_nodetached; /* partition descriptor */ + TransactionId rd_partdesc_nodetached_xmin; /* xmin of partdesc */ + MemoryContext rd_pdcxt; /* private context for rd_partdesc, if any */ /* data managed by RelationGetPartitionQual: */ diff --git a/src/test/isolation/expected/detach-partition-concurrently-3.out b/src/test/isolation/expected/detach-partition-concurrently-3.out index 88e83638c7..dbb3825792 100644 --- a/src/test/isolation/expected/detach-partition-concurrently-3.out +++ b/src/test/isolation/expected/detach-partition-concurrently-3.out @@ -20,6 +20,7 @@ step s1describe: SELECT 'd3_listp' AS root, * FROM pg_partition_tree('d3_listp') root relid parentrelid isleaf level d3_listp d3_listp f 0 +d3_listp d3_listp2 d3_listp t 1 d3_listp1 d3_listp1 t 0 step s1alter: ALTER TABLE d3_listp1 ALTER a DROP NOT NULL; ERROR: cannot alter partition "d3_listp1" with an incomplete detach @@ -123,6 +124,61 @@ a 1 +starting permutation: s2snitch s1b s1s s2detach s1cancel s2detach2 s1c +step s2snitch: INSERT INTO d3_pid SELECT pg_backend_pid(); +step s1b: BEGIN; +step s1s: SELECT * FROM d3_listp; +a + +1 +step s2detach: ALTER TABLE d3_listp DETACH PARTITION d3_listp1 CONCURRENTLY; <waiting ...> +step s1cancel: SELECT pg_cancel_backend(pid) FROM d3_pid; +pg_cancel_backend + +t +step s2detach: <... completed> +error in steps s1cancel s2detach: ERROR: canceling statement due to user request +step s2detach2: ALTER TABLE d3_listp DETACH PARTITION d3_listp2 CONCURRENTLY; +ERROR: partition "d3_listp1" already pending detach in partitioned table "public.d3_listp" +step s1c: COMMIT; + +starting permutation: s2snitch s1b s1s s2detach s1cancel s2detachfinal s1c s2detach2 +step s2snitch: INSERT INTO d3_pid SELECT pg_backend_pid(); +step s1b: BEGIN; +step s1s: SELECT * FROM d3_listp; +a + +1 +step s2detach: ALTER TABLE d3_listp DETACH PARTITION d3_listp1 CONCURRENTLY; <waiting ...> +step s1cancel: SELECT pg_cancel_backend(pid) FROM d3_pid; +pg_cancel_backend + +t +step s2detach: <... completed> +error in steps s1cancel s2detach: ERROR: canceling statement due to user request +step s2detachfinal: ALTER TABLE d3_listp DETACH PARTITION d3_listp1 FINALIZE; <waiting ...> +step s1c: COMMIT; +step s2detachfinal: <... completed> +step s2detach2: ALTER TABLE d3_listp DETACH PARTITION d3_listp2 CONCURRENTLY; + +starting permutation: s2snitch s1b s1s s2detach s1cancel s1c s1droppart s2detach2 +step s2snitch: INSERT INTO d3_pid SELECT pg_backend_pid(); +step s1b: BEGIN; +step s1s: SELECT * FROM d3_listp; +a + +1 +step s2detach: ALTER TABLE d3_listp DETACH PARTITION d3_listp1 CONCURRENTLY; <waiting ...> +step s1cancel: SELECT pg_cancel_backend(pid) FROM d3_pid; +pg_cancel_backend + +t +step s2detach: <... completed> +error in steps s1cancel s2detach: ERROR: canceling statement due to user request +step s1c: COMMIT; +step s1droppart: DROP TABLE d3_listp1; +step s2detach2: ALTER TABLE d3_listp DETACH PARTITION d3_listp2 CONCURRENTLY; + starting permutation: s2snitch s1b s1s s2detach s1cancel s1c s2begin s2drop s1s s2commit step s2snitch: INSERT INTO d3_pid SELECT pg_backend_pid(); step s1b: BEGIN; @@ -279,4 +335,3 @@ step s2detachfinal: ALTER TABLE d3_listp DETACH PARTITION d3_listp1 FINALIZE; step s1insertpart: INSERT INTO d3_listp1 VALUES (1); <waiting ...> step s2commit: COMMIT; step s1insertpart: <... completed> -unused step name: s1droppart diff --git a/src/test/isolation/specs/detach-partition-concurrently-3.spec b/src/test/isolation/specs/detach-partition-concurrently-3.spec index 4b706430e1..4c563890eb 100644 --- a/src/test/isolation/specs/detach-partition-concurrently-3.spec +++ b/src/test/isolation/specs/detach-partition-concurrently-3.spec @@ -4,12 +4,13 @@ setup { CREATE TABLE d3_listp (a int) PARTITION BY LIST(a); CREATE TABLE d3_listp1 PARTITION OF d3_listp FOR VALUES IN (1); + CREATE TABLE d3_listp2 PARTITION OF d3_listp FOR VALUES IN (2); CREATE TABLE d3_pid (pid int); INSERT INTO d3_listp VALUES (1); } teardown { - DROP TABLE IF EXISTS d3_listp, d3_listp1, d3_pid; + DROP TABLE IF EXISTS d3_listp, d3_listp1, d3_listp2, d3_pid; } session "s1" @@ -34,6 +35,7 @@ session "s2" step "s2begin" { BEGIN; } step "s2snitch" { INSERT INTO d3_pid SELECT pg_backend_pid(); } step "s2detach" { ALTER TABLE d3_listp DETACH PARTITION d3_listp1 CONCURRENTLY; } +step "s2detach2" { ALTER TABLE d3_listp DETACH PARTITION d3_listp2 CONCURRENTLY; } step "s2detachfinal" { ALTER TABLE d3_listp DETACH PARTITION d3_listp1 FINALIZE; } step "s2drop" { DROP TABLE d3_listp1; } step "s2commit" { COMMIT; } @@ -49,6 +51,11 @@ permutation "s2snitch" "s1b" "s1s" "s2detach" "s1cancel" "s1c" "s1drop" "s1list" # "truncate" only does parent, not partition permutation "s2snitch" "s1b" "s1s" "s2detach" "s1cancel" "s1c" "s1trunc" "s1spart" +# If a partition pending detach exists, we cannot drop another one +permutation "s2snitch" "s1b" "s1s" "s2detach" "s1cancel" "s2detach2" "s1c" +permutation "s2snitch" "s1b" "s1s" "s2detach" "s1cancel" "s2detachfinal" "s1c" "s2detach2" +permutation "s2snitch" "s1b" "s1s" "s2detach" "s1cancel" "s1c" "s1droppart" "s2detach2" + # When a partition with incomplete detach is dropped, we grab lock on parent too. permutation "s2snitch" "s1b" "s1s" "s2detach" "s1cancel" "s1c" "s2begin" "s2drop" "s1s" "s2commit" -- 2.20.1
