Skip to content

Pace replication intake without releasing slot ownership - #14

Draft
olivermt wants to merge 4 commits into
baselabs:mainfrom
olivermt:fix/cdc-backpressure
Draft

olivermt wants to merge 4 commits into
baselabs:mainfrom
olivermt:fix/cdc-backpressure

Conversation

@olivermt

Copy link
Copy Markdown

WAL-position distance can grow because of unrelated writes or segment switches, even when Replicant has little queued data. A slow sink currently trips a permanent halt instead of allowing the stream to catch up.

This uses the pause/resume API merged in elixir-ecto/postgrex#799 to pace intake by queued payload bytes and message count while retaining the connection and slot. Feedback continues while paused, and processing credits remain separate from durable acknowledgements.

Retained transactions and batches have their own budget: spill or flush where possible, otherwise halt on actual exhaustion. Incremental snapshot pacing uses retained data rather than WAL distance. The port preserves current decoder support and includes coverage for multi-message wal2json payloads.

Submitting this as a draft until Postgrex has a release containing the pause/resume API. Once that lands, I will update the dependency, force-push the branch, and mark this PR ready for review.

This code has been running in prod for a month or so now with no issues.
Originally bumped into the problem after an importer flushed a couple of thousand rows in one write and the replicator fell over.

@olivermt
olivermt marked this pull request as draft October 22, 2026 12:39
@coderabbitai

coderabbitai Bot commented Oct 22, 2026 •

Copy link
Copy Markdown

Important

Draft PR not reviewed

Draft PRs are not automatically reviewed by default.

  • Trigger a manual review

To automatically review draft PRs, update your CodeRabbit configuration:

reviews:
  auto_review:
    drafts: true
  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Tip

🎯 Customize your risk assessments

Guide how CodeRabbit assesses merge risk and security architecture risk. Highlight sensitive changes and explain your security boundaries using Merge Risk Instructions and Security Risk Instructions in your review settings. Security architecture risk assessments require the Advanced plan or above.

✅ Automatic approvals, with your rules

Let CodeRabbit approve pull requests once required threads are resolved, pre-merge checks pass, and your risk limits are met. Enable Automatic approvals in review settings, with overrides for teams or file paths.

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

Share

CodeRabbit Verify

  • Run CodeRabbit Verify to test the changes in this pull request.

@olivermt

Copy link
Copy Markdown
Author

I have put the pr in draft mode until postgrex has a release btw. Created it as a full pr by accident :)

@olivermt

olivermt commented Nov 2, 2026

Copy link
Copy Markdown
Author

@palermo-git anyone home? :D

@palermo-git

Copy link
Copy Markdown
Contributor

Hey Oliver, someone's home! Sorry for the slow reply. This got buried in my GitHub notifications and I only just saw it.

Thanks for putting this together. An importer flushing a few thousand rows shouldn't take the replicator down, and a month in production is useful evidence. I haven't been through the code yet, so I don't have feedback on the approach.

Your plan works for me: keep it as a draft until Postgrex ships a Hex release with #799 (the latest is still 0.22.4, from before that merged), then update the dependency and mark it ready. I'll approve CI and review it then.

Heads up that CI on our main is red at the moment, which isn't from your change. I'll get that sorted so it doesn't muddy your run.

If you'd like a read on the approach before the release, let me know and I'll look sooner.

@palermo-git

Copy link
Copy Markdown
Contributor

I took an early look, since this is a big change to how Replicant handles backpressure. The overall approach looks right to me. Pausing on queued payload instead of halting on WAL distance fixes a real problem, and keeping credits separate from durable acks is the right call.

This comes from reading the code with an AI assistant. We haven't run it, and CI hasn't run on it yet, so some of these may be wrong. Please push back on anything that doesn't hold up. Line numbers are from 81ac1a7.

The main one I'd like your view on

A permanently stuck sink now pauses forever (connection.ex:1245). The feedback timer keeps wal_sender_timeout from firing, :paused fires only once, and the slot keeps holding WAL. With the default max_slot_wal_keep_size of -1, that can eventually fill the primary's disk. The old :sink_too_slow halt was blunt, but it was loud. I'd like a pause deadline, or at least repeated telemetry while paused. What do you see in prod when a sink actually hangs?

I'd want these fixed before merging

  • Batching cost (assembler.ex:219): buffered_bytes runs external_size over every retained batch transaction on every message, on the serial applier, so it costs the most when the sink is already behind. A running counter would avoid that. max_span may already keep the batch small enough that this branch rarely matters.
  • Snapshot pacing (assembler_server.ex:863): paced? can't see the mailbox. In per-transaction delivery, buffered_bytes is about 0 even with a full queue, so backfill keeps going during a backlog, and stream drain is meant to have priority.
  • Non-streamed transactions with spill on (assembler_server.ex:449): the old ceiling was max_inflight_lag + max_spill_bytes. Now a transaction above max_inflight_lag that PostgreSQL didn't stream can't spill, so it halts, and it halts again on every replay. For example, a 16 MiB limit with the default 64 MB logical_decoding_work_mem and a 32 MiB transaction.
  • Changelog (CHANGELOG.md:12): removing :sink_too_slow and changing what max_inflight_lag means are contract changes, so they should go under Changed with migration notes. Anyone alerting on :sink_too_slow won't get an error, the alert just stops firing. config.ex:377, the lag comment in telemetry.ex, and the docs in test/support still describe the old behavior.

