[Andrey Borodin (x4m)]

Support ALTER TABLE ... DETACH PARTITION ... CONCURRENTLY for distributed
partitioned tables.

PostgreSQL implements concurrent detach using two transactions, so shard
commands cannot run inside worker_apply_inter_shard_ddl_command(). This
change instead builds top-level nontransactional commands for each colocated
pair of parent and partition shards. The detached table remains available as
an independent distributed table and can be dropped separately.

DETACH PARTITION ... FINALIZE propagates a full concurrent detach to the
workers. This lets it finish an operation canceled after PostgreSQL committed
the coordinator's pending-detach state but before Citus started worker DDL.

The tests cover a normal detach and that interruption window, including the
catalog state on both workers before and after FINALIZE.

One design question remains: nontransactional DDL can fail on only some
placements after the coordinator has completed. The patch currently uses the
same partial-failure warning mechanism as other concurrent Citus DDL. Should
we also make FINALIZE an idempotent placement-reconciliation operation before
merging this?

Fixes #5264

[Copilot]

Pull request overview

Adds support for PostgreSQL 14’s ALTER TABLE ... DETACH PARTITION ... CONCURRENTLY (and recovery via ... FINALIZE) for distributed partitioned tables in Citus. This fits into Citus’ DDL propagation layer by introducing a non-transactional worker execution path for this specific DDL, since PostgreSQL implements concurrent detach using multiple transactions.

Changes:

• Implement distributed concurrent partition detach by generating top-level, non-transactional per-(parent shard, partition shard) worker commands instead of using worker_apply_inter_shard_ddl_command().
• Treat DETACH PARTITION ... FINALIZE as a mechanism to run the full concurrent detach on workers (to complete operations interrupted after coordinator pending-detach state was committed).
• Add regression + isolation coverage for successful detach and the interruption/FINALIZE recovery window, and wire tests into schedules.

Reviewed changes

Copilot reviewed 9 out of 9 changed files in this pull request and generated no comments.

Show a summary per file

File	Description
src/backend/distributed/commands/table.c	Builds per-shard top-level DETACH PARTITION ... CONCURRENTLY tasks and routes FINALIZE through the same non-transactional worker path.
src/test/regress/sql/pg14.sql	Updates PG14 compatibility test to assert concurrent detach works on distributed partitioned tables.
src/test/regress/expected/pg14.out	Expected output updates for the new concurrent-detach behavior in pg14.sql.
src/test/regress/sql/detach_partition_concurrently.sql	New multi-node regression test validating detach behavior and post-detach usability of the detached table.
src/test/regress/expected/detach_partition_concurrently.out	Expected output for the new regression test.
src/test/regress/spec/isolation_detach_partition_concurrently.spec	New isolation test covering cancellation after coordinator pending-detach commit and recovery via FINALIZE.
src/test/regress/expected/isolation_detach_partition_concurrently.out	Expected isolation output for the new spec.
src/test/regress/multi_schedule	Schedules the new detach_partition_concurrently regression test.
src/test/regress/isolation_schedule	Schedules the new isolation test for concurrent detach recovery.

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[Copilot]

Pull request overview

Copilot reviewed 9 out of 9 changed files in this pull request and generated 1 comment.

[codecov]

Codecov Report

❌ Patch coverage is 86.74699% with 11 lines in your changes missing coverage. Please review.
✅ Project coverage is 88.72%. Comparing base (c5284e1) to head (5c69a92).
⚠️ Report is 1 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #8802      +/-   ##
==========================================
- Coverage   88.74%   88.72%   -0.02%     
==========================================
  Files         289      289              
  Lines       64992    65069      +77     
  Branches     8200     8206       +6     
==========================================
+ Hits        57676    57735      +59     
- Misses       4950     4963      +13     
- Partials     2366     2371       +5     

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[Onur Tirtir (onurctirtir)]

[HIGH]

