Uh oh!
There was an error while loading. Please reload this page.
Advance the global last seen LSN only on commit fragments - #4767
Open
alco wants to merge 1 commit into
Open
Conversation
Since transactions started being published as `TransactionFragment`s, every fragment of a large transaction carries the transaction's final LSN, and `ShapeLogCollector.publish/2` called `LsnTracker.set_last_processed_lsn/2` unconditionally after fanning each fragment out to consumers. Consumers, on the other hand, only make a transaction readable once they see its commit fragment. That left a window where a non-live shape request could be answered with an `up-to-date` control message advertising `global_last_seen_lsn = T` while no shape log contained any data for T yet, and a subsequent request could then return changes at that same LSN. Clients that use the global LSN to align multiple shape streams can durably commit T with one shape's changes missing. `state.last_processed_offset` still advances per fragment, as it is what orders and de-duplicates fragments internally; only the publicly visible LSN now moves in the commit-bearing branch, next to the broadcast that was already gated that way. Fixes#4755
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@## main #4767 +/- ##
==========================================
- Coverage 60.02% 60.02% -0.01%
==========================================
Files 397 397 Lines 43772 43772 Branches 12590 12590 ==========================================
- Hits 26275 26273 -2 - Misses 17416 17418 +2
Partials 81 81
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
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 freeto join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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.
Fixes#4755
Problem
Since transactions started being published as
TransactionFragments, every fragment of a large transaction carries the transaction's final LSN —MessageConvertersetstxn_fragment.lsn = msg.final_lsnonBeginand derives every change'slog_offsetfrom it.ShapeLogCollector.publish/2then calledLsnTracker.set_last_processed_lsn/2unconditionally after fanning each fragment out to consumers, before thecommit-bearing branch.Consumers, meanwhile, deliberately keep a transaction unreadable until they see its commit fragment:
maybe_complete_pending_txn/2short-circuits on%TransactionFragment{commit: nil}, andappend_fragment_to_logexplicitly does not advancelast_seen_txn_offset— onlysignal_txn_commit/2does.That leaves a window, for any transaction larger than
max_batch_size(default 100), where a non-live shape request is answered with anup-to-datecontrol message advertisingglobal_last_seen_lsn = Twhile no shape log contains any data forTyet. A later request can then return changes at that sameT. Clients that use the global LSN to align multiple shape streams can durably commitTwith one shape's changes missing, and only recover by refetching.The API's non-live branch is where this surfaces:
The premise held when the value was a per-transaction LSN; it stopped holding when it became a per-fragment one.
Fix
Move the public LSN update into the commit-bearing branch of
publish/2, next to thebroadcast_last_seen_lsncall that was already gated that way.state.last_processed_offsetstill advances per fragment — it is what orders and de-duplicates fragments internally — but only a commit fragment now moves the publicly visible LSN.One behavioural consequence worth flagging: the tracker update now lands after the undeliverable-shape reduction (
handle_writer_downclassification) rather than before it. That ordering seems the more correct one — crashed consumers are invalidated before the proof goes public — but it is a change from the previous sequence.Notes on the regression
The unconditional call dates to #3541, which replaced the
last_processed_lsnstate field (LSN of the last complete transaction, guarded byLsn.is_larger) withlast_processed_offset(a per-fragmentLogOffset, assigned unconditionally) and rewrote the tracker call asLsn.from_integer(state.last_processed_offset.tx_offset)— a type adaptation that silently changed what was being published.The bug has been live since then. #3783 did not introduce it: before that PR consumers reassembled fragments in memory via
TransactionBuilder, so data atTwas equally unreadable after a non-commit fragment. #3783 only moved the hidden data from a consumer buffer into storage behind a hidden read frontier.