Narrower cases

  • Reconnects (connection.ex:386): FlowControl.new forgets frames still queued in the assembler, so a stuck sink plus a flapping connection can add another full queue on each reconnect.
  • Spilling message-heavy streams (streaming.ex:51): spill keeps message bytes resident and only spills one buffer per observation, but check_buffer halts right away if the total is still over. With A at 0.7 of the limit in messages and B at 0.4 in changes, it spills A, frees almost nothing, and halts without trying B.
  • Subtransaction aborts (streaming.ex:369): partial aborts drop the changes but not resident_bytes, so savepoint-heavy streams can hit :buffer_limit_exceeded with little actually retained.
  • Long open transactions (assembler_server.ex:865): spill starts above the full limit and pacing above half, so an open streamed transaction resting between the two can hold back snapshot windows until it finishes.
  • Mid-transaction flush (assembler_server.ex:448): the :max_buffer_bytes flush goes through dispatch({:flush}), which re-tracks the newest batch transaction in the current window. Calling do_flush directly looks safer.

Which of batching, spill and incremental snapshots does your prod setup use? That tells us how much of this the month in production has already exercised.

No rush on any of this, since it's still waiting on the Postgrex release. Thanks again for the PR!

@olivermt
olivermt force-pushed the fix/cdc-backpressure branch from 81ac1a7 to e71ae1e Compare November 3, 2026 11:37
@olivermt

olivermt commented Nov 3, 2026

Copy link
Copy Markdown
Author

Thanks for the detailed review. I’ve pushed a follow-up addressing these points and rebased onto current main.

For prolonged pauses, I chose repeated telemetry rather than an automatic disconnect: :pause_ongoing fires every five seconds with elapsed pause time and queued bytes/messages. Disconnecting alone wouldn’t release retained WAL. The migration notes now explicitly cover replacement alerts and server-side WAL retention limits.

The other changes are:

  • Running batch-byte accounting instead of repeatedly sizing retained transactions.
  • Snapshot pacing based on queued work, without blocking indefinitely on idle open transactions.
  • Preserved the spill-enabled allowance for non-streamed transactions, explicitly documenting that this uses memory. Added your 32 MiB transaction / 16 MiB limit case as a live PostgreSQL regression.
  • Reconnects wait for the old assembler queue before replay. Tested three backend disconnects while the sink remained blocked.
  • Spill selection considers reclaimable row payload and repeats when necessary.
  • Subtransaction aborts release resident accounting and discarded messages.
  • Buffer-triggered flushes no longer re-track an earlier transaction.

Local validation passed against stock PostgreSQL 15–18, alongside format, strict Credo, warnings-as-errors compilation, and Dialyzer. I also tested the fork with Evo Store’s full backend suite and its PostgreSQL-to-ClickHouse integration spike. Older/plugin-specific integration lanes weren’t run locally.

The checked-in Evo Store configuration uses incremental snapshots and library-owned checkpoints, without batching or spill configured, so I wouldn’t claim the production runtime covered those optional paths.

We also have a separate transport-liveness issue involving a silently dead connection after a VIP handover. This change doesn’t solve receive silence on an unpaused stream; I’m going to be opening a PR stacked onto this one with my suggested fixes for that scenario.

Leaving this as a draft with the Postgrex pause/resume commit still pinned.

@palermo-git

Copy link
Copy Markdown
Contributor

Thanks Oliver, this is a really solid follow-up. I went back through it against my list, and nearly everything looks resolved. As before, this comes from reading the code with an AI assistant, and we haven't run it yet. I'm approving the CI run now so we get a first result.

You've convinced me on the pause deadline: the old halt didn't release WAL either, so server-side retention limits are the right protection. I'd still like to close a gap in how a stuck sink gets noticed:

Stall signal. :pause_ongoing only fires during a pause, which leaves a few cases uncovered:

  • A hung sink on a quiet publication may never reach 512 frames, so it never pauses and emits nothing while the slot pins WAL.
  • A slow sink that drains within five seconds each cycle resets paused_at, so it never fires either. Slot monitoring has to catch that one, so I'd reword the README's advice to replace :sink_too_slow alerts with prolonged-pause alerts.
  • It's telemetry only, so with no handler attached there's no signal at all.

Could this become a stall event that fires when frames are queued and none has completed for N seconds, paused or not, plus a Logger.warning? I'd prefer that replace :pause_ongoing rather than add a second event, but I'm open if you see it differently.

Subtransaction messages. The abort path drops messages where xid == sub, but from reading PG17's source, streamed messages seem to carry the top-level xid (pgoutput_message uses txn->xid, while pgoutput_change uses change->txn->xid). If so, that filter never matches on real PostgreSQL, and the %Message{xid: 2} in the regression test isn't something the server sends. I haven't tested this. Could you try it live, with pg_logical_emit_message inside a rolled-back savepoint in a streamed transaction? If I'm right, please drop the filter and correct the changelog line. Delivery of those messages isn't made worse by your PR, so I'll track that separately.

Small cleanups, if you're in there anyway: the 3-tuple {:message, ...} clause in assembler_server.ex looks unused in lib, the "Post-halt: drop WAL" comment now sits above the wrong clauses, and ADR-0005 still describes the old received − floor − spilled formula.

Unless CI or a live run turns up something new, that's everything I'd ask for on this PR. Please keep the follow-up as its own commit when you force-push, so it stays easy to review. I'll handle the version when we release. These are contract changes, so I'm leaning toward 2.0.0.

Your transport-liveness PR is very welcome. One thing to watch there: keepalives stop while paused, so a receive-silence check mustn't fire during a pause.

@olivermt

olivermt commented Nov 3, 2026

Copy link
Copy Markdown
Author

Opened #15 with the suggested receive-silence watchdog fixes, stacked on this PR. GitHub cannot target our fork branch as the base here, so it currently targets main; I’ll rebase it once this PR merges.

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.

2 participants