Running these jobs before PrevProcessUtility means the workers are changed before PostgreSQL runs its own checks for this command on the coordinator. PreventInTransactionBlock() for DETACH CONCURRENTLY lives in ProcessUtilitySlow(), so it only fires after the shards and the MX shell tables are already detached.

Repro (2 workers, MX):

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');
INSERT INTO parent SELECT generate_series(0,9);

BEGIN;
ALTER TABLE parent DETACH PARTITION child CONCURRENTLY;
-- ERROR:  ALTER TABLE ... DETACH CONCURRENTLY cannot run inside a transaction block
ROLLBACK;

SELECT count(*) FROM parent;      -- 0, expected 10
SELECT count(*) FROM child;       -- 10
INSERT INTO parent VALUES (5);
-- ERROR:  no partition of relation "parent_102020" found for row

With citus.log_remote_commands on, you can see both shard DETACH ... CONCURRENTLY commands and both shell DETACH commands are sent before the coordinator raises the error. Afterwards the coordinator still says child is attached, but no worker does. The same thing happens inside a DO block, and from an MX worker in a tenant schema. In plain PostgreSQL the same statement just errors and changes nothing.

This broken state then spreads:

• ALTER TABLE parent ADD COLUMN z int does not reach the child shards, so SELECT z FROM child fails with column "z" does not exist.
• A block_writes shard move rebuilds the shard from the coordinator catalog and so re-attaches that one child shard. In my run, count(*) on the parent went from 0 to 7. So query results start to depend on which shards were moved in the past.

CIC (create index concurrently) does not have this problem because the local command runs first. PostgreSQL's checks fail before any worker is touched. Can we keep that order here? Or, at the very least, can we call PreventInTransactionBlock() ourselves (and do the other checks that PostgreSQL does for this command) before running anything on the workers?

[Onur Tirtir (onurctirtir)]

[HIGH]

Rewriting FINALIZE into CONCURRENTLY lets a FINALIZE that PostgreSQL would reject still fully detach the partition on the workers. That happens because the workers run before the coordinator's checks (see my comment in utility_hook.c).

Repro, with a partition that is not pending:

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');
INSERT INTO parent SELECT generate_series(0,9);

ALTER TABLE parent DETACH PARTITION child FINALIZE;
-- ERROR:  cannot complete detaching partition "child"
-- DETAIL:  There's no pending concurrent detach.

SELECT count(*) FROM parent;  -- 0, expected 10

After this, the coordinator still has child attached. Both workers have it detached, in the shards and in the shell tables. Plain PostgreSQL gives the same error but changes nothing.

I think FINALIZE should first check that the coordinator partition really is pending (inhdetachpending). Only after that check passes should anything be sent to the workers.

[Onur Tirtir (onurctirtir)]

[HIGH]

Retrying, or running FINALIZE, does not work once every shard placement is already detached. That is exactly the state you are in when the coordinator phase of the detach fails.

Here is why:

1. When every placement is already detached, ConcurrentDetachPartitionTaskList() returns an empty list.
2. With an empty list, TaskListCannotBeExecutedInTransaction() is false.
3. So ExecuteDistributedDDLJob() takes the transactional branch.
4. That branch sends metadataSyncCommand (DETACH ... CONCURRENTLY) to the MX workers inside BEGIN.
5. That fails on every worker.

Repro: interrupt phase 2 on the coordinator after the workers are done. I hold a lock on the coordinator parent only, from a second session.

-- session A
SET citus.enable_ddl_propagation TO off;
BEGIN; LOCK TABLE ONLY parent IN ACCESS SHARE MODE; SELECT pg_sleep(10); COMMIT;

-- session B
SET statement_timeout = '5s';
ALTER TABLE parent DETACH PARTITION child CONCURRENTLY;
-- ERROR:  canceling statement due to statement timeout
-- coordinator: child is PENDING. workers: shards and shells fully detached.

