Skip to content

Revert block-writes shard-move memory change from this PR#8652

Draft
ihalatci with Copilot wants to merge 3 commits into
mainfrom
copilot/fix-citus-move-shard-placement-memory-usage
Draft

Revert block-writes shard-move memory change from this PR#8652
ihalatci with Copilot wants to merge 3 commits into
mainfrom
copilot/fix-citus-move-shard-placement-memory-usage

Conversation

Copilot AI commented Jul 8, 2026

Copy link
Copy Markdown
Contributor

DESCRIPTION: Revert block-writes-only shard-move memory changes from this PR

This PR no longer includes the previous CopyShardTablesViaBlockWrites memory changes.
Based on review feedback, the earlier change was reverted because it does not affect the
logical-replication path used in the reported reproduction.

  • What changed

    • Reverted prior edits in src/backend/distributed/operations/shard_transfer.c.
    • Removed the block-writes-only memory optimization from this PR’s effective diff.
  • Scope

    • This PR is decoupled from addressing the logical-replication memory growth report.

Copilot AI changed the title [WIP] Fix memory usage in citus_move_shard_placement() Reduce coordinator memory growth in citus_move_shard_placement Jul 8, 2026
Copilot AI requested a review from ihalatci July 8, 2026 13:26
@codeforall

Copy link
Copy Markdown
Contributor

Thanks for the effort @copilot, but I don't think this change addresses the issue it's linked to (#8145).

Path mismatch. The reporter explicitly stated the move ran with shard_transfer_mode = auto and "it used logical replication for the job." That means the exercised path is CopyShardTablesViaLogicalReplication → LogicallyReplicateShards → CompleteNonBlockingShardTransfer. This PR only modifies CopyShardTablesViaBlockWrites, which that run never enters, so against the reported repro it's effectively a no-op.

The pattern is already handled on the real path. The logical-replication path already releases transient per-shard DDL memory everywhere via a localContext + per-iteration MemoryContextReset (e.g. ExecuteCreateConstraintsBackedByIndexCommands, CreatePartitioningHierarchy, index/FK creation, and the shard-table-creation loop in CopyShardTablesViaLogicalReplication itself). So the transient DDL buffering this PR removes isn't where the reported memory goes.

Data doesn't flow through the coordinator either. CopyShardsToNode dispatches SELECT worker_copy_table_to_node(...) tasks; the bytes are copied worker→worker. So a 70 GB coordinator peak is not data buffering, and it's not the per-shard command lists (a few KB × shards = MBs, not tens of GBs).

Given all transient contexts are already reset, the growth almost certainly lives in a session-lifetime context (CacheMemoryContext — relcache/catcache + Citus CitusTableCacheEntry for the whole colocation group of a deeply-partitioned 80 TB DB), or a genuine leak into a long-lived context, neither touched here.

Suggested next step before iterating on a fix: reproduce with logical replication on a many-partition colocation group and capture, during the climb:

SELECT * FROM pg_backend_memory_contexts ORDER BY total_bytes DESC LIMIT 30;

(or a MemoryContextStats(TopMemoryContext) dump). That will show which context actually dominates and point the fix at the right place.

The block-writes cleanup itself looks safe and reasonable as a standalone minor improvement, but I'd unlink it from #8145 rather than close the issue with it.

Copilot AI commented Jul 10, 2026

Copy link
Copy Markdown
Contributor Author

Thanks for the effort @copilot, but I don't think this change addresses the issue it's linked to (#8145).

Path mismatch. The reporter explicitly stated the move ran with shard_transfer_mode = a...

Addressed by reverting the block-writes-only change in commit 62a05d0, since it does not affect the logical-replication path used in #8145. This PR is now decoupled from closing #8145.

Copilot AI changed the title Reduce coordinator memory growth in citus_move_shard_placement Revert block-writes shard-move memory change from this PR Jul 10, 2026
Copilot AI requested a review from codeforall July 10, 2026 13:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

citus_move_shard_placement() uses lots of memory on the coordinator

3 participants