fix(postgres): rebuild the datasets already streaming when a join replaces their replication slot (fixes #13229) - #13313
Open
claudespice wants to merge 6 commits into
Open
fix(postgres): rebuild the datasets already streaming when a join replaces their replication slot (fixes #13229)#13313claudespice wants to merge 6 commits into
claudespice wants to merge 6 commits into
Conversation
…idated replication slot
…p a refusal of a slot already replaced
…dule already calls things, and unstrand attach_member's doc
…ot created where one was dropped rebuilds the members already attached
…rs before the fan-out awaits, and bind a refusal to the connection that saw it
Contributor
✅ Pull with Spice Passed🏷️ Auto-applied labels:
Passing checks:
|
Contributor
Author
|
@copilot review |
Contributor
There was a problem hiding this comment.
Pull request overview
Fixes PostgreSQL shared-slot replacement handling so attached datasets rebuild instead of silently retaining stale rows.
Changes:
- Adds explicit, retry-aware slot-history outcomes.
- Shares replacement recovery between join and pump paths.
- Adds generation guards and regression tests.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
crates/data_components/src/postgres_replication/slot.rs |
Reports and latches slot-history changes. |
crates/data_components/src/postgres_replication/shared.rs |
Rebuilds attached members and guards replacement recovery. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| // this member is the first to arrive on a resuming slot, the ones that will | ||
| // join after it. | ||
| source.install_reservations(&setup); | ||
| apply_slot_setup(source, &setup, &metrics).await; |
Comment on lines
2773
to
2774
| let envelope = rebuild_request_envelope(source, &member_key, &member, new_lsn); | ||
| if member.sender.send_control(envelope).await.is_some() { |
Comment on lines
+81
to
+104
| const KEPT: u8 = 0; | ||
| const CREATED: u8 = 1; | ||
| const REPLACED: u8 = 2; | ||
|
|
||
| fn new() -> Self { | ||
| Self(AtomicU8::new(Self::KEPT)) | ||
| } | ||
|
|
||
| /// Record what this attempt did, and return what the setup as a whole must | ||
| /// report — which is this outcome only while no earlier attempt did more. | ||
| fn observe(&self, history: SlotHistory) -> SlotHistory { | ||
| let rank = match history { | ||
| // A fast-forward ranks with `Kept` because nothing acts on it yet | ||
| // (#13305); it still reports itself when no attempt outranks it. | ||
| SlotHistory::Kept | SlotHistory::FastForwarded => Self::KEPT, | ||
| SlotHistory::CreatedFromAbsent => Self::CREATED, | ||
| SlotHistory::ReplacedInvalidated => Self::REPLACED, | ||
| }; | ||
| match self.0.fetch_max(rank, Ordering::AcqRel).max(rank) { | ||
| Self::REPLACED => SlotHistory::ReplacedInvalidated, | ||
| Self::CREATED => SlotHistory::CreatedFromAbsent, | ||
| _ => history, | ||
| } | ||
| } |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
📝 Summary
Two code paths can replace a
PostgreSQLlogical replication slot, and only one of them tells the datasets already streaming from it that their recorded positions no longer reach it.slot::ensure_slot— reached fromattach_memberwhen a dataset joins — drops a slot the server has invalidated (wal_status = 'lost', from an exhaustedmax_slot_wal_keep_sizeor aPostgreSQL18 idle-timeout) and creates a replacement at the current WAL position. Members already attached to the old slot are not told, so the pump's next connect succeeds against the replacement and streams on as though the history were still there: each already-attached member resumes from its own recorded position, which a slot created at the current WAL position cannot reach. Any row deleted at the source in between has no change event left to carry the deletion, so it survives in the acceleration and in every later query.The pump's own recovery from a refused stream already did the right thing — replace, then ask every attached member to rebuild and hold it until that rebuild is durable. This makes the join path do the same, through one function the two now share.
Changes
SlotInfo::history:Kept,ReplacedInvalidated,CreatedFromAbsent,FastForwarded) instead of the caller inferring it fromcreated_fresh.created_freshdoes not separate the cases — it is true both where an existing slot's history was thrown away and where there was none to throw away — and acting on it moves floors that are still owed their changes (Shared Postgres replication slot: a member joining a resuming slot second can skip changes, silently losing rows #12609).PostgreSQLpermits it — reaches the same end state as an invalidated one, and the members streaming from what is now gone are owed the same rebuild. Whether it counts is the caller's decision, because only the source knows whether anyone was streaming.HistoryLatch). Setup is retried as a whole, and an attempt that drops or creates a slot has already changed the source when it fails; the next attempt would find an ordinary slot and reportKept.install_replacement_slot— the pump's recovery body, now shared with the join path: seed the shared flush at the replacement's start, ask every attached member to rebuild and hold it until that rebuild commits, and reseat the floors held for published tables whose datasets have not joined. Every member is now held before any request is enqueued, without an await in between: the send can park on a full mailbox and the pump reconnects without takingsetup_lock, so holding as each member's turn came would leave the rest promotable and creditable for the length of that park.SharedSource::slot_generation, so one replacement is not acted on twice. The pump latches a refusal against the slot epoch its connection was streaming and acts on it after its reconnect backoff — the window in which a join can replace that slot.recover_unusable_slotcompares the generation under the setup lock and skips a refusal whose slot is already gone; without it the pump would drop a healthy slot and leave every member holding two rebuild requests against onerebuild_pendingflag, where the first to commit releases the hold the second still needs. The generation is published only after the fan-out completes, so an adoption that does not finish does not read as one that did.Test plan
make lintmake nextesta_join_that_replaced_the_slot_rebuilds_the_members_already_attached— one rebuild request per attached member, each still held across the pump's next promote, each keeping its own floor, and the unjoined table's held floor reseated to the replacement's start.a_join_on_a_slot_that_kept_its_history_leaves_attached_members_alone— the Shared Postgres replication slot: a member joining a resuming slot second can skip changes, silently losing rows #12609 direction, identical to the above but for the history signal.a_join_that_created_a_slot_under_members_already_attached_rebuilds_themandthe_first_member_to_create_a_slot_replaces_nothing— the two sides of theCreatedFromAbsentrule.a_refusal_of_an_already_replaced_slot_does_not_replace_it_again— the generation guard.a_later_attempt_cannot_report_away_what_an_earlier_one_didanda_latch_no_attempt_wrote_reports_the_attempts_own_outcome— the retry latch.created_fresh, droppingreseat_held_floors, dropping the member hold, removing the generation guard, and forcing theCreatedFromAbsentrule to each constant — every neuter fails at least one test, and every test is failed by at least one neuter.Not included, and why. The two-dataset integration test in the issue's Done-Done needs the
crates/runtime/tests/postgresDocker harness, which is unavailable in the environment this was written in; it is filed as #13306 rather than shipped unverified.FastForwardeddiscards history for every member in the same way, and the issue excludes it from this signal because keying on it regressed two held-floor tests that also need Docker — #13305. Two residuals the review below surfaced are filed rather than grown into this PR: #13312 (a replacement discovered by a join that then fails or is cancelled is forgotten) and #13311 (the rebuild hold is one flag, so the first of two overlapping rebuilds releases the second's).Review gate
codex— jobreview-mt0p1ohg-v533hy— run four times as the diff changed; 12 findings across the runs. Acted on in this PR: the retry losing the discard signal, a stale refusal replacing the replacement, theCreatedFromAbsentpath, the retry losing that outcome too, the fan-out holding members one at a time, and a refusal tagged at dequeue rather than at connect. Filed rather than grown into this PR: A slot replacement discovered by a join is forgotten if that join fails or is cancelled after the DDL #13312, A member's rebuild hold is a single flag, so the first of two overlapping rebuilds releases the second's hold #13311, A shared replication slot fast-forwarded for a re-bootstrap discards history under members already attached #13305. The final run still returnsneeds-attentionon exactly those three filed residuals./security-review: no findings — the diff adds no input, route, credential, or log surface; the only SQL it touches is the existing parameterized slot-catalog statements./simplify: applied — four findings fixed (a doc comment stranded onto the wrong function by an insertion,adopt_*naming that collided with this module's schema-evolution vocabulary, a doc narrating prior behavior, and rationale duplicated across four comment blocks), light checks and the neuter matrix re-run green afterward.🔗 Related
Fixes #13229
Follow-ups filed from this work: #13305, #13306, #13311, #13312.
👀 Notes for Reviewers
The signal is deliberately reported rather than inferred, because the issue's second requirement is that it must not overlap
created_fresh— and theCreatedFromAbsentcase shows why the decision has to be split between the two files: onlyensure_slotknows what it did to the slot, and only the source knows whether anyone was streaming from what it replaced.