ALTER TABLE parent DETACH PARTITION child FINALIZE;
-- ERROR:  ALTER TABLE ... DETACH CONCURRENTLY cannot run inside a transaction block
-- CONTEXT:  while executing command on localhost:10802
ALTER TABLE parent DETACH PARTITION child CONCURRENTLY;
-- same error

So the FINALIZE recovery path does not work on an MX cluster, which is the default setup.

Sending the shell command bare would still not be enough. A shell table that is already detached rejects it with relation "child" is not a partition of relation "parent". I hit that on a retry where the shell tables were already detached but one shard group was not.

The shard placements are reconciled one by one, which is good. The shell tables need the same per-node attached/pending/detached reconciliation. That step must also always run outside a transaction block, the way CIC sends its shell command bare.

[Onur Tirtir (onurctirtir)]

[HIGH]

Allowing these commands means a distributed partition can now be left PENDING on the coordinator. Combined with the FINALIZE problem above, there is no supported way to get out of that state. Several Citus operations do not handle it.

Repro: same setup as above. Get the coordinator to PENDING by timing out phase 2 while another session holds LOCK TABLE ONLY parent IN ACCESS SHARE MODE. Then:

SELECT citus_activate_node('localhost', :worker_2_port);
-- ERROR:  relation 17492 has no parent because it's being detached

SELECT citus_move_shard_placement(...);   -- any shard in the parent's colocation group
-- ERROR:  relation 17492 has no parent because it's being detached

SELECT alter_distributed_table('parent', shard_count := 4);
-- succeeds, but only converts the other partition (child2).
-- The old parent is then dropped, and the pending `child` and its 10 rows are dropped with it.
-- Afterwards pg_class only has parent and child2.

So this one state blocks metadata sync, node activation, and shard moves. It also turns alter_distributed_table / undistribute_table into silent data loss for the pending partition.

Before this PR the state could not happen, because these commands raised an ERROR. Either FINALIZE needs to always be able to finish the coordinator side, or these code paths need to handle pending partitions (or at least error out clearly).

Suggestion:

1. Fix the empty-task-list / shell problem first (see my comment above), so that FINALIZE can always finish the coordinator side. That gives users a working way out of this state.
2. In the code paths that read the partition list (alter_distributed_table / undistribute_table, metadata sync in DetachPartitionCommandList(), and shard moves), detect partitions that have inhdetachpending = true. Raise a clear ERROR with a hint like "run ALTER TABLE ... DETACH PARTITION ... FINALIZE first". This is better than today's behavior: failing with an internal-looking message, or silently dropping the pending partition.
3. Add a regression test for each of these code paths while a partition is pending.

[Onur Tirtir (onurctirtir)]

[HIGH]

One more problem with running the worker tasks here. The coordinator still holds the ShareUpdateExclusiveLock on the parent that it took in preprocess (AlterTableLookupRelation). The worker-side DETACH ... CONCURRENTLY then waits for older snapshots on the shards (virtualxid). This combination produces a distributed deadlock that Citus does not detect.

Repro:

-- session X
SET statement_timeout = '25s';
BEGIN;
SELECT count(*) FROM parent;
SELECT pg_sleep(3);
CREATE INDEX ON parent (a);   -- waits on D's coordinator lock
COMMIT;

-- session D, started 1s after X
SET statement_timeout = '20s';
ALTER TABLE parent DETACH PARTITION child CONCURRENTLY;
-- D's shard DETACH on the worker waits on virtualxid for X's worker transaction

citus_lock_waits shows the cycle:

 blocked_statement                                            | current_statement_in_blocking_process
 CREATE INDEX ON p5.parent (a);                               | ALTER TABLE p5.parent DETACH PARTITION p5.child CONCURRENTLY
 ALTER TABLE p5.parent DETACH PARTITION p5.child CONCURRENTLY | CREATE INDEX ON p5.parent (a);

But SELECT check_distributed_deadlocks() returns f. The cycle only ends when statement_timeout fires. After that, X's CREATE INDEX fails with could not fix child index names: index is not partitioned, because the shards were left pending.

