From 6b8f34741bbe6dc3ee32d30f9a54e6c3877d9233 Mon Sep 17 00:00:00 2001 From: Andrey Borodin Date: Thu, 27 Aug 2026 11:30:11 +0500 Subject: [PATCH 1/4] Support concurrent partition detach Dropping a large distributed partition can spend substantial time unlinking shard storage. PostgreSQL's two-phase concurrent detach lets users first remove the partition from query routing and drop the resulting standalone table separately. Run shard detach commands as top-level nontransactional DDL, since PostgreSQL implements DETACH PARTITION CONCURRENTLY using internal commits and they cannot go through worker_apply_inter_shard_ddl_command. Propagate FINALIZE as a concurrent shard detach so an interrupted coordinator operation can be completed. Add functional coverage and an isolation test that cancels the operation after its first phase, verifies the intermediate coordinator and worker state, and completes it with FINALIZE. Discussion: https://github.com/citusdata/citus/issues/5264 --- src/backend/distributed/commands/table.c | 92 ++++++++++++++++--- .../detach_partition_concurrently.out | 45 +++++++++ ...solation_detach_partition_concurrently.out | 87 ++++++++++++++++++ src/test/regress/expected/pg14.out | 14 +-- src/test/regress/isolation_schedule | 3 + src/test/regress/multi_schedule | 2 +- ...olation_detach_partition_concurrently.spec | 88 ++++++++++++++++++ .../sql/detach_partition_concurrently.sql | 30 ++++++ src/test/regress/sql/pg14.sql | 7 +- 9 files changed, 343 insertions(+), 25 deletions(-) create mode 100644 src/test/regress/expected/detach_partition_concurrently.out create mode 100644 src/test/regress/expected/isolation_detach_partition_concurrently.out create mode 100644 src/test/regress/spec/isolation_detach_partition_concurrently.spec create mode 100644 src/test/regress/sql/detach_partition_concurrently.sql diff --git a/src/backend/distributed/commands/table.c b/src/backend/distributed/commands/table.c index 87a00ba1593..60b1863c286 100644 --- a/src/backend/distributed/commands/table.c +++ b/src/backend/distributed/commands/table.c @@ -51,6 +51,7 @@ #include "distributed/multi_partitioning_utils.h" #include "distributed/reference_table_utils.h" #include "distributed/relation_access_tracking.h" +#include "distributed/relay_utility.h" #include "distributed/resource_lock.h" #include "distributed/tenant_schema_metadata.h" #include "distributed/version_compat.h" @@ -115,6 +116,8 @@ static void ErrorIfUnsupportedAlterAddConstraintStmt(AlterTableStmt *alterTableS static List * CreateRightShardListForInterShardDDLTask(Oid rightRelationId, Oid leftRelationId, List *leftShardList); +static List * ConcurrentDetachPartitionTaskList(Oid parentRelationId, + Oid partitionRelationId); static void SetInterShardDDLTaskPlacementList(Task *task, ShardInterval *leftShardInterval, ShardInterval *rightShardInterval); @@ -1336,6 +1339,7 @@ PreprocessAlterTableStmt(Node *node, const char *alterTableCommand, */ bool deparseAT = false; bool propagateCommandToRemoteNodes = true; + bool concurrentPartitionDetach = false; /* * Sometimes we want to run a different DDL Command string on remote MX workers @@ -1666,6 +1670,18 @@ PreprocessAlterTableStmt(Node *node, const char *alterTableCommand, Assert(list_length(commandList) <= 1); rightRelationId = RangeVarGetRelid(partitionCommand->name, NoLock, false); + concurrentPartitionDetach = partitionCommand->concurrent; + } + else if (alterTableType == AT_DetachPartitionFinalize) + { + PartitionCmd *partitionCommand = (PartitionCmd *) command->def; + + rightRelationId = RangeVarGetRelid(partitionCommand->name, NoLock, false); + concurrentPartitionDetach = true; + alterTableCommand = psprintf("ALTER TABLE %s DETACH PARTITION %s " + "CONCURRENTLY", + generate_qualified_relation_name(leftRelationId), + generate_qualified_relation_name(rightRelationId)); } else if (AlterTableCommandTypeIsTrigger(alterTableType)) { @@ -1709,8 +1725,17 @@ PreprocessAlterTableStmt(Node *node, const char *alterTableCommand, else { /* if foreign key or attaching partition index related, use specialized task list function ... */ - ddlJob->taskList = InterShardDDLTaskList(leftRelationId, rightRelationId, + if (concurrentPartitionDetach) + { + ddlJob->taskList = ConcurrentDetachPartitionTaskList(leftRelationId, + rightRelationId); + ddlJob->warnForPartialFailure = true; + } + else + { + ddlJob->taskList = InterShardDDLTaskList(leftRelationId, rightRelationId, alterTableCommand); + } } } else @@ -3813,8 +3838,6 @@ ErrorIfUnsupportedAlterTableStmt(AlterTableStmt *alterTableStatement) case AT_DetachPartitionFinalize: { - ereport(ERROR, (errmsg("ALTER TABLE .. DETACH PARTITION .. FINALIZE " - "commands are currently unsupported."))); break; } @@ -3830,15 +3853,6 @@ ErrorIfUnsupportedAlterTableStmt(AlterTableStmt *alterTableStatement) "separately."))); } - PartitionCmd *partitionCommand = (PartitionCmd *) command->def; - - if (partitionCommand->concurrent) - { - ereport(ERROR, (errmsg("ALTER TABLE .. DETACH PARTITION .. " - "CONCURRENTLY commands are currently " - "unsupported."))); - } - break; } @@ -4152,6 +4166,60 @@ InterShardDDLTaskList(Oid leftRelationId, Oid rightRelationId, } +/* + * Build top-level commands for DETACH PARTITION CONCURRENTLY. Unlike other + * inter-shard DDL, these commands cannot run through + * worker_apply_inter_shard_ddl_command(), because PostgreSQL implements the + * detach using multiple transactions. + */ +static List * +ConcurrentDetachPartitionTaskList(Oid parentRelationId, Oid partitionRelationId) +{ + List *parentShardList = LoadShardIntervalList(parentRelationId); + List *partitionShardList = CreateRightShardListForInterShardDDLTask( + partitionRelationId, parentRelationId, parentShardList); + List *taskList = NIL; + uint64 jobId = INVALID_JOB_ID; + int taskId = 1; + char *parentSchemaName = get_namespace_name(get_rel_namespace(parentRelationId)); + char *partitionSchemaName = get_namespace_name(get_rel_namespace(partitionRelationId)); + char *parentRelationName = get_rel_name(parentRelationId); + char *partitionRelationName = get_rel_name(partitionRelationId); + + LockShardListMetadata(parentShardList, ShareLock); + + ShardInterval *parentShard = NULL; + ShardInterval *partitionShard = NULL; + forboth_ptr(parentShard, parentShardList, partitionShard, partitionShardList) + { + char *parentShardName = pstrdup(parentRelationName); + char *partitionShardName = pstrdup(partitionRelationName); + Task *task = CitusMakeNode(Task); + + AppendShardIdToName(&parentShardName, parentShard->shardId); + AppendShardIdToName(&partitionShardName, partitionShard->shardId); + + task->jobId = jobId; + task->taskId = taskId++; + task->taskType = DDL_TASK; + SetTaskQueryString(task, + psprintf("ALTER TABLE %s DETACH PARTITION %s CONCURRENTLY", + quote_qualified_identifier(parentSchemaName, parentShardName), + quote_qualified_identifier(partitionSchemaName, + partitionShardName))); + task->replicationModel = REPLICATION_MODEL_INVALID; + task->anchorShardId = parentShard->shardId; + task->cannotBeExecutedInTransaction = true; + SetInterShardDDLTaskPlacementList(task, parentShard, partitionShard); + SetInterShardDDLTaskRelationShardList(task, parentShard, partitionShard); + + taskList = lappend(taskList, task); + } + + return taskList; +} + + /* * CreateRightShardListForInterShardDDLTask is a helper function that creates * shard list for the right relation for InterShardDDLTaskList. diff --git a/src/test/regress/expected/detach_partition_concurrently.out b/src/test/regress/expected/detach_partition_concurrently.out new file mode 100644 index 00000000000..a63926a1dd2 --- /dev/null +++ b/src/test/regress/expected/detach_partition_concurrently.out @@ -0,0 +1,45 @@ +CREATE SCHEMA detach_partition_concurrently; +SET search_path TO detach_partition_concurrently; +SET citus.shard_count TO 2; +SET citus.shard_replication_factor TO 1; +CREATE TABLE parent (a int) PARTITION BY RANGE (a); +CREATE TABLE child PARTITION OF parent FOR VALUES FROM (0) TO (10); +SELECT create_distributed_table('parent', 'a'); + create_distributed_table +--------------------------------------------------------------------- + +(1 row) + +ALTER TABLE parent DETACH PARTITION child CONCURRENTLY; +SELECT relispartition +FROM pg_class +WHERE oid = 'child'::regclass; + relispartition +--------------------------------------------------------------------- + f +(1 row) + +SELECT result +FROM run_command_on_workers($$ + SELECT count(*) + FROM pg_inherits i + JOIN pg_class p ON p.oid = i.inhparent + WHERE p.relname LIKE 'parent\_%' +$$) +ORDER BY result; + result +--------------------------------------------------------------------- + 0 + 0 +(2 rows) + +INSERT INTO child VALUES (1); +SELECT * FROM child; + a +--------------------------------------------------------------------- + 1 +(1 row) + +DROP TABLE child; +DROP TABLE parent; +DROP SCHEMA detach_partition_concurrently; diff --git a/src/test/regress/expected/isolation_detach_partition_concurrently.out b/src/test/regress/expected/isolation_detach_partition_concurrently.out new file mode 100644 index 00000000000..8aba010ae5a --- /dev/null +++ b/src/test/regress/expected/isolation_detach_partition_concurrently.out @@ -0,0 +1,87 @@ +Parsed test spec with 3 sessions + +starting permutation: s1_begin s1_read s2_detach s3_cancel s3_pending s3_workers_attached s1_commit s3_finalize s3_done s3_workers_done +create_distributed_table +--------------------------------------------------------------------- + +(1 row) + +step s1_begin: BEGIN; +step s1_read: SELECT * FROM detach_parent; +a +- +(0 rows) + +step s2_detach: + ALTER TABLE detach_parent DETACH PARTITION detach_child CONCURRENTLY; + +step s3_cancel: + SELECT pg_cancel_backend(pid) + FROM pg_stat_activity + WHERE pid <> pg_backend_pid() + AND query LIKE + '%ALTER TABLE detach_parent DETACH PARTITION detach_child CONCURRENTLY%'; + +pg_cancel_backend +--------------------------------------------------------------------- +t +(1 row) + +step s2_detach: <... completed> +ERROR: canceling statement due to user request +step s3_pending: + SELECT inhdetachpending + FROM pg_inherits + WHERE inhparent = 'detach_parent'::regclass; + +inhdetachpending +--------------------------------------------------------------------- +t +(1 row) + +step s3_workers_attached: + SELECT result + FROM run_command_on_workers($$ + SELECT count(*) + FROM pg_inherits i + JOIN pg_class p ON p.oid = i.inhparent + WHERE p.relname LIKE 'detach_parent\_%' + $$) + ORDER BY result; + +result +--------------------------------------------------------------------- + 1 + 1 +(2 rows) + +step s1_commit: COMMIT; +step s3_finalize: + ALTER TABLE detach_parent DETACH PARTITION detach_child FINALIZE; + +step s3_done: + SELECT relispartition + FROM pg_class + WHERE oid = 'detach_child'::regclass; + +relispartition +--------------------------------------------------------------------- +f +(1 row) + +step s3_workers_done: + SELECT result + FROM run_command_on_workers($$ + SELECT count(*) + FROM pg_inherits i + JOIN pg_class p ON p.oid = i.inhparent + WHERE p.relname LIKE 'detach_parent\_%' + $$) + ORDER BY result; + +result +--------------------------------------------------------------------- + 0 + 0 +(2 rows) + diff --git a/src/test/regress/expected/pg14.out b/src/test/regress/expected/pg14.out index ae7b7b0d721..c9eeb6a18fc 100644 --- a/src/test/regress/expected/pg14.out +++ b/src/test/regress/expected/pg14.out @@ -220,14 +220,11 @@ CREATE STATISTICS s1 (dependencies) ON a, b FROM tbl1; CREATE STATISTICS s2 (mcv) ON a, b FROM tbl1; CREATE STATISTICS s3 (ndistinct) ON date_trunc('month', a), date_trunc('day', a) FROM tbl1; set citus.log_remote_commands to off; --- error out in case of ALTER TABLE .. DETACH PARTITION .. CONCURRENTLY/FINALIZE --- only if it's a distributed partitioned table +-- concurrent detach works for distributed partitioned tables CREATE TABLE par (a INT UNIQUE) PARTITION BY RANGE(a); CREATE TABLE par_1 PARTITION OF par FOR VALUES FROM (1) TO (4); CREATE TABLE par_2 PARTITION OF par FOR VALUES FROM (5) TO (8); --- works as it's not distributed ALTER TABLE par DETACH PARTITION par_1 CONCURRENTLY; --- errors out SELECT create_distributed_table('par','a'); create_distributed_table --------------------------------------------------------------------- @@ -235,9 +232,12 @@ SELECT create_distributed_table('par','a'); (1 row) ALTER TABLE par DETACH PARTITION par_2 CONCURRENTLY; -ERROR: ALTER TABLE .. DETACH PARTITION .. CONCURRENTLY commands are currently unsupported. -ALTER TABLE par DETACH PARTITION par_2 FINALIZE; -ERROR: ALTER TABLE .. DETACH PARTITION .. FINALIZE commands are currently unsupported. +SELECT relispartition FROM pg_class WHERE oid = 'par_2'::regclass; + relispartition +--------------------------------------------------------------------- + f +(1 row) + -- test column compression propagation in distribution SET citus.shard_replication_factor TO 1; CREATE TABLE col_compression (a TEXT COMPRESSION pglz, b TEXT); diff --git a/src/test/regress/isolation_schedule b/src/test/regress/isolation_schedule index cf99f17932b..316c7b48715 100644 --- a/src/test/regress/isolation_schedule +++ b/src/test/regress/isolation_schedule @@ -115,6 +115,9 @@ test: isolation_concurrent_move_create_table test: isolation_merge test: isolation_merge_replicated +# Concurrent partition detach and interrupted-operation recovery +test: isolation_detach_partition_concurrently + # Note: Always keep this test at the end test: isolation_check_mx diff --git a/src/test/regress/multi_schedule b/src/test/regress/multi_schedule index 0960831df83..d15e1282d28 100644 --- a/src/test/regress/multi_schedule +++ b/src/test/regress/multi_schedule @@ -41,7 +41,7 @@ test: ensure_no_intermediate_data_leak # ---------- # Tests for partitioning support # ---------- -test: multi_partitioning_utils multi_partitioning replicated_partitioned_table +test: multi_partitioning_utils multi_partitioning replicated_partitioned_table detach_partition_concurrently # ---------- diff --git a/src/test/regress/spec/isolation_detach_partition_concurrently.spec b/src/test/regress/spec/isolation_detach_partition_concurrently.spec new file mode 100644 index 00000000000..bc343771a1b --- /dev/null +++ b/src/test/regress/spec/isolation_detach_partition_concurrently.spec @@ -0,0 +1,88 @@ +setup +{ + SET citus.shard_count TO 2; + SET citus.shard_replication_factor TO 1; + ALTER SEQUENCE pg_catalog.pg_dist_shardid_seq RESTART 1490100; + + CREATE TABLE detach_parent (a int) PARTITION BY RANGE (a); + CREATE TABLE detach_child PARTITION OF detach_parent + FOR VALUES FROM (0) TO (10); + SELECT create_distributed_table('detach_parent', 'a'); +} + +teardown +{ + DROP TABLE detach_child; + DROP TABLE detach_parent; +} + +session "s1" +step "s1_begin" { BEGIN; } +step "s1_read" { SELECT * FROM detach_parent; } +step "s1_commit" { COMMIT; } + +session "s2" +step "s2_detach" +{ + ALTER TABLE detach_parent DETACH PARTITION detach_child CONCURRENTLY; +} + +session "s3" +step "s3_cancel" +{ + SELECT pg_cancel_backend(pid) + FROM pg_stat_activity + WHERE pid <> pg_backend_pid() + AND query LIKE + '%ALTER TABLE detach_parent DETACH PARTITION detach_child CONCURRENTLY%'; +} +step "s3_pending" +{ + SELECT inhdetachpending + FROM pg_inherits + WHERE inhparent = 'detach_parent'::regclass; +} +step "s3_workers_attached" +{ + SELECT result + FROM run_command_on_workers($$ + SELECT count(*) + FROM pg_inherits i + JOIN pg_class p ON p.oid = i.inhparent + WHERE p.relname LIKE 'detach_parent\_%' + $$) + ORDER BY result; +} +step "s3_finalize" +{ + ALTER TABLE detach_parent DETACH PARTITION detach_child FINALIZE; +} +step "s3_done" +{ + SELECT relispartition + FROM pg_class + WHERE oid = 'detach_child'::regclass; +} +step "s3_workers_done" +{ + SELECT result + FROM run_command_on_workers($$ + SELECT count(*) + FROM pg_inherits i + JOIN pg_class p ON p.oid = i.inhparent + WHERE p.relname LIKE 'detach_parent\_%' + $$) + ORDER BY result; +} + +permutation + s1_begin + s1_read + s2_detach(s3_cancel) + s3_cancel + s3_pending + s3_workers_attached + s1_commit + s3_finalize + s3_done + s3_workers_done diff --git a/src/test/regress/sql/detach_partition_concurrently.sql b/src/test/regress/sql/detach_partition_concurrently.sql new file mode 100644 index 00000000000..b6fa97967c8 --- /dev/null +++ b/src/test/regress/sql/detach_partition_concurrently.sql @@ -0,0 +1,30 @@ +CREATE SCHEMA detach_partition_concurrently; +SET search_path TO detach_partition_concurrently; +SET citus.shard_count TO 2; +SET citus.shard_replication_factor TO 1; + +CREATE TABLE parent (a int) PARTITION BY RANGE (a); +CREATE TABLE child PARTITION OF parent FOR VALUES FROM (0) TO (10); +SELECT create_distributed_table('parent', 'a'); + +ALTER TABLE parent DETACH PARTITION child CONCURRENTLY; + +SELECT relispartition +FROM pg_class +WHERE oid = 'child'::regclass; + +SELECT result +FROM run_command_on_workers($$ + SELECT count(*) + FROM pg_inherits i + JOIN pg_class p ON p.oid = i.inhparent + WHERE p.relname LIKE 'parent\_%' +$$) +ORDER BY result; + +INSERT INTO child VALUES (1); +SELECT * FROM child; + +DROP TABLE child; +DROP TABLE parent; +DROP SCHEMA detach_partition_concurrently; diff --git a/src/test/regress/sql/pg14.sql b/src/test/regress/sql/pg14.sql index aa9a30fa257..f2c12d3f906 100644 --- a/src/test/regress/sql/pg14.sql +++ b/src/test/regress/sql/pg14.sql @@ -50,17 +50,14 @@ CREATE STATISTICS s2 (mcv) ON a, b FROM tbl1; CREATE STATISTICS s3 (ndistinct) ON date_trunc('month', a), date_trunc('day', a) FROM tbl1; set citus.log_remote_commands to off; --- error out in case of ALTER TABLE .. DETACH PARTITION .. CONCURRENTLY/FINALIZE --- only if it's a distributed partitioned table +-- concurrent detach works for distributed partitioned tables CREATE TABLE par (a INT UNIQUE) PARTITION BY RANGE(a); CREATE TABLE par_1 PARTITION OF par FOR VALUES FROM (1) TO (4); CREATE TABLE par_2 PARTITION OF par FOR VALUES FROM (5) TO (8); --- works as it's not distributed ALTER TABLE par DETACH PARTITION par_1 CONCURRENTLY; --- errors out SELECT create_distributed_table('par','a'); ALTER TABLE par DETACH PARTITION par_2 CONCURRENTLY; -ALTER TABLE par DETACH PARTITION par_2 FINALIZE; +SELECT relispartition FROM pg_class WHERE oid = 'par_2'::regclass; -- test column compression propagation in distribution From 71e4944ccecb828bc52b9c75b6a9b5a74bab3d78 Mon Sep 17 00:00:00 2001 From: Andrey Borodin Date: Thu, 27 Aug 2026 14:12:20 +0500 Subject: [PATCH 2/4] Apply citus_indent --- src/backend/distributed/commands/table.c | 25 ++++++++++++------------ 1 file changed, 12 insertions(+), 13 deletions(-) diff --git a/src/backend/distributed/commands/table.c b/src/backend/distributed/commands/table.c index 60b1863c286..9cd9f86f688 100644 --- a/src/backend/distributed/commands/table.c +++ b/src/backend/distributed/commands/table.c @@ -117,7 +117,7 @@ static List * CreateRightShardListForInterShardDDLTask(Oid rightRelationId, Oid leftRelationId, List *leftShardList); static List * ConcurrentDetachPartitionTaskList(Oid parentRelationId, - Oid partitionRelationId); + Oid partitionRelationId); static void SetInterShardDDLTaskPlacementList(Task *task, ShardInterval *leftShardInterval, ShardInterval *rightShardInterval); @@ -1405,7 +1405,6 @@ PreprocessAlterTableStmt(Node *node, const char *alterTableCommand, SwitchToSequentialAndLocalExecutionIfConstraintNameTooLong( leftRelationId, constraint); } - /* * When constraint->indexname is not NULL we are handling an * ADD {PRIMARY KEY, UNIQUE} USING INDEX command. In this case @@ -1597,7 +1596,6 @@ PreprocessAlterTableStmt(Node *node, const char *alterTableCommand, } } } - /* * We check for ALTER COLUMN .. SET/DROP DEFAULT * we should not propagate anything to shards @@ -1681,7 +1679,8 @@ PreprocessAlterTableStmt(Node *node, const char *alterTableCommand, alterTableCommand = psprintf("ALTER TABLE %s DETACH PARTITION %s " "CONCURRENTLY", generate_qualified_relation_name(leftRelationId), - generate_qualified_relation_name(rightRelationId)); + generate_qualified_relation_name(rightRelationId) + ); } else if (AlterTableCommandTypeIsTrigger(alterTableType)) { @@ -1728,13 +1727,13 @@ PreprocessAlterTableStmt(Node *node, const char *alterTableCommand, if (concurrentPartitionDetach) { ddlJob->taskList = ConcurrentDetachPartitionTaskList(leftRelationId, - rightRelationId); + rightRelationId); ddlJob->warnForPartialFailure = true; } else { ddlJob->taskList = InterShardDDLTaskList(leftRelationId, rightRelationId, - alterTableCommand); + alterTableCommand); } } } @@ -2880,7 +2879,6 @@ PostprocessAlterTableStmt(AlterTableStmt *alterTableStatement) } } } - /* * We check for ALTER COLUMN .. SET DEFAULT nextval('user_defined_seq') * we should make sure that the type of the column that uses @@ -3034,7 +3032,6 @@ FixAlterTableStmtIndexNames(AlterTableStmt *alterTableStatement) FixPartitionShardIndexNames(relationId, parentIndexOid); } - /* * If this is an ALTER TABLE .. ATTACH PARTITION command * we have wrong index names generated on indexes of shards of @@ -4182,7 +4179,8 @@ ConcurrentDetachPartitionTaskList(Oid parentRelationId, Oid partitionRelationId) uint64 jobId = INVALID_JOB_ID; int taskId = 1; char *parentSchemaName = get_namespace_name(get_rel_namespace(parentRelationId)); - char *partitionSchemaName = get_namespace_name(get_rel_namespace(partitionRelationId)); + char *partitionSchemaName = get_namespace_name(get_rel_namespace(partitionRelationId)) + ; char *parentRelationName = get_rel_name(parentRelationId); char *partitionRelationName = get_rel_name(partitionRelationId); @@ -4203,10 +4201,11 @@ ConcurrentDetachPartitionTaskList(Oid parentRelationId, Oid partitionRelationId) task->taskId = taskId++; task->taskType = DDL_TASK; SetTaskQueryString(task, - psprintf("ALTER TABLE %s DETACH PARTITION %s CONCURRENTLY", - quote_qualified_identifier(parentSchemaName, parentShardName), - quote_qualified_identifier(partitionSchemaName, - partitionShardName))); + psprintf("ALTER TABLE %s DETACH PARTITION %s CONCURRENTLY", + quote_qualified_identifier(parentSchemaName, + parentShardName), + quote_qualified_identifier(partitionSchemaName, + partitionShardName))); task->replicationModel = REPLICATION_MODEL_INVALID; task->anchorShardId = parentShard->shardId; task->cannotBeExecutedInTransaction = true; From ca38c8d20d8f7575cf2ae95a584f973952e588eb Mon Sep 17 00:00:00 2001 From: Andrey Borodin Date: Thu, 27 Aug 2026 17:39:03 +0500 Subject: [PATCH 3/4] Fix style and test isolation --- src/backend/distributed/commands/table.c | 4 ++++ src/test/regress/multi_schedule | 3 ++- 2 files changed, 6 insertions(+), 1 deletion(-) diff --git a/src/backend/distributed/commands/table.c b/src/backend/distributed/commands/table.c index 9cd9f86f688..53e8195b7e0 100644 --- a/src/backend/distributed/commands/table.c +++ b/src/backend/distributed/commands/table.c @@ -1405,6 +1405,7 @@ PreprocessAlterTableStmt(Node *node, const char *alterTableCommand, SwitchToSequentialAndLocalExecutionIfConstraintNameTooLong( leftRelationId, constraint); } + /* * When constraint->indexname is not NULL we are handling an * ADD {PRIMARY KEY, UNIQUE} USING INDEX command. In this case @@ -1596,6 +1597,7 @@ PreprocessAlterTableStmt(Node *node, const char *alterTableCommand, } } } + /* * We check for ALTER COLUMN .. SET/DROP DEFAULT * we should not propagate anything to shards @@ -2879,6 +2881,7 @@ PostprocessAlterTableStmt(AlterTableStmt *alterTableStatement) } } } + /* * We check for ALTER COLUMN .. SET DEFAULT nextval('user_defined_seq') * we should make sure that the type of the column that uses @@ -3032,6 +3035,7 @@ FixAlterTableStmtIndexNames(AlterTableStmt *alterTableStatement) FixPartitionShardIndexNames(relationId, parentIndexOid); } + /* * If this is an ALTER TABLE .. ATTACH PARTITION command * we have wrong index names generated on indexes of shards of diff --git a/src/test/regress/multi_schedule b/src/test/regress/multi_schedule index d15e1282d28..3e57a88d0ad 100644 --- a/src/test/regress/multi_schedule +++ b/src/test/regress/multi_schedule @@ -41,7 +41,8 @@ test: ensure_no_intermediate_data_leak # ---------- # Tests for partitioning support # ---------- -test: multi_partitioning_utils multi_partitioning replicated_partitioned_table detach_partition_concurrently +test: multi_partitioning_utils multi_partitioning replicated_partitioned_table +test: detach_partition_concurrently # ---------- From 76746527673e97eb90aae23f4fda12fe31464813 Mon Sep 17 00:00:00 2001 From: Andrey Borodin Date: Thu, 27 Aug 2026 23:48:11 +0500 Subject: [PATCH 4/4] Make concurrent partition detach retryable A nontransactional detach can leave placements attached, pending, or already detached after a partial failure. Running the same command on every placement cannot recover all three states, and changing the coordinator first can make a retry impossible. Reconcile each placement before changing the coordinator: use CONCURRENTLY for attached shards, FINALIZE for pending shards, and skip detached shards. Add coverage for workers that completed different amounts of work and for retrying after cancellation. --- src/backend/distributed/commands/table.c | 118 +++++++++++++++--- .../distributed/commands/utility_hook.c | 20 +++ .../distributed/commands/utility_hook.h | 7 ++ .../detach_partition_concurrently.out | 46 +++++++ ...solation_detach_partition_concurrently.out | 13 +- ...olation_detach_partition_concurrently.spec | 10 +- .../sql/detach_partition_concurrently.sql | 41 ++++++ 7 files changed, 229 insertions(+), 26 deletions(-) diff --git a/src/backend/distributed/commands/table.c b/src/backend/distributed/commands/table.c index fb57a545888..4d5368852b1 100644 --- a/src/backend/distributed/commands/table.c +++ b/src/backend/distributed/commands/table.c @@ -37,6 +37,7 @@ #include "distributed/colocation_utils.h" #include "distributed/commands.h" #include "distributed/commands/utility_hook.h" +#include "distributed/connection_management.h" #include "distributed/coordinator_protocol.h" #include "distributed/deparse_shard_query.h" #include "distributed/deparser.h" @@ -52,6 +53,7 @@ #include "distributed/reference_table_utils.h" #include "distributed/relation_access_tracking.h" #include "distributed/relay_utility.h" +#include "distributed/remote_commands.h" #include "distributed/resource_lock.h" #include "distributed/tenant_schema_metadata.h" #include "distributed/version_compat.h" @@ -118,6 +120,9 @@ static List * CreateRightShardListForInterShardDDLTask(Oid rightRelationId, List *leftShardList); static List * ConcurrentDetachPartitionTaskList(Oid parentRelationId, Oid partitionRelationId); +static char * ConcurrentDetachPartitionCommand(ShardPlacement *placement, + const char *parentRelation, + const char *partitionRelation); static void SetInterShardDDLTaskPlacementList(Task *task, ShardInterval *leftShardInterval, ShardInterval *rightShardInterval); @@ -1731,6 +1736,7 @@ PreprocessAlterTableStmt(Node *node, const char *alterTableCommand, ddlJob->taskList = ConcurrentDetachPartitionTaskList(leftRelationId, rightRelationId); ddlJob->warnForPartialFailure = true; + ddlJob->executeBeforeLocalCommand = true; } else { @@ -4196,33 +4202,113 @@ ConcurrentDetachPartitionTaskList(Oid parentRelationId, Oid partitionRelationId) { char *parentShardName = pstrdup(parentRelationName); char *partitionShardName = pstrdup(partitionRelationName); - Task *task = CitusMakeNode(Task); AppendShardIdToName(&parentShardName, parentShard->shardId); AppendShardIdToName(&partitionShardName, partitionShard->shardId); + char *parentShardRelation = quote_qualified_identifier(parentSchemaName, + parentShardName); + char *partitionShardRelation = quote_qualified_identifier(partitionSchemaName, + partitionShardName); - task->jobId = jobId; - task->taskId = taskId++; - task->taskType = DDL_TASK; - SetTaskQueryString(task, - psprintf("ALTER TABLE %s DETACH PARTITION %s CONCURRENTLY", - quote_qualified_identifier(parentSchemaName, - parentShardName), - quote_qualified_identifier(partitionSchemaName, - partitionShardName))); - task->replicationModel = REPLICATION_MODEL_INVALID; - task->anchorShardId = parentShard->shardId; - task->cannotBeExecutedInTransaction = true; - SetInterShardDDLTaskPlacementList(task, parentShard, partitionShard); - SetInterShardDDLTaskRelationShardList(task, parentShard, partitionShard); + Task placementTask = { 0 }; + SetInterShardDDLTaskPlacementList(&placementTask, parentShard, partitionShard); - taskList = lappend(taskList, task); + ShardPlacement *placement = NULL; + foreach_declared_ptr(placement, placementTask.taskPlacementList) + { + char *command = ConcurrentDetachPartitionCommand(placement, + parentShardRelation, + partitionShardRelation); + if (command == NULL) + { + continue; + } + + Task *task = CitusMakeNode(Task); + + task->jobId = jobId; + task->taskId = taskId++; + task->taskType = DDL_TASK; + SetTaskQueryString(task, command); + task->replicationModel = REPLICATION_MODEL_INVALID; + task->anchorShardId = parentShard->shardId; + task->cannotBeExecutedInTransaction = true; + task->taskPlacementList = list_make1(placement); + SetInterShardDDLTaskRelationShardList(task, parentShard, partitionShard); + + taskList = lappend(taskList, task); + } } return taskList; } +/* + * Return the command needed to advance one placement's detach operation. A + * placement can be attached, pending after PostgreSQL's first detach + * transaction, or already detached after a previous partial attempt. + */ +static char * +ConcurrentDetachPartitionCommand(ShardPlacement *placement, + const char *parentRelation, + const char *partitionRelation) +{ + char *stateQuery = psprintf( + "SELECT COALESCE((SELECT CASE WHEN inhdetachpending THEN 'pending' " + "ELSE 'attached' END FROM pg_catalog.pg_inherits " + "WHERE inhparent = pg_catalog.to_regclass(%s) " + "AND inhrelid = pg_catalog.to_regclass(%s)), 'detached')", + quote_literal_cstr(parentRelation), quote_literal_cstr(partitionRelation)); + + int connectionFlags = OUTSIDE_TRANSACTION | FORCE_NEW_CONNECTION; + MultiConnection *connection = GetNodeUserDatabaseConnection(connectionFlags, + placement->nodeName, + placement->nodePort, + CurrentUserName(), NULL); + ClaimConnectionExclusively(connection); + + if (SendRemoteCommand(connection, stateQuery) == 0) + { + ReportConnectionError(connection, ERROR); + } + + bool raiseInterrupts = true; + PGresult *result = GetRemoteCommandResult(connection, raiseInterrupts); + if (!IsResponseOK(result)) + { + ReportResultError(connection, result, ERROR); + } + if (PQntuples(result) != 1 || PQnfields(result) != 1) + { + elog(ERROR, "unexpected result while checking partition detach state"); + } + + char *state = pstrdup(PQgetvalue(result, 0, 0)); + PQclear(result); + ForgetResults(connection); + UnclaimConnection(connection); + + if (strcmp(state, "attached") == 0) + { + return psprintf("ALTER TABLE %s DETACH PARTITION %s CONCURRENTLY", + parentRelation, partitionRelation); + } + else if (strcmp(state, "pending") == 0) + { + return psprintf("ALTER TABLE %s DETACH PARTITION %s FINALIZE", + parentRelation, partitionRelation); + } + else if (strcmp(state, "detached") == 0) + { + return NULL; + } + + elog(ERROR, "unexpected partition detach state: %s", state); + pg_unreachable(); +} + + /* * CreateRightShardListForInterShardDDLTask is a helper function that creates * shard list for the right relation for InterShardDDLTaskList. diff --git a/src/backend/distributed/commands/utility_hook.c b/src/backend/distributed/commands/utility_hook.c index 5c7eeb46364..1d95692bcca 100644 --- a/src/backend/distributed/commands/utility_hook.c +++ b/src/backend/distributed/commands/utility_hook.c @@ -776,6 +776,26 @@ citus_ProcessUtilityInternal(PlannedStmt *pstmt, { IncrementUtilityHookCountersIfNecessary(parsetree); + /* + * Some nontransactional DDL must reach a retryable state on workers before + * changing the coordinator. Keep all ordinary DDL in its traditional + * post-local-command execution order. + */ + List *remainingDDLJobs = NIL; + DDLJob *ddlJob = NULL; + foreach_declared_ptr(ddlJob, ddlJobs) + { + if (ddlJob->executeBeforeLocalCommand) + { + ExecuteDistributedDDLJob(ddlJob); + } + else + { + remainingDDLJobs = lappend(remainingDDLJobs, ddlJob); + } + } + ddlJobs = remainingDDLJobs; + /* * Check if we are running ALTER EXTENSION citus UPDATE (TO "") command and * the available version is different than the current version of Citus. In this case, diff --git a/src/include/distributed/commands/utility_hook.h b/src/include/distributed/commands/utility_hook.h index 00698554b5f..115b1002fb6 100644 --- a/src/include/distributed/commands/utility_hook.h +++ b/src/include/distributed/commands/utility_hook.h @@ -76,6 +76,13 @@ typedef struct DDLJob List *taskList; /* worker DDL tasks to execute */ + /* + * Whether worker tasks must finish before running the local command. This is + * used by nontransactional commands whose partial worker state must remain + * retryable if either worker or local execution fails. + */ + bool executeBeforeLocalCommand; + /* * Only applicable when any of the tasks cannot be executed in a * transaction block. diff --git a/src/test/regress/expected/detach_partition_concurrently.out b/src/test/regress/expected/detach_partition_concurrently.out index a63926a1dd2..24940ec9800 100644 --- a/src/test/regress/expected/detach_partition_concurrently.out +++ b/src/test/regress/expected/detach_partition_concurrently.out @@ -42,4 +42,50 @@ SELECT * FROM child; DROP TABLE child; DROP TABLE parent; +-- A retry must reconcile workers that completed different amounts of work. +CREATE TABLE retry_parent (a int) PARTITION BY RANGE (a); +CREATE TABLE retry_child PARTITION OF retry_parent FOR VALUES FROM (0) TO (10); +SELECT create_distributed_table('retry_parent', 'a'); + create_distributed_table +--------------------------------------------------------------------- + +(1 row) + +SELECT parent_shard.shardid AS parent_shard_id, + child_shard.shardid AS child_shard_id +FROM pg_dist_shard parent_shard +JOIN pg_dist_shard child_shard + ON child_shard.shardminvalue = parent_shard.shardminvalue + AND child_shard.shardmaxvalue = parent_shard.shardmaxvalue +JOIN pg_dist_placement placement + ON placement.shardid = parent_shard.shardid +JOIN pg_dist_node node + ON node.groupid = placement.groupid +WHERE parent_shard.logicalrelid = 'retry_parent'::regclass + AND child_shard.logicalrelid = 'retry_child'::regclass + AND node.nodeport = :worker_1_port +\gset +\c - - - :worker_1_port +ALTER TABLE detach_partition_concurrently.retry_parent_:parent_shard_id + DETACH PARTITION detach_partition_concurrently.retry_child_:child_shard_id + CONCURRENTLY; +\c - - - :master_port +SET search_path TO detach_partition_concurrently; +ALTER TABLE retry_parent DETACH PARTITION retry_child CONCURRENTLY; +SELECT result +FROM run_command_on_workers($$ + SELECT count(*) + FROM pg_inherits i + JOIN pg_class p ON p.oid = i.inhparent + WHERE p.relname LIKE 'retry_parent\_%' +$$) +ORDER BY result; + result +--------------------------------------------------------------------- + 0 + 0 +(2 rows) + +DROP TABLE retry_child; +DROP TABLE retry_parent; DROP SCHEMA detach_partition_concurrently; diff --git a/src/test/regress/expected/isolation_detach_partition_concurrently.out b/src/test/regress/expected/isolation_detach_partition_concurrently.out index 8aba010ae5a..4db28c1ec78 100644 --- a/src/test/regress/expected/isolation_detach_partition_concurrently.out +++ b/src/test/regress/expected/isolation_detach_partition_concurrently.out @@ -1,6 +1,6 @@ Parsed test spec with 3 sessions -starting permutation: s1_begin s1_read s2_detach s3_cancel s3_pending s3_workers_attached s1_commit s3_finalize s3_done s3_workers_done +starting permutation: s1_begin s1_read s2_detach s3_cancel s3_coordinator_attached s3_workers_attached s1_commit s3_retry s3_done s3_workers_done create_distributed_table --------------------------------------------------------------------- @@ -27,16 +27,19 @@ pg_cancel_backend t (1 row) +s2: WARNING: Commands that are not transaction-safe may result in partial failure, potentially leading to an inconsistent state. +If the problematic command is a CREATE operation, consider using the 'IF EXISTS' syntax to drop the object, +if applicable, and then re-attempt the original command. step s2_detach: <... completed> ERROR: canceling statement due to user request -step s3_pending: +step s3_coordinator_attached: SELECT inhdetachpending FROM pg_inherits WHERE inhparent = 'detach_parent'::regclass; inhdetachpending --------------------------------------------------------------------- -t +f (1 row) step s3_workers_attached: @@ -56,8 +59,8 @@ result (2 rows) step s1_commit: COMMIT; -step s3_finalize: - ALTER TABLE detach_parent DETACH PARTITION detach_child FINALIZE; +step s3_retry: + ALTER TABLE detach_parent DETACH PARTITION detach_child CONCURRENTLY; step s3_done: SELECT relispartition diff --git a/src/test/regress/spec/isolation_detach_partition_concurrently.spec b/src/test/regress/spec/isolation_detach_partition_concurrently.spec index bc343771a1b..8bdba77f250 100644 --- a/src/test/regress/spec/isolation_detach_partition_concurrently.spec +++ b/src/test/regress/spec/isolation_detach_partition_concurrently.spec @@ -36,7 +36,7 @@ step "s3_cancel" AND query LIKE '%ALTER TABLE detach_parent DETACH PARTITION detach_child CONCURRENTLY%'; } -step "s3_pending" +step "s3_coordinator_attached" { SELECT inhdetachpending FROM pg_inherits @@ -53,9 +53,9 @@ step "s3_workers_attached" $$) ORDER BY result; } -step "s3_finalize" +step "s3_retry" { - ALTER TABLE detach_parent DETACH PARTITION detach_child FINALIZE; + ALTER TABLE detach_parent DETACH PARTITION detach_child CONCURRENTLY; } step "s3_done" { @@ -80,9 +80,9 @@ permutation s1_read s2_detach(s3_cancel) s3_cancel - s3_pending + s3_coordinator_attached s3_workers_attached s1_commit - s3_finalize + s3_retry s3_done s3_workers_done diff --git a/src/test/regress/sql/detach_partition_concurrently.sql b/src/test/regress/sql/detach_partition_concurrently.sql index b6fa97967c8..717b8b32cd3 100644 --- a/src/test/regress/sql/detach_partition_concurrently.sql +++ b/src/test/regress/sql/detach_partition_concurrently.sql @@ -27,4 +27,45 @@ SELECT * FROM child; DROP TABLE child; DROP TABLE parent; + +-- A retry must reconcile workers that completed different amounts of work. +CREATE TABLE retry_parent (a int) PARTITION BY RANGE (a); +CREATE TABLE retry_child PARTITION OF retry_parent FOR VALUES FROM (0) TO (10); +SELECT create_distributed_table('retry_parent', 'a'); + +SELECT parent_shard.shardid AS parent_shard_id, + child_shard.shardid AS child_shard_id +FROM pg_dist_shard parent_shard +JOIN pg_dist_shard child_shard + ON child_shard.shardminvalue = parent_shard.shardminvalue + AND child_shard.shardmaxvalue = parent_shard.shardmaxvalue +JOIN pg_dist_placement placement + ON placement.shardid = parent_shard.shardid +JOIN pg_dist_node node + ON node.groupid = placement.groupid +WHERE parent_shard.logicalrelid = 'retry_parent'::regclass + AND child_shard.logicalrelid = 'retry_child'::regclass + AND node.nodeport = :worker_1_port +\gset + +\c - - - :worker_1_port +ALTER TABLE detach_partition_concurrently.retry_parent_:parent_shard_id + DETACH PARTITION detach_partition_concurrently.retry_child_:child_shard_id + CONCURRENTLY; + +\c - - - :master_port +SET search_path TO detach_partition_concurrently; +ALTER TABLE retry_parent DETACH PARTITION retry_child CONCURRENTLY; + +SELECT result +FROM run_command_on_workers($$ + SELECT count(*) + FROM pg_inherits i + JOIN pg_class p ON p.oid = i.inhparent + WHERE p.relname LIKE 'retry_parent\_%' +$$) +ORDER BY result; + +DROP TABLE retry_child; +DROP TABLE retry_parent; DROP SCHEMA detach_partition_concurrently;