public inbox for [email protected]help / color / mirror / Atom feed
[PATCH v1 2/2] ALTER TABLE ... DETACH CONCURRENTLY 4+ messages / 3 participants [nested] [flat]
* [PATCH v1 2/2] ALTER TABLE ... DETACH CONCURRENTLY @ 2020-08-03 23:21 Alvaro Herrera <[email protected]> 0 siblings, 0 replies; 4+ messages in thread From: Alvaro Herrera @ 2020-08-03 23:21 UTC (permalink / raw) --- doc/src/sgml/catalogs.sgml | 10 + doc/src/sgml/ref/alter_table.sgml | 25 +- src/backend/catalog/heap.c | 13 +- src/backend/catalog/index.c | 4 +- src/backend/catalog/pg_inherits.c | 51 +- src/backend/commands/indexcmds.c | 5 +- src/backend/commands/tablecmds.c | 491 ++++++++++++++---- src/backend/commands/trigger.c | 7 +- src/backend/executor/execPartition.c | 16 +- src/backend/nodes/copyfuncs.c | 1 + src/backend/nodes/equalfuncs.c | 1 + src/backend/optimizer/util/plancat.c | 2 +- src/backend/parser/gram.y | 22 +- src/backend/partitioning/partbounds.c | 6 +- src/backend/partitioning/partdesc.c | 21 +- src/backend/tcop/utility.c | 19 + src/bin/psql/describe.c | 40 +- src/include/catalog/pg_inherits.h | 7 +- src/include/nodes/parsenodes.h | 2 + src/include/parser/kwlist.h | 1 + src/include/partitioning/partdesc.h | 5 +- .../detach-partition-concurrently-1.out | 182 +++++++ .../detach-partition-concurrently-2.out | 66 +++ src/test/isolation/isolation_schedule | 2 + .../detach-partition-concurrently-1.spec | 71 +++ .../detach-partition-concurrently-2.spec | 41 ++ 26 files changed, 958 insertions(+), 153 deletions(-) create mode 100644 src/test/isolation/expected/detach-partition-concurrently-1.out create mode 100644 src/test/isolation/expected/detach-partition-concurrently-2.out create mode 100644 src/test/isolation/specs/detach-partition-concurrently-1.spec create mode 100644 src/test/isolation/specs/detach-partition-concurrently-2.spec diff --git a/doc/src/sgml/catalogs.sgml b/doc/src/sgml/catalogs.sgml index 26fda20d19..0f8abdc2d0 100644 --- a/doc/src/sgml/catalogs.sgml +++ b/doc/src/sgml/catalogs.sgml @@ -4471,6 +4471,16 @@ SCRAM-SHA-256$<replaceable><iteration count></replaceable>:<replaceable>&l when using declarative partitioning. </para></entry> </row> + + <row> + <entry role="catalog_table_entry"><para role="column_definition"> + <structfield>inhdetached</structfield> <type>bool</type> + </para> + <para> + Set to true for a partition that is in the process of being detached; + false otherwise. + </para></entry> + </row> </tbody> </tgroup> </table> diff --git a/doc/src/sgml/ref/alter_table.sgml b/doc/src/sgml/ref/alter_table.sgml index b2eb7097a9..2e67468e46 100644 --- a/doc/src/sgml/ref/alter_table.sgml +++ b/doc/src/sgml/ref/alter_table.sgml @@ -36,7 +36,9 @@ ALTER TABLE ALL IN TABLESPACE <replaceable class="parameter">name</replaceable> ALTER TABLE [ IF EXISTS ] <replaceable class="parameter">name</replaceable> ATTACH PARTITION <replaceable class="parameter">partition_name</replaceable> { FOR VALUES <replaceable class="parameter">partition_bound_spec</replaceable> | DEFAULT } ALTER TABLE [ IF EXISTS ] <replaceable class="parameter">name</replaceable> - DETACH PARTITION <replaceable class="parameter">partition_name</replaceable> + DETACH PARTITION <replaceable class="parameter">partition_name</replaceable> [ CONCURRENTLY ] +ALTER TABLE [ IF EXISTS ] <replaceable class="parameter">name</replaceable> + DETACH PARTITION <replaceable class="parameter">partition_name</replaceable> FINALIZE <phrase>where <replaceable class="parameter">action</replaceable> is one of:</phrase> @@ -938,7 +940,8 @@ WITH ( MODULUS <replaceable class="parameter">numeric_literal</replaceable>, REM </varlistentry> <varlistentry> - <term><literal>DETACH PARTITION</literal> <replaceable class="parameter">partition_name</replaceable></term> + <term><literal>DETACH PARTITION</literal> <replaceable class="parameter">partition_name</replaceable> [ { CONCURRENTLY | FINALIZE } ]</term> + <listitem> <para> This form detaches the specified partition of the target table. The detached @@ -947,6 +950,24 @@ WITH ( MODULUS <replaceable class="parameter">numeric_literal</replaceable>, REM attached to the target table's indexes are detached. Any triggers that were created as clones of those in the target table are removed. </para> + <para> + If <literal>CONCURRENTLY</literal> is specified, this process runs in two + transactions in order to avoid blocking other sessions that might be accessing + the partitioned table. During the first transaction, + <literal>SHARE UPDATE EXCLUSIVE</literal> is taken in both parent table and + partition, and the partition is marked detached; at that point, the transaction + is committed and all transactions using the partitioned table are waited for. + Once all those transactions are gone, the second stage acquires + <literal>ACCESS EXCLUSIVE</literal> on the partition, and the detach process + completes. + <literal>CONCURRENTLY</literal> is not allowed if the + partitioned table contains a default partition. + </para> + <para> + If <literal>FINALIZE</literal> is specified, complete actions of a + previous <literal>DETACH CONCURRENTLY</literal> invocation that + was cancelled or crashed. + </para> </listitem> </varlistentry> diff --git a/src/backend/catalog/heap.c b/src/backend/catalog/heap.c index f2ca686397..7cc5cb4d51 100644 --- a/src/backend/catalog/heap.c +++ b/src/backend/catalog/heap.c @@ -2552,10 +2552,12 @@ StoreConstraints(Relation rel, List *cooked_constraints, bool is_internal) * Returns a list of CookedConstraint nodes that shows the cooked form of * the default and constraint expressions added to the relation. * - * NB: caller should have opened rel with AccessExclusiveLock, and should - * hold that lock till end of transaction. Also, we assume the caller has - * done a CommandCounterIncrement if necessary to make the relation's catalog - * tuples visible. + * NB: caller should have opened rel with some self-conflicting lock mode, + * and should hold that lock till end of transaction; for normal cases that'll + * be AccessExclusiveLock, but if caller knows that the constraint is already + * enforced by some other means, it can be ShareUpdateExclusiveLock. Also, we + * assume the caller has done a CommandCounterIncrement if necessary to make + * the relation's catalog tuples visible. */ List * AddRelationNewConstraints(Relation rel, @@ -3787,7 +3789,8 @@ StorePartitionBound(Relation rel, Relation parent, PartitionBoundSpec *bound) * relcache entry for that partition every time a partition is added or * removed. */ - defaultPartOid = get_default_oid_from_partdesc(RelationGetPartitionDesc(parent)); + defaultPartOid = + get_default_oid_from_partdesc(RelationGetPartitionDesc(parent, false)); if (OidIsValid(defaultPartOid)) CacheInvalidateRelcacheByRelid(defaultPartOid); diff --git a/src/backend/catalog/index.c b/src/backend/catalog/index.c index 1be27eec52..ad545a7866 100644 --- a/src/backend/catalog/index.c +++ b/src/backend/catalog/index.c @@ -1655,7 +1655,7 @@ index_concurrently_swap(Oid newIndexId, Oid oldIndexId, const char *oldName) List *ancestors = get_partition_ancestors(oldIndexId); Oid parentIndexRelid = linitial_oid(ancestors); - DeleteInheritsTuple(oldIndexId, parentIndexRelid); + DeleteInheritsTuple(oldIndexId, parentIndexRelid, false, NULL); StoreSingleInheritance(newIndexId, parentIndexRelid, 1); list_free(ancestors); @@ -2241,7 +2241,7 @@ index_drop(Oid indexId, bool concurrent, bool concurrent_lock_mode) /* * fix INHERITS relation */ - DeleteInheritsTuple(indexId, InvalidOid); + DeleteInheritsTuple(indexId, InvalidOid, false, NULL); /* * We are presently too lazy to attempt to compute the new correct value diff --git a/src/backend/catalog/pg_inherits.c b/src/backend/catalog/pg_inherits.c index 17f37eb39f..f143db0ad4 100644 --- a/src/backend/catalog/pg_inherits.c +++ b/src/backend/catalog/pg_inherits.c @@ -52,7 +52,8 @@ typedef struct SeenRelsEntry * against possible DROPs of child relations. */ List * -find_inheritance_children(Oid parentrelId, LOCKMODE lockmode) +find_inheritance_children(Oid parentrelId, bool include_detached, + LOCKMODE lockmode) { List *list = NIL; Relation relation; @@ -91,7 +92,13 @@ find_inheritance_children(Oid parentrelId, LOCKMODE lockmode) while ((inheritsTuple = systable_getnext(scan)) != NULL) { + /* skip detached at caller's request */ + if (((Form_pg_inherits) GETSTRUCT(inheritsTuple))->inhdetached && + !include_detached) + continue; + inhrelid = ((Form_pg_inherits) GETSTRUCT(inheritsTuple))->inhrelid; + if (numoids >= maxoids) { maxoids *= 2; @@ -160,6 +167,9 @@ find_inheritance_children(Oid parentrelId, LOCKMODE lockmode) * given rel; caller should already have locked it). If lockmode is NoLock * 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. */ List * find_all_inheritors(Oid parentrelId, LOCKMODE lockmode, List **numparents) @@ -200,7 +210,8 @@ 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(currentrel, false, + lockmode); /* * Add to the queue only those children not already seen. This avoids @@ -429,6 +440,7 @@ StoreSingleInheritance(Oid relationId, Oid parentOid, int32 seqNumber) values[Anum_pg_inherits_inhrelid - 1] = ObjectIdGetDatum(relationId); values[Anum_pg_inherits_inhparent - 1] = ObjectIdGetDatum(parentOid); values[Anum_pg_inherits_inhseqno - 1] = Int32GetDatum(seqNumber); + values[Anum_pg_inherits_inhdetached - 1] = BoolGetDatum(false); memset(nulls, 0, sizeof(nulls)); @@ -448,10 +460,21 @@ StoreSingleInheritance(Oid relationId, Oid parentOid, int32 seqNumber) * as InvalidOid, in which case all tuples matching inhrelid are deleted; * otherwise only delete tuples with the specified inhparent. * + * 'detached' is the expected state of the inhdetached flag. If the catalog + * row does not match that state, an error is raised. When used on + * partitions, the partition name must be passed, for possible error messages. + * + * If allow_detached is passed false, then an error is raised if the child is + * already marked detached. A name must be supplied in that case, for the + * error message. This should only be used with table partitions. + * XXX the name bit is pretty weird and problematic for non-partition cases; + * what if the flag check fails? + * * Returns whether at least one row was deleted. */ bool -DeleteInheritsTuple(Oid inhrelid, Oid inhparent) +DeleteInheritsTuple(Oid inhrelid, Oid inhparent, bool detached, + const char *childname) { bool found = false; Relation catalogRelation; @@ -478,6 +501,28 @@ DeleteInheritsTuple(Oid inhrelid, Oid inhparent) parent = ((Form_pg_inherits) GETSTRUCT(inheritsTuple))->inhparent; if (!OidIsValid(inhparent) || parent == inhparent) { + bool inhdetached; + + inhdetached = ((Form_pg_inherits) GETSTRUCT(inheritsTuple))->inhdetached; + + /* + * Raise error depending on state. This should only happen for + * partitions, but we have no way to cross-check. + */ + if (inhdetached && !detached) + ereport(ERROR, + (errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE), + errmsg("cannot detach partition \"%s\"", + childname), + errdetail("The partition is being detached concurrently or has an unfinished detach."), + errhint("Use ALTER TABLE ... DETACH PARTITION ... FINALIZE to complete the pending detach operation"))); + if (!inhdetached && detached) + ereport(ERROR, + (errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE), + errmsg("cannot complete detaching partition \"%s\"", + childname), + errdetail("There's no partial concurrent detach in progress."))); + CatalogTupleDelete(catalogRelation, &inheritsTuple->t_self); found = true; } diff --git a/src/backend/commands/indexcmds.c b/src/backend/commands/indexcmds.c index 2baca12c5f..d032fe4af3 100644 --- a/src/backend/commands/indexcmds.c +++ b/src/backend/commands/indexcmds.c @@ -1092,7 +1092,7 @@ DefineIndex(Oid relationId, */ if (partitioned && stmt->relation && !stmt->relation->inh) { - PartitionDesc pd = RelationGetPartitionDesc(rel); + PartitionDesc pd = RelationGetPartitionDesc(rel, false); if (pd->nparts != 0) flags |= INDEX_CREATE_INVALID; @@ -1149,7 +1149,7 @@ DefineIndex(Oid relationId, */ if (!stmt->relation || stmt->relation->inh) { - PartitionDesc partdesc = RelationGetPartitionDesc(rel); + PartitionDesc partdesc = RelationGetPartitionDesc(rel, false); int nparts = partdesc->nparts; Oid *part_oids = palloc(sizeof(Oid) * nparts); bool invalidate_parent = false; @@ -3550,6 +3550,7 @@ IndexSetParentIndex(Relation partitionIdx, Oid parentOid) values[Anum_pg_inherits_inhparent - 1] = ObjectIdGetDatum(parentOid); values[Anum_pg_inherits_inhseqno - 1] = Int32GetDatum(1); + values[Anum_pg_inherits_inhdetached - 1] = BoolGetDatum(false); memset(isnull, false, sizeof(isnull)); tuple = heap_form_tuple(RelationGetDescr(pg_inherits), diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c index 07f4f562ee..682aa6a015 100644 --- a/src/backend/commands/tablecmds.c +++ b/src/backend/commands/tablecmds.c @@ -541,7 +541,8 @@ static PartitionSpec *transformPartitionSpec(Relation rel, PartitionSpec *partsp static void ComputePartitionAttrs(ParseState *pstate, Relation rel, List *partParams, AttrNumber *partattrs, List **partexprs, Oid *partopclass, Oid *partcollation, char strategy); static void CreateInheritance(Relation child_rel, Relation parent_rel); -static void RemoveInheritance(Relation child_rel, Relation parent_rel); +static void RemoveInheritance(Relation child_rel, Relation parent_rel, + bool allow_detached); static ObjectAddress ATExecAttachPartition(List **wqueue, Relation rel, PartitionCmd *cmd); static void AttachPartitionEnsureIndexes(Relation rel, Relation attachrel); @@ -550,7 +551,12 @@ static void QueuePartitionConstraintValidation(List **wqueue, Relation scanrel, bool validate_default); static void CloneRowTriggersToPartition(Relation parent, Relation partition); static void DropClonedTriggersFromPartition(Oid partitionId); -static ObjectAddress ATExecDetachPartition(Relation rel, RangeVar *name); +static ObjectAddress ATExecDetachPartition(List **wqueue, AlteredTableInfo *tab, + Relation rel, RangeVar *name, + bool concurrent); +static void DetachPartitionFinalize(Relation rel, Relation partRel, + bool concurrent, Oid defaultPartOid); +static ObjectAddress ATExecDetachPartitionFinalize(Relation rel, RangeVar *name); static ObjectAddress ATExecAttachPartitionIdx(List **wqueue, Relation rel, RangeVar *name); static void validatePartitionedIndex(Relation partedIdx, Relation partedTbl); @@ -984,7 +990,8 @@ DefineRelation(CreateStmt *stmt, char relkind, Oid ownerId, * lock the partition so as to avoid a deadlock. */ defaultPartOid = - get_default_oid_from_partdesc(RelationGetPartitionDesc(parent)); + get_default_oid_from_partdesc(RelationGetPartitionDesc(parent, + false)); if (OidIsValid(defaultPartOid)) defaultRel = table_open(defaultPartOid, AccessExclusiveLock); @@ -3099,7 +3106,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, NoLock) != NIL) + find_inheritance_children(myrelid, false, NoLock) != NIL) ereport(ERROR, (errcode(ERRCODE_INVALID_TABLE_DEFINITION), errmsg("inherited column \"%s\" must be renamed in child tables too", @@ -3298,7 +3305,7 @@ rename_constraint_internal(Oid myrelid, else { if (expected_parents == 0 && - find_inheritance_children(myrelid, NoLock) != NIL) + find_inheritance_children(myrelid, false, NoLock) != NIL) ereport(ERROR, (errcode(ERRCODE_INVALID_TABLE_DEFINITION), errmsg("inherited constraint \"%s\" must be renamed in child tables too", @@ -3916,7 +3923,14 @@ AlterTableGetLockLevel(List *cmds) break; case AT_DetachPartition: - cmd_lockmode = AccessExclusiveLock; + if (((PartitionCmd *) cmd->def)->concurrent) + cmd_lockmode = ShareUpdateExclusiveLock; + else + cmd_lockmode = AccessExclusiveLock; + break; + + case AT_DetachPartitionFinalize: + cmd_lockmode = ShareUpdateExclusiveLock; break; case AT_CheckNotNull: @@ -4288,6 +4302,11 @@ ATPrepCmd(List **wqueue, Relation rel, AlterTableCmd *cmd, /* No command-specific prep needed */ pass = AT_PASS_MISC; break; + case AT_DetachPartitionFinalize: + ATSimplePermissions(rel, ATT_TABLE); + /* No command-specific prep needed */ + pass = AT_PASS_MISC; + break; default: /* oops */ elog(ERROR, "unrecognized alter table type: %d", (int) cmd->subtype); @@ -4663,7 +4682,12 @@ ATExecCmd(List **wqueue, AlteredTableInfo *tab, Assert(cmd != NULL); /* ATPrepCmd ensures it must be a table */ Assert(rel->rd_rel->relkind == RELKIND_PARTITIONED_TABLE); - ATExecDetachPartition(rel, ((PartitionCmd *) cmd->def)->name); + ATExecDetachPartition(wqueue, tab, rel, + ((PartitionCmd *) cmd->def)->name, + ((PartitionCmd *) cmd->def)->concurrent); + break; + case AT_DetachPartitionFinalize: + ATExecDetachPartitionFinalize(rel, ((PartitionCmd *) cmd->def)->name); break; default: /* oops */ elog(ERROR, "unrecognized alter table type: %d", @@ -6081,7 +6105,7 @@ ATExecAddColumn(List **wqueue, AlteredTableInfo *tab, Relation rel, */ if (colDef->identity && recurse && - find_inheritance_children(myrelid, NoLock) != NIL) + find_inheritance_children(myrelid, false, NoLock) != NIL) ereport(ERROR, (errcode(ERRCODE_INVALID_TABLE_DEFINITION), errmsg("cannot recursively add identity column to table that has child tables"))); @@ -6314,7 +6338,8 @@ ATExecAddColumn(List **wqueue, AlteredTableInfo *tab, Relation rel, * routines, we have to do this one level of recursion at a time; we can't * use find_all_inheritors to do it in one pass. */ - children = find_inheritance_children(RelationGetRelid(rel), lockmode); + children = + find_inheritance_children(RelationGetRelid(rel), false, lockmode); /* * If we are told not to recurse, there had better not be any child @@ -6468,7 +6493,7 @@ ATPrepDropNotNull(Relation rel, bool recurse, bool recursing) */ if (rel->rd_rel->relkind == RELKIND_PARTITIONED_TABLE) { - PartitionDesc partdesc = RelationGetPartitionDesc(rel); + PartitionDesc partdesc = RelationGetPartitionDesc(rel, false); Assert(partdesc != NULL); if (partdesc->nparts > 0 && !recurse && !recursing) @@ -7680,7 +7705,8 @@ ATExecDropColumn(List **wqueue, Relation rel, const char *colName, * routines, we have to do this one level of recursion at a time; we can't * use find_all_inheritors to do it in one pass. */ - children = find_inheritance_children(RelationGetRelid(rel), lockmode); + children = + find_inheritance_children(RelationGetRelid(rel), false, lockmode); if (children) { @@ -8144,7 +8170,8 @@ ATAddCheckConstraint(List **wqueue, AlteredTableInfo *tab, Relation rel, * routines, we have to do this one level of recursion at a time; we can't * use find_all_inheritors to do it in one pass. */ - children = find_inheritance_children(RelationGetRelid(rel), lockmode); + children = + find_inheritance_children(RelationGetRelid(rel), false, lockmode); /* * Check if ONLY was specified with ALTER TABLE. If so, allow the @@ -8759,7 +8786,7 @@ addFkRecurseReferenced(List **wqueue, Constraint *fkconstraint, Relation rel, */ if (pkrel->rd_rel->relkind == RELKIND_PARTITIONED_TABLE) { - PartitionDesc pd = RelationGetPartitionDesc(pkrel); + PartitionDesc pd = RelationGetPartitionDesc(pkrel, false); for (int i = 0; i < pd->nparts; i++) { @@ -8893,7 +8920,7 @@ addFkRecurseReferencing(List **wqueue, Constraint *fkconstraint, Relation rel, } else if (rel->rd_rel->relkind == RELKIND_PARTITIONED_TABLE) { - PartitionDesc pd = RelationGetPartitionDesc(rel); + PartitionDesc pd = RelationGetPartitionDesc(rel, false); /* * Recurse to take appropriate action on each partition; either we @@ -10674,7 +10701,8 @@ 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), lockmode); + children = + find_inheritance_children(RelationGetRelid(rel), false, lockmode); else children = NIL; @@ -11058,7 +11086,8 @@ ATPrepAlterColumnType(List **wqueue, } } else if (!recursing && - find_inheritance_children(RelationGetRelid(rel), NoLock) != NIL) + find_inheritance_children(RelationGetRelid(rel), false, + NoLock) != NIL) ereport(ERROR, (errcode(ERRCODE_INVALID_TABLE_DEFINITION), errmsg("type of inherited column \"%s\" must be changed in child tables too", @@ -13953,7 +13982,7 @@ ATExecDropInherit(Relation rel, RangeVar *parent, LOCKMODE lockmode) */ /* Off to RemoveInheritance() where most of the work happens */ - RemoveInheritance(rel, parent_rel); + RemoveInheritance(rel, parent_rel, false); ObjectAddressSet(address, RelationRelationId, RelationGetRelid(parent_rel)); @@ -13964,12 +13993,70 @@ ATExecDropInherit(Relation rel, RangeVar *parent, LOCKMODE lockmode) return address; } +/* + * MarkInheritDetached + * + * When a partition is detached from its parent concurrently, we don't + * remove the pg_inherits row until a second transaction; as a preparatory + * step, this function marks the entry as 'detached', so that other + * transactions know to ignore the partition. + */ +static void +MarkInheritDetached(Relation child_rel, Relation parent_rel) +{ + Relation catalogRelation; + SysScanDesc scan; + ScanKeyData key; + HeapTuple inheritsTuple; + bool found = false; + + /* + * Find pg_inherits entries by inhrelid. + */ + catalogRelation = table_open(InheritsRelationId, RowExclusiveLock); + ScanKeyInit(&key, + Anum_pg_inherits_inhrelid, + BTEqualStrategyNumber, F_OIDEQ, + ObjectIdGetDatum(RelationGetRelid(child_rel))); + scan = systable_beginscan(catalogRelation, InheritsRelidSeqnoIndexId, + true, NULL, 1, &key); + + while (HeapTupleIsValid(inheritsTuple = systable_getnext(scan))) + { + HeapTuple newtup; + + if (((Form_pg_inherits) GETSTRUCT(inheritsTuple))->inhparent != + RelationGetRelid(parent_rel)) + elog(ERROR, "bad parent tuple found for partition %u", + RelationGetRelid(child_rel)); + + newtup = heap_copytuple(inheritsTuple); + ((Form_pg_inherits) GETSTRUCT(newtup))->inhdetached = true; + + CatalogTupleUpdate(catalogRelation, + &inheritsTuple->t_self, + newtup); + found = true; + } + + /* Done */ + systable_endscan(scan); + table_close(catalogRelation, RowExclusiveLock); + + if (!found) + ereport(ERROR, + (errcode(ERRCODE_UNDEFINED_TABLE), + errmsg("relation \"%s\" is not a partition of relation \"%s\"", + RelationGetRelationName(child_rel), + RelationGetRelationName(parent_rel)))); +} + /* * RemoveInheritance * * Drop a parent from the child's parents. This just adjusts the attinhcount * and attislocal of the columns and removes the pg_inherit and pg_depend - * entries. + * entries. expect_detached is passed down to DeleteInheritsTuple, q.v.. * * If attinhcount goes to 0 then attislocal gets set to true. If it goes back * up attislocal stays true, which means if a child is ever removed from a @@ -13983,7 +14070,7 @@ ATExecDropInherit(Relation rel, RangeVar *parent, LOCKMODE lockmode) * Common to ATExecDropInherit() and ATExecDetachPartition(). */ static void -RemoveInheritance(Relation child_rel, Relation parent_rel) +RemoveInheritance(Relation child_rel, Relation parent_rel, bool expect_detached) { Relation catalogRelation; SysScanDesc scan; @@ -13999,7 +14086,9 @@ RemoveInheritance(Relation child_rel, Relation parent_rel) child_is_partition = true; found = DeleteInheritsTuple(RelationGetRelid(child_rel), - RelationGetRelid(parent_rel)); + RelationGetRelid(parent_rel), + expect_detached, + RelationGetRelationName(child_rel)); if (!found) { if (child_is_partition) @@ -16107,7 +16196,7 @@ QueuePartitionConstraintValidation(List **wqueue, Relation scanrel, } else if (scanrel->rd_rel->relkind == RELKIND_PARTITIONED_TABLE) { - PartitionDesc partdesc = RelationGetPartitionDesc(scanrel); + PartitionDesc partdesc = RelationGetPartitionDesc(scanrel, false); int i; for (i = 0; i < partdesc->nparts; i++) @@ -16163,7 +16252,7 @@ ATExecAttachPartition(List **wqueue, Relation rel, PartitionCmd *cmd) * new partition will change its partition constraint. */ defaultPartOid = - get_default_oid_from_partdesc(RelationGetPartitionDesc(rel)); + get_default_oid_from_partdesc(RelationGetPartitionDesc(rel, false)); if (OidIsValid(defaultPartOid)) LockRelationOid(defaultPartOid, AccessExclusiveLock); @@ -16751,105 +16840,226 @@ CloneRowTriggersToPartition(Relation parent, Relation partition) * ALTER TABLE DETACH PARTITION * * Return the address of the relation that is no longer a partition of rel. + * + * If concurrent mode is requested, we run in two transactions. A side- + * effect is that this command cannot run in a multi-part ALTER TABLE. + * Currently, that's enforced by the grammar. + * + * The strategy for concurrency is to first modify the partition catalog + * rows to make it visible to everyone that the partition is detached, + * lock the partition against writes, and commit the transaction; anyone + * who requests the partition descriptor from that point onwards has to + * ignore such a partition. In a second transaction, we wait until all + * transactions that could have seen the partition as attached are gone, + * then we remove the rest of partition metadata (pg_inherits and + * pg_class.relpartbounds). */ static ObjectAddress -ATExecDetachPartition(Relation rel, RangeVar *name) +ATExecDetachPartition(List **wqueue, AlteredTableInfo *tab, Relation rel, + RangeVar *name, bool concurrent) { - Relation partRel, - classRel; - HeapTuple tuple, - newtuple; - Datum new_val[Natts_pg_class]; - bool new_null[Natts_pg_class], - new_repl[Natts_pg_class]; + Relation partRel; ObjectAddress address; Oid defaultPartOid; - List *indexes; - List *fks; - ListCell *cell; /* * We must lock the default partition, because detaching this partition * will change its partition constraint. */ defaultPartOid = - get_default_oid_from_partdesc(RelationGetPartitionDesc(rel)); - if (OidIsValid(defaultPartOid)) - LockRelationOid(defaultPartOid, AccessExclusiveLock); - - partRel = table_openrv(name, ShareUpdateExclusiveLock); - - /* Ensure that foreign keys still hold after this detach */ - ATDetachCheckNoForeignKeyRefs(partRel); - - /* All inheritance related checks are performed within the function */ - RemoveInheritance(partRel, rel); - - /* Update pg_class tuple */ - classRel = table_open(RelationRelationId, RowExclusiveLock); - tuple = SearchSysCacheCopy1(RELOID, - ObjectIdGetDatum(RelationGetRelid(partRel))); - if (!HeapTupleIsValid(tuple)) - elog(ERROR, "cache lookup failed for relation %u", - RelationGetRelid(partRel)); - Assert(((Form_pg_class) GETSTRUCT(tuple))->relispartition); - - /* Clear relpartbound and reset relispartition */ - memset(new_val, 0, sizeof(new_val)); - memset(new_null, false, sizeof(new_null)); - memset(new_repl, false, sizeof(new_repl)); - new_val[Anum_pg_class_relpartbound - 1] = (Datum) 0; - new_null[Anum_pg_class_relpartbound - 1] = true; - new_repl[Anum_pg_class_relpartbound - 1] = true; - newtuple = heap_modify_tuple(tuple, RelationGetDescr(classRel), - new_val, new_null, new_repl); - - ((Form_pg_class) GETSTRUCT(newtuple))->relispartition = false; - CatalogTupleUpdate(classRel, &newtuple->t_self, newtuple); - heap_freetuple(newtuple); - + get_default_oid_from_partdesc(RelationGetPartitionDesc(rel, false)); if (OidIsValid(defaultPartOid)) { /* - * If the relation being detached is the default partition itself, - * remove it from the parent's pg_partitioned_table entry. + * Concurrent detaching when a default partition exists is not + * supported. The main problem is that the default partition + * constraint would change. And there's a definitional problem: what + * should happen to the tuples that are being inserted that belong to + * the partition being detached? Putting them on the partition being + * detached would be wrong, since they'd become "lost" after the but + * we cannot put them in the default partition either until we alter + * its partition constraint. * - * If not, we must invalidate default partition's relcache entry, as - * in StorePartitionBound: its partition constraint depends on every - * other partition's partition constraint. + * I think we could solve this problem if we effected the constraint + * change before committing the first transaction. But the lock would + * have to remain AEL and it would cause concurrent query planning to + * be blocked, so changing it that way would be even worse. */ - if (RelationGetRelid(partRel) == defaultPartOid) - update_default_partition_oid(RelationGetRelid(rel), InvalidOid); - else - CacheInvalidateRelcacheByRelid(defaultPartOid); + if (concurrent) + ereport(ERROR, + (errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE), + errmsg("cannot detach partitions concurrently when a default partition exists"))); + LockRelationOid(defaultPartOid, AccessExclusiveLock); } - /* detach indexes too */ - indexes = RelationGetIndexList(partRel); - foreach(cell, indexes) + /* + * In concurrent mode, the partition is locked with + * ShareUpdateExclusiveLock in the first stage. + */ + partRel = table_openrv(name, concurrent ? + ShareUpdateExclusiveLock : AccessExclusiveLock); + + /* + * Check inheritance conditions and either delete the pg_inherits row + * (in non-concurrent mode) or just set the inhisdetached flag. + */ + if (!concurrent) + RemoveInheritance(partRel, rel, false); + else + MarkInheritDetached(partRel, rel); + + /* + * Ensure that foreign keys still hold after this detach. This keeps lock + * on the referencing tables, which prevent concurrent transactions from + * adding rows that we wouldn't see. For this to work in concurrent mode, + * it is critical that the partition appears as no longer attached for the + * RI queries as soon as the first transaction commits. + */ + ATDetachCheckNoForeignKeyRefs(partRel); + + /* + * Concurrent mode has to work harder; first we add a new constraint to the + * partition that matches the partition constraint. The reason for this is + * that the planner may have made optimizations that depend on the + * constraint. XXX Isn't it sufficient to invalidate the partition's + * relcache entry? + * + * Then we close our existing transaction, and in a new one wait for + * all process to catch up on the catalog updates we've done so far; at + * that point we can complete the operation. + */ + if (concurrent) { - Oid idxid = lfirst_oid(cell); - Relation idx; - Oid constrOid; + Oid partrelid, + parentrelid; + Constraint *n; + LOCKTAG tag; + char *parentrelname; + char *partrelname; - if (!has_superclass(idxid)) - continue; + /* Add constraint on partition, equivalent to the partition constraint */ + n = makeNode(Constraint); + n->contype = CONSTR_CHECK; + n->conname = NULL; + n->location = -1; + n->is_no_inherit = false; + n->raw_expr = NULL; + n->cooked_expr = nodeToString(make_ands_explicit(RelationGetPartitionQual(partRel))); + n->initially_valid = true; + n->skip_validation = true; + /* It's a re-add, since it nominally already exists */ + ATAddCheckConstraint(wqueue, tab, partRel, n, + true, false, true, ShareUpdateExclusiveLock); - Assert((IndexGetRelation(get_partition_parent(idxid), false) == - RelationGetRelid(rel))); + /* + * We're almost done now; the only traces that remain are the + * pg_inherits tuple and the partition's relpartbounds. Before we can + * remove those, we need to wait until all transactions that know that + * this is a partition are gone. + */ - idx = index_open(idxid, AccessExclusiveLock); - IndexSetParentIndex(idx, InvalidOid); + /* + * Remember relation OIDs to re-acquire them later; and relation names + * too, for error messages if something is dropped in between. + */ + partrelid = RelationGetRelid(partRel); + parentrelid = RelationGetRelid(rel); + parentrelname = MemoryContextStrdup(PortalContext, + RelationGetRelationName(rel)); + partrelname = MemoryContextStrdup(PortalContext, + RelationGetRelationName(partRel)); - /* If there's a constraint associated with the index, detach it too */ - constrOid = get_relation_idx_constraint_oid(RelationGetRelid(partRel), - idxid); - if (OidIsValid(constrOid)) - ConstraintSetParentConstraint(constrOid, InvalidOid, InvalidOid); + /* Invalidate relcache entries for the parent -- must be before close */ + CacheInvalidateRelcache(rel); + + table_close(partRel, NoLock); + table_close(rel, NoLock); + tab->rel = NULL; + + /* Make updated catalog entry visible */ + PopActiveSnapshot(); + CommitTransactionCommand(); + + StartTransactionCommand(); + + /* + * Now wait. This ensures that all queries that were planned including + * the partition are finished before we remove the rest of catalog + * entries. We don't need or indeed want to acquire this lock, though + * -- that would block later queries. + * + * We don't need to concern ourselves with waiting for a lock the + * partition itself, since we will acquire AccessExclusiveLock below. + */ + SET_LOCKTAG_RELATION(tag, MyDatabaseId, parentrelid); + WaitForLockersMultiple(list_make1(&tag), AccessExclusiveLock, false); + + /* + * Now acquire locks in both relations again. Note they may have been + * removed in the meantime, so care is required. + */ + rel = try_relation_open(parentrelid, ShareUpdateExclusiveLock); + partRel = try_relation_open(partrelid, AccessExclusiveLock); + + /* If the relations aren't there, something bad happened; bail out */ + if (rel == NULL) + { + if (partRel != NULL) /* shouldn't happen */ + elog(WARNING, "dangling partition \"%s\" remains, can't fix", + partrelname); + ereport(ERROR, + (errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE), + errmsg("partitioned table \"%s\" was removed concurrently", + parentrelname))); + } + if (partRel == NULL) + ereport(ERROR, + (errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE), + errmsg("partition \"%s\" was removed concurrently", partrelname))); + + tab->rel = rel; - index_close(idx, NoLock); } - table_close(classRel, RowExclusiveLock); + + /* Do the final part of detaching */ + DetachPartitionFinalize(rel, partRel, concurrent, defaultPartOid); + + ObjectAddressSet(address, RelationRelationId, RelationGetRelid(partRel)); + + /* keep our lock until commit */ + table_close(partRel, NoLock); + + return address; +} + +/* + * Second part of ALTER TABLE .. DETACH. + * + * This is separate so that it can be run independently when the second + * transaction of the concurrent algorithm fails (crash or abort). + */ +static void +DetachPartitionFinalize(Relation rel, Relation partRel, bool concurrent, + Oid defaultPartOid) +{ + Relation classRel; + List *fks; + ListCell *cell; + List *indexes; + Datum new_val[Natts_pg_class]; + bool new_null[Natts_pg_class], + new_repl[Natts_pg_class]; + HeapTuple tuple, + newtuple; + + if (concurrent) + { + /* + * We can remove the pg_inherits row now. (In the non-concurrent case, + * this was already done). + */ + RemoveInheritance(partRel, rel, true); + } /* Drop any triggers that were cloned on creation/attach. */ DropClonedTriggersFromPartition(RelationGetRelid(partRel)); @@ -16922,17 +17132,98 @@ ATExecDetachPartition(Relation rel, RangeVar *name) ObjectAddressSet(constraint, ConstraintRelationId, constrOid); performDeletion(&constraint, DROP_RESTRICT, 0); } - CommandCounterIncrement(); + + /* Now we can detach indexes */ + indexes = RelationGetIndexList(partRel); + foreach(cell, indexes) + { + Oid idxid = lfirst_oid(cell); + Relation idx; + Oid constrOid; + + if (!has_superclass(idxid)) + continue; + + Assert((IndexGetRelation(get_partition_parent(idxid), false) == + RelationGetRelid(rel))); + + idx = index_open(idxid, AccessExclusiveLock); + IndexSetParentIndex(idx, InvalidOid); + + /* If there's a constraint associated with the index, detach it too */ + constrOid = get_relation_idx_constraint_oid(RelationGetRelid(partRel), + idxid); + if (OidIsValid(constrOid)) + ConstraintSetParentConstraint(constrOid, InvalidOid, InvalidOid); + + index_close(idx, NoLock); + } + + /* Update pg_class tuple */ + classRel = table_open(RelationRelationId, RowExclusiveLock); + tuple = SearchSysCacheCopy1(RELOID, + ObjectIdGetDatum(RelationGetRelid(partRel))); + if (!HeapTupleIsValid(tuple)) + elog(ERROR, "cache lookup failed for relation %u", + RelationGetRelid(partRel)); + Assert(((Form_pg_class) GETSTRUCT(tuple))->relispartition); + + /* Clear relpartbound and reset relispartition */ + memset(new_val, 0, sizeof(new_val)); + memset(new_null, false, sizeof(new_null)); + memset(new_repl, false, sizeof(new_repl)); + new_val[Anum_pg_class_relpartbound - 1] = (Datum) 0; + new_null[Anum_pg_class_relpartbound - 1] = true; + new_repl[Anum_pg_class_relpartbound - 1] = true; + newtuple = heap_modify_tuple(tuple, RelationGetDescr(classRel), + new_val, new_null, new_repl); + + ((Form_pg_class) GETSTRUCT(newtuple))->relispartition = false; + CatalogTupleUpdate(classRel, &newtuple->t_self, newtuple); + heap_freetuple(newtuple); + table_close(classRel, RowExclusiveLock); + + if (OidIsValid(defaultPartOid)) + { + /* + * If the relation being detached is the default partition itself, + * remove it from the parent's pg_partitioned_table entry. + * + * If not, we must invalidate default partition's relcache entry, as + * in StorePartitionBound: its partition constraint depends on every + * other partition's partition constraint. + */ + if (RelationGetRelid(partRel) == defaultPartOid) + update_default_partition_oid(RelationGetRelid(rel), InvalidOid); + else + CacheInvalidateRelcacheByRelid(defaultPartOid); + } /* * Invalidate the parent's relcache so that the partition is no longer * included in its partition descriptor. */ CacheInvalidateRelcache(rel); +} + +/* + * ALTER TABLE ... DETACH PARTITION ... FINALIZE + * + * To use when a DETACH PARTITION command previously did not run to + * completion; this completes the detaching process. + */ +static ObjectAddress +ATExecDetachPartitionFinalize(Relation rel, RangeVar *name) +{ + Relation partRel; + ObjectAddress address; + + partRel = table_openrv(name, AccessExclusiveLock); + + DetachPartitionFinalize(rel, partRel, true, InvalidOid); ObjectAddressSet(address, RelationRelationId, RelationGetRelid(partRel)); - /* keep our lock until commit */ table_close(partRel, NoLock); return address; @@ -17133,7 +17424,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); + partDesc = RelationGetPartitionDesc(parentTbl, false); found = false; for (i = 0; i < partDesc->nparts; i++) { @@ -17287,7 +17578,7 @@ validatePartitionedIndex(Relation partedIdx, Relation partedTbl) * If we found as many inherited indexes as the partitioned table has * partitions, we're good; update pg_index to set indisvalid. */ - if (tuples == RelationGetPartitionDesc(partedTbl)->nparts) + if (tuples == RelationGetPartitionDesc(partedTbl, false)->nparts) { Relation idxRel; HeapTuple newtup; diff --git a/src/backend/commands/trigger.c b/src/backend/commands/trigger.c index 672fccff5b..ce65abd57e 100644 --- a/src/backend/commands/trigger.c +++ b/src/backend/commands/trigger.c @@ -1054,7 +1054,7 @@ CreateTrigger(CreateTrigStmt *stmt, const char *queryString, */ if (partition_recurse) { - PartitionDesc partdesc = RelationGetPartitionDesc(rel); + PartitionDesc partdesc = RelationGetPartitionDesc(rel, false); List *idxs = NIL; List *childTbls = NIL; ListCell *l; @@ -1076,7 +1076,8 @@ CreateTrigger(CreateTrigStmt *stmt, const char *queryString, ListCell *l; List *idxs = NIL; - idxs = find_inheritance_children(indexOid, ShareRowExclusiveLock); + idxs = find_inheritance_children(indexOid, false, + ShareRowExclusiveLock); foreach(l, idxs) childTbls = lappend_oid(childTbls, IndexGetRelation(lfirst_oid(l), @@ -1537,7 +1538,7 @@ EnableDisableTrigger(Relation rel, const char *tgname, if (rel->rd_rel->relkind == RELKIND_PARTITIONED_TABLE && (TRIGGER_FOR_ROW(oldtrig->tgtype))) { - PartitionDesc partdesc = RelationGetPartitionDesc(rel); + PartitionDesc partdesc = RelationGetPartitionDesc(rel, false); int i; for (i = 0; i < partdesc->nparts; i++) diff --git a/src/backend/executor/execPartition.c b/src/backend/executor/execPartition.c index fb6ce49056..ef4fd7bf90 100644 --- a/src/backend/executor/execPartition.c +++ b/src/backend/executor/execPartition.c @@ -507,6 +507,7 @@ ExecInitPartitionInfo(ModifyTableState *mtstate, EState *estate, int partidx) { ModifyTable *node = (ModifyTable *) mtstate->ps.plan; + Oid partOid = dispatch->partdesc->oids[partidx]; Relation rootrel = rootResultRelInfo->ri_RelationDesc, partrel; Relation firstResultRel = mtstate->resultRelInfo[0].ri_RelationDesc; @@ -517,7 +518,7 @@ ExecInitPartitionInfo(ModifyTableState *mtstate, EState *estate, oldcxt = MemoryContextSwitchTo(proute->memcxt); - partrel = table_open(dispatch->partdesc->oids[partidx], RowExclusiveLock); + partrel = table_open(partOid, RowExclusiveLock); leaf_part_rri = makeNode(ResultRelInfo); InitResultRelInfo(leaf_part_rri, @@ -997,7 +998,7 @@ ExecInitPartitionDispatchInfo(EState *estate, if (estate->es_partition_directory == NULL) estate->es_partition_directory = - CreatePartitionDirectory(estate->es_query_cxt); + CreatePartitionDirectory(estate->es_query_cxt, true); oldcxt = MemoryContextSwitchTo(proute->memcxt); @@ -1559,7 +1560,7 @@ ExecCreatePartitionPruneState(PlanState *planstate, if (estate->es_partition_directory == NULL) estate->es_partition_directory = - CreatePartitionDirectory(estate->es_query_cxt); + CreatePartitionDirectory(estate->es_query_cxt, true); n_part_hierarchies = list_length(partitionpruneinfo->prune_infos); Assert(n_part_hierarchies > 0); @@ -1625,9 +1626,12 @@ ExecCreatePartitionPruneState(PlanState *planstate, partrel); /* - * Initialize the subplan_map and subpart_map. Since detaching a - * partition requires AccessExclusiveLock, no partitions can have - * disappeared, nor can the bounds for any partition have changed. + * Initialize the subplan_map and subpart_map. + * + * Because we request detached partitions to be included, and + * detaching waits for old transactions, it is safe to assume that + * no partitions have disappeared since this query was planned. + * * However, new partitions may have been added. */ Assert(partdesc->nparts >= pinfo->nparts); diff --git a/src/backend/nodes/copyfuncs.c b/src/backend/nodes/copyfuncs.c index 89c409de66..f48350bc2f 100644 --- a/src/backend/nodes/copyfuncs.c +++ b/src/backend/nodes/copyfuncs.c @@ -4670,6 +4670,7 @@ _copyPartitionCmd(const PartitionCmd *from) COPY_NODE_FIELD(name); COPY_NODE_FIELD(bound); + COPY_SCALAR_FIELD(concurrent); return newnode; } diff --git a/src/backend/nodes/equalfuncs.c b/src/backend/nodes/equalfuncs.c index e3f33c40be..01ff684f90 100644 --- a/src/backend/nodes/equalfuncs.c +++ b/src/backend/nodes/equalfuncs.c @@ -2932,6 +2932,7 @@ _equalPartitionCmd(const PartitionCmd *a, const PartitionCmd *b) { COMPARE_NODE_FIELD(name); COMPARE_NODE_FIELD(bound); + COMPARE_SCALAR_FIELD(concurrent); return true; } diff --git a/src/backend/optimizer/util/plancat.c b/src/backend/optimizer/util/plancat.c index 25545029d7..75fe783b41 100644 --- a/src/backend/optimizer/util/plancat.c +++ b/src/backend/optimizer/util/plancat.c @@ -2129,7 +2129,7 @@ set_relation_partition_info(PlannerInfo *root, RelOptInfo *rel, /* Create the PartitionDirectory infrastructure if we didn't already */ if (root->glob->partition_directory == NULL) root->glob->partition_directory = - CreatePartitionDirectory(CurrentMemoryContext); + CreatePartitionDirectory(CurrentMemoryContext, false); partdesc = PartitionDirectoryLookup(root->glob->partition_directory, relation); diff --git a/src/backend/parser/gram.y b/src/backend/parser/gram.y index dbb47d4982..da0b233695 100644 --- a/src/backend/parser/gram.y +++ b/src/backend/parser/gram.y @@ -649,7 +649,7 @@ static Node *makeRecursiveViewSelect(char *relname, List *aliases, Node *query); EXCLUDE EXCLUDING EXCLUSIVE EXECUTE EXISTS EXPLAIN EXPRESSION EXTENSION EXTERNAL EXTRACT - FALSE_P FAMILY FETCH FILTER FIRST_P FLOAT_P FOLLOWING FOR + FALSE_P FAMILY FETCH FILTER FINALIZE FIRST_P FLOAT_P FOLLOWING FOR FORCE FOREIGN FORWARD FREEZE FROM FULL FUNCTION FUNCTIONS GENERATED GLOBAL GRANT GRANTED GREATEST GROUP_P GROUPING GROUPS @@ -2057,12 +2057,13 @@ partition_cmd: n->subtype = AT_AttachPartition; cmd->name = $3; cmd->bound = $4; + cmd->concurrent = false; n->def = (Node *) cmd; $$ = (Node *) n; } - /* ALTER TABLE <name> DETACH PARTITION <partition_name> */ - | DETACH PARTITION qualified_name + /* ALTER TABLE <name> DETACH PARTITION <partition_name> [CONCURRENTLY] */ + | DETACH PARTITION qualified_name opt_concurrently { AlterTableCmd *n = makeNode(AlterTableCmd); PartitionCmd *cmd = makeNode(PartitionCmd); @@ -2070,8 +2071,21 @@ partition_cmd: n->subtype = AT_DetachPartition; cmd->name = $3; cmd->bound = NULL; + cmd->concurrent = $4; n->def = (Node *) cmd; + $$ = (Node *) n; + } + | DETACH PARTITION qualified_name FINALIZE + { + AlterTableCmd *n = makeNode(AlterTableCmd); + PartitionCmd *cmd = makeNode(PartitionCmd); + + n->subtype = AT_DetachPartitionFinalize; + cmd->name = $3; + cmd->bound = NULL; + cmd->concurrent = false; + n->def = (Node *) cmd; $$ = (Node *) n; } ; @@ -2086,6 +2100,7 @@ index_partition_cmd: n->subtype = AT_AttachPartition; cmd->name = $3; cmd->bound = NULL; + cmd->concurrent = false; n->def = (Node *) cmd; $$ = (Node *) n; @@ -15110,6 +15125,7 @@ unreserved_keyword: | EXTERNAL | FAMILY | FILTER + | FINALIZE | FIRST_P | FOLLOWING | FORCE diff --git a/src/backend/partitioning/partbounds.c b/src/backend/partitioning/partbounds.c index 7553d55987..966d4bbe7f 100644 --- a/src/backend/partitioning/partbounds.c +++ b/src/backend/partitioning/partbounds.c @@ -2810,7 +2810,7 @@ check_new_partition_bound(char *relname, Relation parent, PartitionBoundSpec *spec) { PartitionKey key = RelationGetPartitionKey(parent); - PartitionDesc partdesc = RelationGetPartitionDesc(parent); + PartitionDesc partdesc = RelationGetPartitionDesc(parent, true); PartitionBoundInfo boundinfo = partdesc->boundinfo; ParseState *pstate = make_parsestate(NULL); int with = -1; @@ -3984,7 +3984,7 @@ get_qual_for_list(Relation parent, PartitionBoundSpec *spec) { int i; int ndatums = 0; - PartitionDesc pdesc = RelationGetPartitionDesc(parent); + PartitionDesc pdesc = RelationGetPartitionDesc(parent, true); /* XXX correct? */ PartitionBoundInfo boundinfo = pdesc->boundinfo; if (boundinfo) @@ -4184,7 +4184,7 @@ get_qual_for_range(Relation parent, PartitionBoundSpec *spec, if (spec->is_default) { List *or_expr_args = NIL; - PartitionDesc pdesc = RelationGetPartitionDesc(parent); + PartitionDesc pdesc = RelationGetPartitionDesc(parent, true); /* XXX correct? */ Oid *inhoids = pdesc->oids; int nparts = pdesc->nparts, i; diff --git a/src/backend/partitioning/partdesc.c b/src/backend/partitioning/partdesc.c index 0f124a5254..3c21f42ea9 100644 --- a/src/backend/partitioning/partdesc.c +++ b/src/backend/partitioning/partdesc.c @@ -38,6 +38,7 @@ typedef struct PartitionDirectoryData { MemoryContext pdir_mcxt; HTAB *pdir_hash; + bool include_detached; } PartitionDirectoryData; typedef struct PartitionDirectoryEntry @@ -47,7 +48,7 @@ typedef struct PartitionDirectoryEntry PartitionDesc pd; } PartitionDirectoryEntry; -static void RelationBuildPartitionDesc(Relation rel); +static void RelationBuildPartitionDesc(Relation rel, bool include_detached); /* @@ -62,13 +63,14 @@ static void RelationBuildPartitionDesc(Relation rel); * that the data doesn't become stale. */ PartitionDesc -RelationGetPartitionDesc(Relation rel) +RelationGetPartitionDesc(Relation rel, bool include_detached) { if (rel->rd_rel->relkind != RELKIND_PARTITIONED_TABLE) return NULL; - if (unlikely(rel->rd_partdesc == NULL)) - RelationBuildPartitionDesc(rel); + if (unlikely(rel->rd_partdesc == NULL || + rel->rd_partdesc->includes_detached != include_detached)) + RelationBuildPartitionDesc(rel, include_detached); return rel->rd_partdesc; } @@ -89,7 +91,7 @@ RelationGetPartitionDesc(Relation rel) * permanently. */ static void -RelationBuildPartitionDesc(Relation rel) +RelationBuildPartitionDesc(Relation rel, bool include_detached) { PartitionDesc partdesc; PartitionBoundInfo boundinfo = NULL; @@ -111,7 +113,8 @@ RelationBuildPartitionDesc(Relation rel) * concurrently, whatever this function returns will be accurate as of * some well-defined point in time. */ - inhoids = find_inheritance_children(RelationGetRelid(rel), NoLock); + inhoids = find_inheritance_children(RelationGetRelid(rel), NoLock, + include_detached); nparts = list_length(inhoids); /* Allocate working arrays for OIDs, leaf flags, and boundspecs. */ @@ -239,6 +242,7 @@ RelationBuildPartitionDesc(Relation rel) partdesc->boundinfo = partition_bounds_copy(boundinfo, key); partdesc->oids = (Oid *) palloc(nparts * sizeof(Oid)); partdesc->is_leaf = (bool *) palloc(nparts * sizeof(bool)); + partdesc->includes_detached = include_detached; /* * Assign OIDs from the original array into mapped indexes of the @@ -281,7 +285,7 @@ RelationBuildPartitionDesc(Relation rel) * Create a new partition directory object. */ PartitionDirectory -CreatePartitionDirectory(MemoryContext mcxt) +CreatePartitionDirectory(MemoryContext mcxt, bool include_detached) { MemoryContext oldcontext = MemoryContextSwitchTo(mcxt); PartitionDirectory pdir; @@ -296,6 +300,7 @@ CreatePartitionDirectory(MemoryContext mcxt) pdir->pdir_mcxt = mcxt; pdir->pdir_hash = hash_create("partition directory", 256, &ctl, HASH_ELEM | HASH_BLOBS | HASH_CONTEXT); + pdir->include_detached = include_detached; MemoryContextSwitchTo(oldcontext); return pdir; @@ -328,7 +333,7 @@ PartitionDirectoryLookup(PartitionDirectory pdir, Relation rel) */ RelationIncrementReferenceCount(rel); pde->rel = rel; - pde->pd = RelationGetPartitionDesc(rel); + pde->pd = RelationGetPartitionDesc(rel, pdir->include_detached); Assert(pde->pd != NULL); } return pde->pd; diff --git a/src/backend/tcop/utility.c b/src/backend/tcop/utility.c index 9b0c376c8c..57ce760edc 100644 --- a/src/backend/tcop/utility.c +++ b/src/backend/tcop/utility.c @@ -1240,6 +1240,25 @@ ProcessUtilitySlow(ParseState *pstate, AlterTableStmt *atstmt = (AlterTableStmt *) parsetree; Oid relid; LOCKMODE lockmode; + ListCell *cell; + + /* + * Disallow ALTER TABLE .. DETACH CONCURRENTLY in a + * transaction block or function. (Perhaps it could be + * allowed in a procedure, but don't hold your breath.) + */ + foreach(cell, atstmt->cmds) + { + AlterTableCmd *cmd = (AlterTableCmd *) lfirst(cell); + + /* Disallow DETACH CONCURRENTLY in a transaction block */ + if (cmd->subtype == AT_DetachPartition) + { + if (((PartitionCmd *) cmd->def)->concurrent) + PreventInTransactionBlock(isTopLevel, + "ALTER TABLE ... DETACH CONCURRENTLY"); + } + } /* * Figure out lock mode, and acquire lock. This also does diff --git a/src/bin/psql/describe.c b/src/bin/psql/describe.c index 57266f4fc3..329b650e12 100644 --- a/src/bin/psql/describe.c +++ b/src/bin/psql/describe.c @@ -2109,7 +2109,12 @@ describeOneTableDetails(const char *schemaname, printfPQExpBuffer(&buf, "SELECT inhparent::pg_catalog.regclass,\n" - " pg_catalog.pg_get_expr(c.relpartbound, c.oid)"); + " pg_catalog.pg_get_expr(c.relpartbound, c.oid),\n "); + + appendPQExpBuffer(&buf, + pset.sversion >= 140000 ? "inhdetached" : + "false as inhdetached"); + /* If verbose, also request the partition constraint definition */ if (verbose) appendPQExpBufferStr(&buf, @@ -2127,17 +2132,19 @@ describeOneTableDetails(const char *schemaname, { char *parent_name = PQgetvalue(result, 0, 0); char *partdef = PQgetvalue(result, 0, 1); + char *detached = PQgetvalue(result, 0, 2); - printfPQExpBuffer(&tmpbuf, _("Partition of: %s %s"), parent_name, - partdef); + printfPQExpBuffer(&tmpbuf, _("Partition of: %s %s%s"), parent_name, + partdef, + strcmp(detached, "t") == 0 ? " DETACHED" : ""); printTableAddFooter(&cont, tmpbuf.data); if (verbose) { char *partconstraintdef = NULL; - if (!PQgetisnull(result, 0, 2)) - partconstraintdef = PQgetvalue(result, 0, 2); + if (!PQgetisnull(result, 0, 3)) + partconstraintdef = PQgetvalue(result, 0, 3); /* If there isn't any constraint, show that explicitly */ if (partconstraintdef == NULL || partconstraintdef[0] == '\0') printfPQExpBuffer(&tmpbuf, _("No partition constraint")); @@ -3179,9 +3186,18 @@ describeOneTableDetails(const char *schemaname, } /* print child tables (with additional info if partitions) */ - if (pset.sversion >= 100000) + if (pset.sversion >= 140000) printfPQExpBuffer(&buf, - "SELECT c.oid::pg_catalog.regclass, c.relkind," + "SELECT c.oid::pg_catalog.regclass, c.relkind, inhdetached," + " pg_catalog.pg_get_expr(c.relpartbound, c.oid)\n" + "FROM pg_catalog.pg_class c, pg_catalog.pg_inherits i\n" + "WHERE c.oid = i.inhrelid AND i.inhparent = '%s'\n" + "ORDER BY pg_catalog.pg_get_expr(c.relpartbound, c.oid) = 'DEFAULT'," + " c.oid::pg_catalog.regclass::pg_catalog.text;", + oid); + else if (pset.sversion >= 100000) + printfPQExpBuffer(&buf, + "SELECT c.oid::pg_catalog.regclass, c.relkind, false AS inhdetached," " pg_catalog.pg_get_expr(c.relpartbound, c.oid)\n" "FROM pg_catalog.pg_class c, pg_catalog.pg_inherits i\n" "WHERE c.oid = i.inhrelid AND i.inhparent = '%s'\n" @@ -3190,14 +3206,14 @@ describeOneTableDetails(const char *schemaname, oid); else if (pset.sversion >= 80300) printfPQExpBuffer(&buf, - "SELECT c.oid::pg_catalog.regclass, c.relkind, NULL\n" + "SELECT c.oid::pg_catalog.regclass, c.relkind, false AS inhdetached, NULL\n" "FROM pg_catalog.pg_class c, pg_catalog.pg_inherits i\n" "WHERE c.oid = i.inhrelid AND i.inhparent = '%s'\n" "ORDER BY c.oid::pg_catalog.regclass::pg_catalog.text;", oid); else printfPQExpBuffer(&buf, - "SELECT c.oid::pg_catalog.regclass, c.relkind, NULL\n" + "SELECT c.oid::pg_catalog.regclass, c.relkind, false AS inhdetached, NULL\n" "FROM pg_catalog.pg_class c, pg_catalog.pg_inherits i\n" "WHERE c.oid = i.inhrelid AND i.inhparent = '%s'\n" "ORDER BY c.relname;", @@ -3247,11 +3263,13 @@ describeOneTableDetails(const char *schemaname, else printfPQExpBuffer(&buf, "%*s %s", ctw, "", PQgetvalue(result, i, 0)); - if (!PQgetisnull(result, i, 2)) - appendPQExpBuffer(&buf, " %s", PQgetvalue(result, i, 2)); + if (!PQgetisnull(result, i, 3)) + appendPQExpBuffer(&buf, " %s", PQgetvalue(result, i, 3)); if (child_relkind == RELKIND_PARTITIONED_TABLE || child_relkind == RELKIND_PARTITIONED_INDEX) appendPQExpBufferStr(&buf, ", PARTITIONED"); + if (strcmp(PQgetvalue(result, i, 2), "t") == 0) + appendPQExpBuffer(&buf, " (DETACHED)"); if (i < tuples - 1) appendPQExpBufferChar(&buf, ','); diff --git a/src/include/catalog/pg_inherits.h b/src/include/catalog/pg_inherits.h index 0432c3ab88..fcdea9f26f 100644 --- a/src/include/catalog/pg_inherits.h +++ b/src/include/catalog/pg_inherits.h @@ -34,6 +34,7 @@ CATALOG(pg_inherits,2611,InheritsRelationId) Oid inhrelid; Oid inhparent; int32 inhseqno; + bool inhdetached; } FormData_pg_inherits; /* ---------------- @@ -44,7 +45,8 @@ CATALOG(pg_inherits,2611,InheritsRelationId) typedef FormData_pg_inherits *Form_pg_inherits; -extern List *find_inheritance_children(Oid parentrelId, LOCKMODE lockmode); +extern List *find_inheritance_children(Oid parentrelId, bool include_detached, + LOCKMODE lockmode); extern List *find_all_inheritors(Oid parentrelId, LOCKMODE lockmode, List **parents); extern bool has_subclass(Oid relationId); @@ -52,6 +54,7 @@ extern bool has_superclass(Oid relationId); extern bool typeInheritsFrom(Oid subclassTypeId, Oid superclassTypeId); extern void StoreSingleInheritance(Oid relationId, Oid parentOid, int32 seqNumber); -extern bool DeleteInheritsTuple(Oid inhrelid, Oid inhparent); +extern bool DeleteInheritsTuple(Oid inhrelid, Oid inhparent, bool allow_detached, + const char *childname); #endif /* PG_INHERITS_H */ diff --git a/src/include/nodes/parsenodes.h b/src/include/nodes/parsenodes.h index 151bcdb7ef..d291b4d7df 100644 --- a/src/include/nodes/parsenodes.h +++ b/src/include/nodes/parsenodes.h @@ -859,6 +859,7 @@ typedef struct PartitionCmd NodeTag type; RangeVar *name; /* name of partition to attach/detach */ PartitionBoundSpec *bound; /* FOR VALUES, if attaching */ + bool concurrent; } PartitionCmd; /**************************************************************************** @@ -1845,6 +1846,7 @@ typedef enum AlterTableType AT_GenericOptions, /* OPTIONS (...) */ AT_AttachPartition, /* ATTACH PARTITION */ AT_DetachPartition, /* DETACH PARTITION */ + AT_DetachPartitionFinalize, /* DETACH PARTITION FINALIZE */ AT_AddIdentity, /* ADD IDENTITY */ AT_SetIdentity, /* SET identity column options */ AT_DropIdentity /* DROP IDENTITY */ diff --git a/src/include/parser/kwlist.h b/src/include/parser/kwlist.h index 08f22ce211..27414b0912 100644 --- a/src/include/parser/kwlist.h +++ b/src/include/parser/kwlist.h @@ -163,6 +163,7 @@ PG_KEYWORD("false", FALSE_P, RESERVED_KEYWORD) PG_KEYWORD("family", FAMILY, UNRESERVED_KEYWORD) PG_KEYWORD("fetch", FETCH, RESERVED_KEYWORD) PG_KEYWORD("filter", FILTER, UNRESERVED_KEYWORD) +PG_KEYWORD("finalize", FINALIZE, UNRESERVED_KEYWORD) PG_KEYWORD("first", FIRST_P, UNRESERVED_KEYWORD) PG_KEYWORD("float", FLOAT_P, COL_NAME_KEYWORD) PG_KEYWORD("following", FOLLOWING, UNRESERVED_KEYWORD) diff --git a/src/include/partitioning/partdesc.h b/src/include/partitioning/partdesc.h index 70df764981..b1ee9fd5e3 100644 --- a/src/include/partitioning/partdesc.h +++ b/src/include/partitioning/partdesc.h @@ -21,6 +21,7 @@ typedef struct PartitionDescData { int nparts; /* Number of partitions */ + bool includes_detached; /* Does it include detached partitions */ Oid *oids; /* Array of 'nparts' elements containing * partition OIDs in order of the their bounds */ bool *is_leaf; /* Array of 'nparts' elements storing whether @@ -30,9 +31,9 @@ typedef struct PartitionDescData } PartitionDescData; -extern PartitionDesc RelationGetPartitionDesc(Relation rel); +extern PartitionDesc RelationGetPartitionDesc(Relation rel, bool include_detached); -extern PartitionDirectory CreatePartitionDirectory(MemoryContext mcxt); +extern PartitionDirectory CreatePartitionDirectory(MemoryContext mcxt, bool include_detached); extern PartitionDesc PartitionDirectoryLookup(PartitionDirectory, Relation); extern void DestroyPartitionDirectory(PartitionDirectory pdir); diff --git a/src/test/isolation/expected/detach-partition-concurrently-1.out b/src/test/isolation/expected/detach-partition-concurrently-1.out new file mode 100644 index 0000000000..04a0446ccb --- /dev/null +++ b/src/test/isolation/expected/detach-partition-concurrently-1.out @@ -0,0 +1,182 @@ +Parsed test spec with 3 sessions + +starting permutation: s1b s1s s2d s1s s1c s2drop s1s +step s1b: BEGIN; +step s1s: SELECT * FROM d_listp; +a + +1 +2 +step s2d: ALTER TABLE d_listp DETACH PARTITION d_listp2 CONCURRENTLY; <waiting ...> +step s1s: SELECT * FROM d_listp; +a + +1 +step s1c: COMMIT; +step s2d: <... completed> +step s2drop: DROP TABLE d_listp2; +step s1s: SELECT * FROM d_listp; +a + +1 + +starting permutation: s1b s1s s2d s1s s3s s3i s1c s3i s2drop s1s +step s1b: BEGIN; +step s1s: SELECT * FROM d_listp; +a + +1 +2 +step s2d: ALTER TABLE d_listp DETACH PARTITION d_listp2 CONCURRENTLY; <waiting ...> +step s1s: SELECT * FROM d_listp; +a + +1 +step s3s: SELECT * FROM d_listp; +a + +1 +step s3i: SELECT relpartbound IS NULL FROM pg_class where relname = 'd_listp2'; +?column? + +f +step s1c: COMMIT; +step s2d: <... completed> +step s3i: SELECT relpartbound IS NULL FROM pg_class where relname = 'd_listp2'; +?column? + +t +step s2drop: DROP TABLE d_listp2; +step s1s: SELECT * FROM d_listp; +a + +1 + +starting permutation: s1b s1s s2d s1ins s1c +step s1b: BEGIN; +step s1s: SELECT * FROM d_listp; +a + +1 +2 +step s2d: ALTER TABLE d_listp DETACH PARTITION d_listp2 CONCURRENTLY; <waiting ...> +step s1ins: INSERT INTO d_listp VALUES (1); +step s1c: COMMIT; +step s2d: <... completed> + +starting permutation: s1brr s1s s2d s1ins s1c +step s1brr: BEGIN ISOLATION LEVEL REPEATABLE READ; +step s1s: SELECT * FROM d_listp; +a + +1 +2 +step s2d: ALTER TABLE d_listp DETACH PARTITION d_listp2 CONCURRENTLY; <waiting ...> +step s1ins: INSERT INTO d_listp VALUES (1); +step s1c: COMMIT; +step s2d: <... completed> + +starting permutation: s1brr s1s s2d s1s s1c +step s1brr: BEGIN ISOLATION LEVEL REPEATABLE READ; +step s1s: SELECT * FROM d_listp; +a + +1 +2 +step s2d: ALTER TABLE d_listp DETACH PARTITION d_listp2 CONCURRENTLY; <waiting ...> +step s1s: SELECT * FROM d_listp; +a + +1 +step s1c: COMMIT; +step s2d: <... completed> + +starting permutation: s1brr s1prep s1s s2d s1s s1exec s1c +step s1brr: BEGIN ISOLATION LEVEL REPEATABLE READ; +step s1prep: PREPARE f(int) AS INSERT INTO d_listp VALUES ($1); +step s1s: SELECT * FROM d_listp; +a + +1 +2 +step s2d: ALTER TABLE d_listp DETACH PARTITION d_listp2 CONCURRENTLY; <waiting ...> +step s1s: SELECT * FROM d_listp; +a + +1 +step s1exec: EXECUTE f(1); DEALLOCATE f; +step s1c: COMMIT; +step s2d: <... completed> + +starting permutation: s1brr s1prep s1s s2d s1s s1exec2 s1c +step s1brr: BEGIN ISOLATION LEVEL REPEATABLE READ; +step s1prep: PREPARE f(int) AS INSERT INTO d_listp VALUES ($1); +step s1s: SELECT * FROM d_listp; +a + +1 +2 +step s2d: ALTER TABLE d_listp DETACH PARTITION d_listp2 CONCURRENTLY; <waiting ...> +step s1s: SELECT * FROM d_listp; +a + +1 +step s1exec2: EXECUTE f(2); DEALLOCATE f; +step s2d: <... completed> +error in steps s1exec2 s2d: ERROR: no partition of relation "d_listp" found for row +step s1c: COMMIT; + +starting permutation: s1b s1s s2d s3drop1 s1c +step s1b: BEGIN; +step s1s: SELECT * FROM d_listp; +a + +1 +2 +step s2d: ALTER TABLE d_listp DETACH PARTITION d_listp2 CONCURRENTLY; <waiting ...> +step s3drop1: DROP TABLE d_listp; <waiting ...> +step s1c: COMMIT; +step s2d: <... completed> +step s3drop1: <... completed> +error in steps s1c s2d s3drop1: ERROR: partitioned table "d_listp" was removed concurrently + +starting permutation: s1b s1s s2d s3drop2 s1c +step s1b: BEGIN; +step s1s: SELECT * FROM d_listp; +a + +1 +2 +step s2d: ALTER TABLE d_listp DETACH PARTITION d_listp2 CONCURRENTLY; <waiting ...> +step s3drop2: DROP TABLE d_listp2; <waiting ...> +step s1c: COMMIT; +step s2d: <... completed> +step s3drop2: <... completed> +error in steps s1c s2d s3drop2: ERROR: partition "d_listp2" was removed concurrently + +starting permutation: s1b s1s s2d s3detach s1c +step s1b: BEGIN; +step s1s: SELECT * FROM d_listp; +a + +1 +2 +step s2d: ALTER TABLE d_listp DETACH PARTITION d_listp2 CONCURRENTLY; <waiting ...> +step s3detach: ALTER TABLE d_listp DETACH PARTITION d_listp2; <waiting ...> +step s1c: COMMIT; +step s2d: <... completed> +step s3detach: <... completed> +error in steps s1c s2d s3detach: ERROR: cannot detach partition "d_listp2" + +starting permutation: s1b s1s s2d s3rename s1c +step s1b: BEGIN; +step s1s: SELECT * FROM d_listp; +a + +1 +2 +step s2d: ALTER TABLE d_listp DETACH PARTITION d_listp2 CONCURRENTLY; <waiting ...> +step s3rename: ALTER TABLE d_listp2 RENAME TO foobar; <waiting ...> +step s1c: COMMIT; +step s2d: <... completed> +step s3rename: <... completed> diff --git a/src/test/isolation/expected/detach-partition-concurrently-2.out b/src/test/isolation/expected/detach-partition-concurrently-2.out new file mode 100644 index 0000000000..85be707b40 --- /dev/null +++ b/src/test/isolation/expected/detach-partition-concurrently-2.out @@ -0,0 +1,66 @@ +Parsed test spec with 3 sessions + +starting permutation: s1b s1s s2d s3i1 s1c +step s1b: BEGIN; +step s1s: SELECT * FROM d_lp_fk; +a + +1 +2 +step s2d: ALTER TABLE d_lp_fk DETACH PARTITION d_lp_fk_1 CONCURRENTLY; <waiting ...> +step s3i1: INSERT INTO d_lp_fk_r VALUES (1); +ERROR: insert or update on table "d_lp_fk_r" violates foreign key constraint "d_lp_fk_r_a_fkey" +step s1c: COMMIT; +step s2d: <... completed> + +starting permutation: s1b s1s s2d s3i2 s3i2 s1c +step s1b: BEGIN; +step s1s: SELECT * FROM d_lp_fk; +a + +1 +2 +step s2d: ALTER TABLE d_lp_fk DETACH PARTITION d_lp_fk_1 CONCURRENTLY; <waiting ...> +step s3i2: INSERT INTO d_lp_fk_r VALUES (2); +step s3i2: INSERT INTO d_lp_fk_r VALUES (2); +step s1c: COMMIT; +step s2d: <... completed> + +starting permutation: s1b s1s s3i1 s2d s1c +step s1b: BEGIN; +step s1s: SELECT * FROM d_lp_fk; +a + +1 +2 +step s3i1: INSERT INTO d_lp_fk_r VALUES (1); +step s2d: ALTER TABLE d_lp_fk DETACH PARTITION d_lp_fk_1 CONCURRENTLY; +ERROR: removing partition "d_lp_fk_1" violates foreign key constraint "d_lp_fk_r_a_fkey1" +step s1c: COMMIT; + +starting permutation: s1b s1s s3i2 s2d s1c +step s1b: BEGIN; +step s1s: SELECT * FROM d_lp_fk; +a + +1 +2 +step s3i2: INSERT INTO d_lp_fk_r VALUES (2); +step s2d: ALTER TABLE d_lp_fk DETACH PARTITION d_lp_fk_1 CONCURRENTLY; <waiting ...> +step s1c: COMMIT; +step s2d: <... completed> + +starting permutation: s1b s1s s3b s2d s3i1 s1c s3c +step s1b: BEGIN; +step s1s: SELECT * FROM d_lp_fk; +a + +1 +2 +step s3b: BEGIN; +step s2d: ALTER TABLE d_lp_fk DETACH PARTITION d_lp_fk_1 CONCURRENTLY; <waiting ...> +step s3i1: INSERT INTO d_lp_fk_r VALUES (1); +ERROR: insert or update on table "d_lp_fk_r" violates foreign key constraint "d_lp_fk_r_a_fkey" +step s1c: COMMIT; +step s2d: <... completed> +step s3c: COMMIT; diff --git a/src/test/isolation/isolation_schedule b/src/test/isolation/isolation_schedule index 218c87b24b..a9c1abc244 100644 --- a/src/test/isolation/isolation_schedule +++ b/src/test/isolation/isolation_schedule @@ -24,6 +24,8 @@ test: deadlock-hard test: deadlock-soft test: deadlock-soft-2 test: deadlock-parallel +test: detach-partition-concurrently-1 +test: detach-partition-concurrently-2 test: fk-contention test: fk-deadlock test: fk-deadlock2 diff --git a/src/test/isolation/specs/detach-partition-concurrently-1.spec b/src/test/isolation/specs/detach-partition-concurrently-1.spec new file mode 100644 index 0000000000..ca92530824 --- /dev/null +++ b/src/test/isolation/specs/detach-partition-concurrently-1.spec @@ -0,0 +1,71 @@ +# Test that detach partition concurrently makes the partition invisible at the +# correct time. + +setup +{ + DROP TABLE IF EXISTS d_listp, d_listp1, d_listp2; + CREATE TABLE d_listp (a int) PARTITION BY LIST(a); + CREATE TABLE d_listp1 PARTITION OF d_listp FOR VALUES IN (1); + CREATE TABLE d_listp2 PARTITION OF d_listp FOR VALUES IN (2); + INSERT INTO d_listp VALUES (1),(2); +} + +teardown { + DROP TABLE IF EXISTS d_listp, d_listp2; +} + +session "s1" +step "s1b" { BEGIN; } +step "s1brr" { BEGIN ISOLATION LEVEL REPEATABLE READ; } +step "s1s" { SELECT * FROM d_listp; } +step "s1ins" { INSERT INTO d_listp VALUES (1); } +step "s1prep" { PREPARE f(int) AS INSERT INTO d_listp VALUES ($1); } +step "s1exec" { EXECUTE f(1); DEALLOCATE f; } +step "s1exec2" { EXECUTE f(2); DEALLOCATE f; } +step "s1c" { COMMIT; } + +session "s2" +step "s2d" { ALTER TABLE d_listp DETACH PARTITION d_listp2 CONCURRENTLY; } +step "s2drop" { DROP TABLE d_listp2; } + +session "s3" +step "s3s" { SELECT * FROM d_listp; } +step "s3i" { SELECT relpartbound IS NULL FROM pg_class where relname = 'd_listp2'; } +step "s3drop1" { DROP TABLE d_listp; } +step "s3drop2" { DROP TABLE d_listp2; } +step "s3detach" { ALTER TABLE d_listp DETACH PARTITION d_listp2; } +step "s3rename" { ALTER TABLE d_listp2 RENAME TO foobar; } + +# The transaction that detaches hangs until it sees any older transaction +# terminate. +permutation "s1b" "s1s" "s2d" "s1s" "s1c" "s2drop" "s1s" + +# relpartbound remains set until s1 commits +# XXX this could be timing dependent :-( +permutation "s1b" "s1s" "s2d" "s1s" "s3s" "s3i" "s1c" "s3i" "s2drop" "s1s" + +# Session s1 no longer sees the partition ... +permutation "s1b" "s1s" "s2d" "s1ins" "s1c" +# ... regardless of the isolation level. +permutation "s1brr" "s1s" "s2d" "s1ins" "s1c" +# However, DETACH CONCURRENTLY is not isolation-level-correct +permutation "s1brr" "s1s" "s2d" "s1s" "s1c" + +# a prepared query is not blocked +permutation "s1brr" "s1prep" "s1s" "s2d" "s1s" "s1exec" "s1c" +permutation "s1brr" "s1prep" "s1s" "s2d" "s1s" "s1exec2" "s1c" + + +# Note: the following tests are not very interesting in isolationtester because +# the way this tool handles multiple sessions: it'll only run the drop/detach/ +# rename commands after s2 is blocked waiting. It would be more useful to +# run the DDL when s2 commits its first transaction instead. Maybe these should +# become TAP tests someday. + +# What happens if the partition or the parent table is dropped while waiting? +permutation "s1b" "s1s" "s2d" "s3drop1" "s1c" +permutation "s1b" "s1s" "s2d" "s3drop2" "s1c" +# Also try a non-concurrent detach while the other one is waiting +permutation "s1b" "s1s" "s2d" "s3detach" "s1c" +# and some other DDL +permutation "s1b" "s1s" "s2d" "s3rename" "s1c" diff --git a/src/test/isolation/specs/detach-partition-concurrently-2.spec b/src/test/isolation/specs/detach-partition-concurrently-2.spec new file mode 100644 index 0000000000..9281c80a69 --- /dev/null +++ b/src/test/isolation/specs/detach-partition-concurrently-2.spec @@ -0,0 +1,41 @@ +# Test that detach partition concurrently makes the partition safe +# for foreign keys that reference it. + +setup +{ + DROP TABLE IF EXISTS d_lp_fk, d_lp_fk_1, d_lp_fk_2, d_lp_fk_r; + + CREATE TABLE d_lp_fk (a int PRIMARY KEY) PARTITION BY LIST(a); + CREATE TABLE d_lp_fk_1 PARTITION OF d_lp_fk FOR VALUES IN (1); + CREATE TABLE d_lp_fk_2 PARTITION OF d_lp_fk FOR VALUES IN (2); + INSERT INTO d_lp_fk VALUES (1), (2); + + CREATE TABLE d_lp_fk_r (a int references d_lp_fk); +} + +teardown { DROP TABLE IF EXISTS d_lp_fk, d_lp_fk_1, d_lp_fk_2, d_lp_fk_r; } + +session "s1" +step "s1b" { BEGIN; } +step "s1s" { SELECT * FROM d_lp_fk; } +step "s1c" { COMMIT; } + +session "s2" +step "s2d" { ALTER TABLE d_lp_fk DETACH PARTITION d_lp_fk_1 CONCURRENTLY; } + +session "s3" +step "s3b" { BEGIN; } +step "s3i1" { INSERT INTO d_lp_fk_r VALUES (1); } +step "s3i2" { INSERT INTO d_lp_fk_r VALUES (2); } +step "s3c" { COMMIT; } + +# The transaction that detaches hangs until it sees any older transaction +# terminate. +permutation "s1b" "s1s" "s2d" "s3i1" "s1c" +permutation "s1b" "s1s" "s2d" "s3i2" "s3i2" "s1c" + +permutation "s1b" "s1s" "s3i1" "s2d" "s1c" +permutation "s1b" "s1s" "s3i2" "s2d" "s1c" + +# what if s3 has an uncommitted insertion? +permutation "s1b" "s1s" "s3b" "s2d" "s3i1" "s1c" "s3c" -- 2.20.1 --ikeVEW9yuYc//A+q-- ^ permalink raw reply [nested|flat] 4+ messages in thread
* Re: Patch: Add parse_type Function @ 2024-02-12 16:37 David E. Wheeler <[email protected]> 2024-02-12 17:53 ` Re: Patch: Add parse_type Function Tom Lane <[email protected]> 0 siblings, 1 reply; 4+ messages in thread From: David E. Wheeler @ 2024-02-12 16:37 UTC (permalink / raw) To: Erik Wienhold <[email protected]>; +Cc: jian he <[email protected]>; Jim Jones <[email protected]>; Dagfinn Ilmari Mannsåker <[email protected]>; [email protected]; Pavel Stehule <[email protected]> On Feb 10, 2024, at 20:52, Erik Wienhold <[email protected]> wrote: > > Let me comment on some issues since I wrote the very first version of > parse_type() on which David's patch is based. Thanks Erik. >> On 2024-02-01 01:00 +0100, jian he <[email protected]> wrote: >> if you are wondering around other code deal with NULL, ErrorSaveContext. >> NULL would be more correct? >> `(void) parseTypeString(type, &typid, &typmod, NULL);` Fixed. >> also parseTypeString already explained the third argument, our >> comments can be simplified as: >> /* >> * Parse type-name argument to obtain type OID and encoded typmod. We don't >> * need to handle parseTypeString failure, just let the error be >> * raised. >> */ Thanks, adopted that language. >> cosmetic issue. Looking around other error handling code, the format >> should be something like: >> ` >> if (get_call_result_type(fcinfo, NULL, &tupdesc) != TYPEFUNC_COMPOSITE) >> ereport(ERROR, >> (errcode(ERRCODE_FEATURE_NOT_SUPPORTED), >> errmsg("function returning record called in" >> "context that cannot accept type record"))); >> ` > > +1 I poked around and found some other examples, yes. I went with a single long line for errmsg following the example in adminpack.c[1] >> `PG_FUNCTION_INFO_V1(parse_type);` >> is unnecessary? >> based on the error information: https://cirrus-ci.com/task/5899457502380032 >> not sure why it only fails on windows. > > Yeah, it's not necessary for internal functions per [1]. It's a > leftover from when this function was part of the pgtap extension. Removed. >> +#define PARSE_TYPE_STRING_COLS 2 /* Returns two columns. */ >> +#undef PARSE_TYPE_STRING_COLS >> Are these necessary? >> given that the comments already say return the OID and type modifier. > > Not sure what the convention here is but I found this in a couple of > places, maybe even a tutorial on writing C functions. See > `git grep '_COLS\s\+[1-9]'` for those instances. It's in line with my > habit of avoiding magic constants. Left in place for now. > >> + ( <parameter>typid</parameter> <type>oid</type>, >> + <parameter>typmod</parameter> <type>int4</type> ) >> here `int4` should be `integer`? > > +1 Fixed. > Yes, the sentence is rather handwavy. What is meant here is that this > function also resolves types whose typmod is determined by the SQL > parser or some step after that. There are types whose typmod is > whatever value is found inside the parenthesis, e.g. bit(13) has typmod > 13. Our first idea before coming up with that function was to do some > string manipulation and extract the typmod from inside the parenthesis. > But we soon found out that other typmods don't translate one to one, > e.g. varchar(13) has typmod 17. The interval type is also special > because the typmod is some bitmask encoding of e.g. 'second(0)'. Hence > the mention of the SQL grammar. I adopted some of your language here --- and fixed the spelling errors :-) >> can be simplified: >> + <para> >> + Parses a string representing an SQL data type, optionally >> schema-qualified. >> + Returns a record with two fields, <parameter>typid</parameter> and >> + <parameter>typmod</parameter>, representing the OID and >> modifier for the >> + type. These correspond to the parameters to pass to the >> + <link linkend="format_type"><function>format_type</function> >> function.</link> >> + </para> >> (I don't have a strong opinion though) > > Sounds better. The CREATE TABLE reference is superfluous. Done. Best, David [1] https://github.com/postgres/postgres/blob/09eb633e1baa3b7cd7929f3cc77f9c46f63c20b1/contrib/adminpack... Attachments: [application/octet-stream] v4-0001-Add-parse_type-SQL-function.patch (10.1K, ../../[email protected]/2-v4-0001-Add-parse_type-SQL-function.patch) download | inline diff: From 72b2e9dc4f677d9d17544fbe23aee0418f76b5cf Mon Sep 17 00:00:00 2001 From: "David E. Wheeler" <[email protected]> Date: Mon, 12 Feb 2024 11:25:41 -0500 Subject: [PATCH v4] Add parse_type() SQL function The `parse_type()` function uses the underlying `parseTypeString()` C function to parse a string representing a data type into a type ID and typmod suitable for passing to `format_type()`. This allows one to derive the formal SQL name for a type from a string that may be an alias: SELECT format_type(p.typid, p.typmod) FROM parse_type('timestamp(4)') p; format_type -------------------------------- timestamp(4) without time zone This function also resolves types whose typmod is determined by the SQL parser or some step after that, such as interval types where the stored field option is encoded in the typmod: SELECT format_type(p.typid, p.typmod) FROM parse_type('interval second(0)') p; format_type -------------------- interval second(0) Useful for unit tests for against column data types, for example. Originally written by Erik Wienhold for use in pgTAP. --- doc/src/sgml/func.sgml | 30 ++++++++++++- src/backend/utils/adt/format_type.c | 52 ++++++++++++++++++++++ src/include/catalog/pg_proc.dat | 4 ++ src/include/utils/builtins.h | 2 + src/test/regress/expected/create_type.out | 53 +++++++++++++++++++++++ src/test/regress/sql/create_type.sql | 28 ++++++++++++ 6 files changed, 168 insertions(+), 1 deletion(-) diff --git a/doc/src/sgml/func.sgml b/doc/src/sgml/func.sgml index 11d537b341..adcc433a69 100644 --- a/doc/src/sgml/func.sgml +++ b/doc/src/sgml/func.sgml @@ -24759,7 +24759,7 @@ SELECT pg_type_is_visible('myschema.widget'::regtype); <tbody> <row> - <entry role="func_table_entry"><para role="func_signature"> + <entry id="format_type" role="func_table_entry"><para role="func_signature"> <indexterm> <primary>format_type</primary> </indexterm> @@ -24773,6 +24773,34 @@ SELECT pg_type_is_visible('myschema.widget'::regtype); </para></entry> </row> + <row> + <entry id="parse_type" role="func_table_entry"><para role="func_signature"> + <indexterm> + <primary>parse_type</primary> + </indexterm> + <function>parse_type</function> ( <parameter>type</parameter> <type>text</type> ) + <returnvalue>record</returnvalue> + ( <parameter>typid</parameter> <type>oid</type>, + <parameter>typmod</parameter> <type>integer</type> ) + </para> + <para> + Parses a string representing an SQL data type, optionally schema-qualified. + Returns a record with two fields, <parameter>typid</parameter> and + <parameter>typmod</parameter>, representing the OID and modifier for the + type. These correspond to the parameters to pass to the + <link linkend="format_type"><function>format_type</function> function.</link> + </para> + <para> + For example: +<programlisting> +SELECT format_type(p.typid, p.typmod) FROM parse_type('timestamp(4)') p; + format_type + -------------------------------- + timestamp(4) without time zone +</programlisting> + </para></entry> + </row> + <row> <entry id="pg-char-to-encoding" role="func_table_entry"><para role="func_signature"> <indexterm> diff --git a/src/backend/utils/adt/format_type.c b/src/backend/utils/adt/format_type.c index 28ba0fbd19..2be1cf9f5e 100644 --- a/src/backend/utils/adt/format_type.c +++ b/src/backend/utils/adt/format_type.c @@ -26,6 +26,9 @@ #include "utils/lsyscache.h" #include "utils/numeric.h" #include "utils/syscache.h" +#include "fmgr.h" +#include "funcapi.h" +#include "parser/parse_type.h" static char *printTypmod(const char *typname, int32 typmod, Oid typmodout); @@ -482,3 +485,52 @@ oidvectortypes(PG_FUNCTION_ARGS) PG_RETURN_TEXT_P(cstring_to_text(result)); } + +/* + * parse_type() is the inverse of pg_catalog.format_type(): it takes a string + * representing an SQL-compatible type declaration, such as "int4" or "integer" + * or "character varying(32)", parses it, and returns the OID and type modifier. + * + * Raises an error on an invalid type. + * + * Internally it relies on the Postgres core parseTypeString() function defined + * in src/backend/parser/parse_type.c. + */ +Datum +parse_type(PG_FUNCTION_ARGS) +{ +#define PARSE_TYPE_STRING_COLS 2 /* Returns two columns. */ + const char *type; /* the type string we want to resolve */ + Oid typid; /* the resolved type oid */ + int32 typmod; /* the resolved type modifier */ + TupleDesc tupdesc; + HeapTuple rettuple; + Datum values[PARSE_TYPE_STRING_COLS] = {0}; + bool nulls[PARSE_TYPE_STRING_COLS] = {0}; + + type = text_to_cstring(PG_GETARG_TEXT_PP(0)); + + /* + * Build a tuple descriptor for our result type; return an error if not + * called in a context that expects a record. + */ + if (get_call_result_type(fcinfo, NULL, &tupdesc) != TYPEFUNC_COMPOSITE) + ereport(ERROR, + (errcode(ERRCODE_FEATURE_NOT_SUPPORTED), + errmsg("function returning record called in context that cannot accept type record"))); + + BlessTupleDesc(tupdesc); + + /* + * Parse type-name argument to obtain type OID and encoded typmod. We don't + * need to handle parseTypeString failure, just let the error be raised. + */ + (void) parseTypeString(type, &typid, &typmod, NULL); + + /* Create and return tuple. */ + values[0] = typid; + values[1] = typmod; + rettuple = heap_form_tuple(tupdesc, values, nulls); + return HeapTupleGetDatum(rettuple); +#undef PARSE_TYPE_STRING_COLS +} diff --git a/src/include/catalog/pg_proc.dat b/src/include/catalog/pg_proc.dat index 29af4ce65d..37205e1b31 100644 --- a/src/include/catalog/pg_proc.dat +++ b/src/include/catalog/pg_proc.dat @@ -2184,6 +2184,10 @@ { oid => '1081', descr => 'format a type oid and atttypmod to canonical SQL', proname => 'format_type', proisstrict => 'f', provolatile => 's', prorettype => 'text', proargtypes => 'oid int4', prosrc => 'format_type' }, +{ oid => '8401', descr => 'parse a type string into its a type oid and atttypmod', + proname => 'parse_type', proisstrict => 't', provolatile => 's', + prorettype => 'record', proargtypes => 'text', prosrc => 'parse_type', + proallargtypes => '{text,oid,int4}', proargmodes => '{i,o,o}', proargnames => '{typname,typid,typmod}' }, { oid => '1084', descr => 'I/O', proname => 'date_in', provolatile => 's', prorettype => 'date', proargtypes => 'cstring', prosrc => 'date_in' }, diff --git a/src/include/utils/builtins.h b/src/include/utils/builtins.h index 359c570f23..264ad090be 100644 --- a/src/include/utils/builtins.h +++ b/src/include/utils/builtins.h @@ -133,6 +133,8 @@ extern char *format_type_with_typemod(Oid type_oid, int32 typemod); extern int32 type_maximum_size(Oid type_oid, int32 typemod); +extern Datum parse_type(PG_FUNCTION_ARGS); + /* quote.c */ extern char *quote_literal_cstr(const char *rawstr); diff --git a/src/test/regress/expected/create_type.out b/src/test/regress/expected/create_type.out index 7383fcdbb1..84ea71090c 100644 --- a/src/test/regress/expected/create_type.out +++ b/src/test/regress/expected/create_type.out @@ -249,6 +249,59 @@ select format_type('bpchar'::regtype, -1); bpchar (1 row) +-- Test parse_type +SELECT * FROM parse_type('text') p(typid, typmod); + typid | typmod +-------+-------- + 25 | -1 +(1 row) + +SELECT * FROM parse_type(NULL) p(typid, typmod); + typid | typmod +-------+-------- + | +(1 row) + +-- Test parse_type errors +SELECT parse_type('nonesuch'); -- error expected +ERROR: type "nonesuch" does not exist +SELECT parse_type('interval nonesuch'); -- grammar error expected +ERROR: syntax error at or near "nonesuch" +LINE 1: SELECT parse_type('interval nonesuch'); + ^ +CONTEXT: invalid type name "interval nonesuch" +SELECT parse_type('year(4)'); -- grammar error expected +ERROR: type "year" does not exist +-- Test parse_type with various aliases and grammar-based types +WITH s(s) AS ( + SELECT * FROM unnest(ARRAY[ + 'timestamp(4)', + 'interval(0)', + 'interval second(0)', + 'timestamptz', + 'timestamptz(6)', + 'varchar', + 'varchar(128)', + 'mytab' + ]) +), +p(typid, typmod) AS ( + SELECT ((parse_type(s)).*) + FROM s +) +SELECT format_type(typid, typmod) FROM p; + format_type +-------------------------------- + timestamp(4) without time zone + interval(0) + interval second(0) + timestamp with time zone + timestamp(6) with time zone + character varying + character varying(128) + mytab +(8 rows) + -- Test non-error-throwing APIs using widget, which still throws errors SELECT pg_input_is_valid('(1,2,3)', 'widget'); pg_input_is_valid diff --git a/src/test/regress/sql/create_type.sql b/src/test/regress/sql/create_type.sql index c25018029c..e4700b8719 100644 --- a/src/test/regress/sql/create_type.sql +++ b/src/test/regress/sql/create_type.sql @@ -192,6 +192,34 @@ select format_type('bpchar'::regtype, null); -- this behavior difference is intentional select format_type('bpchar'::regtype, -1); +-- Test parse_type +SELECT * FROM parse_type('text') p(typid, typmod); +SELECT * FROM parse_type(NULL) p(typid, typmod); + +-- Test parse_type errors +SELECT parse_type('nonesuch'); -- error expected +SELECT parse_type('interval nonesuch'); -- grammar error expected +SELECT parse_type('year(4)'); -- grammar error expected + +-- Test parse_type with various aliases and grammar-based types +WITH s(s) AS ( + SELECT * FROM unnest(ARRAY[ + 'timestamp(4)', + 'interval(0)', + 'interval second(0)', + 'timestamptz', + 'timestamptz(6)', + 'varchar', + 'varchar(128)', + 'mytab' + ]) +), +p(typid, typmod) AS ( + SELECT ((parse_type(s)).*) + FROM s +) +SELECT format_type(typid, typmod) FROM p; + -- Test non-error-throwing APIs using widget, which still throws errors SELECT pg_input_is_valid('(1,2,3)', 'widget'); SELECT pg_input_is_valid('(1,2)', 'widget'); -- hard error expected -- 2.43.1 ^ permalink raw reply [nested|flat] 4+ messages in thread
* Re: Patch: Add parse_type Function 2024-02-12 16:37 Re: Patch: Add parse_type Function David E. Wheeler <[email protected]> @ 2024-02-12 17:53 ` Tom Lane <[email protected]> 2024-02-12 20:55 ` Re: Patch: Add parse_type Function David E. Wheeler <[email protected]> 0 siblings, 1 reply; 4+ messages in thread From: Tom Lane @ 2024-02-12 17:53 UTC (permalink / raw) To: David E. Wheeler <[email protected]>; +Cc: Erik Wienhold <[email protected]>; jian he <[email protected]>; Jim Jones <[email protected]>; Dagfinn Ilmari Mannsåker <[email protected]>; [email protected]; Pavel Stehule <[email protected]> "David E. Wheeler" <[email protected]> writes: > [ v4-0001-Add-parse_type-SQL-function.patch ] It strikes me that this is basically to_regtype() with the additional option to return the typmod. That leads to some questions: * Should we choose a name related to to_regtype? I don't have any immediate suggestions, but since you didn't seem entirely set on parse_type, maybe it's worth thinking along those lines. OTOH, to the extent that people would use this with format_type, maybe parse_type is fine. * Perhaps the type OID output argument should be declared regtype not plain OID? It wouldn't make any difference for passing it to format_type, but in other use-cases perhaps regtype would be more readable. It's not a deal-breaker either way of course, since you can cast oid to regtype or vice versa. * Maybe the code should be in adt/regproc.c not format_type.c. * Experience with to_regtype suggests strongly that people would prefer "return NULL" to failure for an unrecognized type name. Skimming the patch, I notice that the manual addition to builtins.h should be unnecessary: the pg_proc.dat entry should be enough to create an extern in fmgrprotos.h. Also, I'm pretty sure that reformat_dat_file.pl will think your pg_proc.dat entry is overly verbose. See https://www.postgresql.org/docs/devel/system-catalog-initial-data.html regards, tom lane ^ permalink raw reply [nested|flat] 4+ messages in thread
* Re: Patch: Add parse_type Function 2024-02-12 16:37 Re: Patch: Add parse_type Function David E. Wheeler <[email protected]> 2024-02-12 17:53 ` Re: Patch: Add parse_type Function Tom Lane <[email protected]> @ 2024-02-12 20:55 ` David E. Wheeler <[email protected]> 0 siblings, 0 replies; 4+ messages in thread From: David E. Wheeler @ 2024-02-12 20:55 UTC (permalink / raw) To: Tom Lane <[email protected]>; +Cc: Erik Wienhold <[email protected]>; jian he <[email protected]>; Jim Jones <[email protected]>; Dagfinn Ilmari Mannsåker <[email protected]>; [email protected]; Pavel Stehule <[email protected]> On Feb 12, 2024, at 12:53 PM, Tom Lane <[email protected]> wrote: > "David E. Wheeler" <[email protected]> writes: >> [ v4-0001-Add-parse_type-SQL-function.patch ] > > It strikes me that this is basically to_regtype() with the additional > option to return the typmod. That leads to some questions: Huh. I saw it more on a par with format_type(), at least in the output I expect. > * Should we choose a name related to to_regtype? I don't have any > immediate suggestions, but since you didn't seem entirely set on > parse_type, maybe it's worth thinking along those lines. OTOH, > to the extent that people would use this with format_type, maybe > parse_type is fine. Yeah, and the `to_*` functions return a single OID, not two fields. > * Perhaps the type OID output argument should be declared regtype > not plain OID? It wouldn't make any difference for passing it to > format_type, but in other use-cases perhaps regtype would be more > readable. It's not a deal-breaker either way of course, since > you can cast oid to regtype or vice versa. Sure. I used `oid` because that’s exactly what the argument to format_type() is called, so thought the parity there more appropriate. Maybe its argument should be renamed to regtype? > * Maybe the code should be in adt/regproc.c not format_type.c. Happy to move it wherever makes the most sense. > * Experience with to_regtype suggests strongly that people would > prefer "return NULL" to failure for an unrecognized type name. Could do, but as with to_regtype() there’s an issue with error handling when it descends into the SQL parser. > Skimming the patch, I notice that the manual addition to > builtins.h should be unnecessary: the pg_proc.dat entry > should be enough to create an extern in fmgrprotos.h. Confirmed, will remove in next patch. > Also, I'm pretty sure that reformat_dat_file.pl will > think your pg_proc.dat entry is overly verbose. See > https://www.postgresql.org/docs/devel/system-catalog-initial-data.html There are a bunch of things it reformats: ❯ git diff --name-only src/include/catalog/pg_aggregate.dat src/include/catalog/pg_database.dat src/include/catalog/pg_proc.dat src/include/catalog/pg_type.dat src/include/utils/builtins.h And even in pg_proc.dat there are 13 blocks it reformats. I can submit with just my block formatted if you’d prefer. > BTW, another way that this problem could be approached is to use > to_regtype() as-is, with a separate function to obtain the typmod: > > select format_type(to_regtype('timestamp(4)'), to_regtypmod('timestamp(4)')); Oh interesting. > This is intellectually ugly, since it implies parsing the same > typename string twice. But on the other hand it avoids the notational > pain and runtime overhead involved in using a record-returning > function. So I think it might be roughly a wash for performance. > Question to think about is which way is easier to use. I don't > have an opinion particularly; just throwing the idea out there. Honestly I haven’t cared for the fact that parse_type() returns a record; it makes it a bit clunky. But yeah, so does this to pass the same value to two function calls. Perhaps it makes sense to add to_regtypmod() as you suggest, but then also something like: CREATE FUNCTION format_type_string(text) RETURNS TEXT AS $$ SELECT format_type(to_regtype($1), to_regtypmod($1)) $$; Best, David ^ permalink raw reply [nested|flat] 4+ messages in thread
end of thread, other threads:[~2024-02-12 20:55 UTC | newest] Thread overview: 4+ messages (download: mbox mbox.gz follow: Atom feed) -- links below jump to the message on this page -- 2020-08-03 23:21 [PATCH v1 2/2] ALTER TABLE ... DETACH CONCURRENTLY Alvaro Herrera <[email protected]> 2024-02-12 16:37 Re: Patch: Add parse_type Function David E. Wheeler <[email protected]> 2024-02-12 17:53 ` Re: Patch: Add parse_type Function Tom Lane <[email protected]> 2024-02-12 20:55 ` Re: Patch: Add parse_type Function David E. Wheeler <[email protected]>
This inbox is served by agora; see mirroring instructions for how to clone and mirror all data and code used for this inbox