In plain PostgreSQL the equivalent wait is a local deadlock, and PostgreSQL detects it.

CIC avoids this with startNewTransaction. It commits the coordinator transaction, and so releases the coordinator locks, before it waits on the workers. Can we do something similar here? If not, can we write down a lock-ordering argument for why this cannot hang?

[Onur Tirtir (onurctirtir)]

[LOW]

This opens a new connection (FORCE_NEW_CONNECTION) and makes one round trip per placement. It does this one placement after another, while the task list is being built. On a 64-shard table I counted 64 state-check queries before any DETACH was sent.

Timings on that table:

• DETACH ... CONCURRENTLY: about 790 ms.
• Plain DETACH: about 55 ms.

Part of that difference is the normal cost of a concurrent detach. Still, the per-placement connection is avoidable.

Two options:

• Send one batched query per node that returns the state for all of that node's shard pairs.
• Do the state check inside each task's own command (for example, a small worker-side function that runs CONCURRENTLY or FINALIZE or nothing, based on the state).

The second option also removes the race between this check and the moment the task actually runs.

[Onur Tirtir (onurctirtir)]

[HIGH]

The COALESCE(..., 'detached') fallback also covers the case where the shard tables simply do not exist on that node, because to_regclass() returns NULL for them. A missing shard would then be skipped silently as if it were "already detached".

Could we tell these two cases apart? Return a separate state when either to_regclass() is NULL, and raise an ERROR for it.

[Onur Tirtir (onurctirtir)]

[MEDIUM]

This counts pg_inherits rows, so it cannot tell a pending shard from an attached one. Both have a row. Could we select inhdetachpending here instead, the same way s3_coordinator_attached does?

Also, none of the tests look at the MX shell tables on the workers (detach_parent itself, not detach_parent_<shardid>). That is where the retry/FINALIZE problems show up. Something like this, run through run_command_on_workers:

SELECT c.relname, i.inhdetachpending
FROM pg_inherits i
JOIN pg_class c ON c.oid = i.inhrelid
WHERE i.inhparent IN ('detach_parent'::regclass)
ORDER BY 1;

It would also be good to add a permutation that cancels the coordinator phase. For example, block it with a session that holds LOCK TABLE ONLY detach_parent IN ACCESS SHARE MODE with DDL propagation off. Then run FINALIZE and check that it finishes. Right now that permutation would fail (see my comment on the empty task list).

[Onur Tirtir (onurctirtir)]

[MEDIUM]

After this change, no test in the PR runs DETACH PARTITION ... FINALIZE anymore. This line used to be the only one that did. The PR description says FINALIZE is covered, so I think either the description or the tests need updating.

Could we add a FINALIZE test for each of these cases?

• The coordinator is pending and the workers are already detached. This is the main recovery case.
• Some shards are pending.
• FINALIZE on a partition that is not pending. This should error and leave every node unchanged.

[Onur Tirtir (onurctirtir)]

[MEDIUM]

A few cases are missing here. All of them currently misbehave:

-- must error and leave every node unchanged (today the workers get detached)
BEGIN;
ALTER TABLE parent DETACH PARTITION child CONCURRENTLY;
ROLLBACK;
SELECT count(*) FROM parent;   -- should still return all rows

DO $$ BEGIN EXECUTE 'ALTER TABLE parent DETACH PARTITION child CONCURRENTLY'; END $$;

After each case, check pg_inherits (including inhdetachpending) on the coordinator, on the worker shards, and on the worker shell tables.

A failure-injection test modeled on failure_create_index_concurrently.sql would also help. It covers the partial-failure path that this PR makes reachable. For example:

SELECT citus.mitmproxy('conn.onQuery(query="DETACH PARTITION").kill()');
ALTER TABLE parent DETACH PARTITION child CONCURRENTLY;
SELECT citus.mitmproxy('conn.allow()');
-- check that the state is consistent and recoverable, then retry / FINALIZE
ALTER TABLE parent DETACH PARTITION child CONCURRENTLY;