Repository navigation
fix: make replay-safety explicit, and stop the sign test depending on string interning - #1403
Merged
Merged
Conversation
… string interning
Delivery from the embedded Debezium engine is AT-LEAST-ONCE, not
exactly-once. markBatchFinished() only REQUESTS an offset flush; the
engine honours it no more often than offset.flush.interval.ms (5000 in
the shipped configs), so the committed position in
altinity_sink_connector.replica_source_info lags the data already
written to ClickHouse, and a crash in that window re-delivers every
event after the last flushed position.
Measured on 2026-08-25 against ClickHouse 24.8.14 by hard-killing the
connector mid-flight (no graceful flush). The same batch AND the same
DDL were re-executed after restart:
05:29:57.959 EXECUTED BATCH Successfully Records: 3
05:29:58.090 Executed Source DB DDL: ... ADD COLUMN burstcol
--- hard kill + restart ---
05:30:03.553 EXECUTED BATCH Successfully Records: 3 <- re-sent
05:30:03.672 Executed Source DB DDL: ... ADD COLUMN burstcol
Nothing was corrupted, and the reason is worth stating precisely because
it is load-bearing: the APPLY is idempotent, not the delivery. Each row
carries a stable key (MySQL GIPK) as the ReplacingMergeTree sorting key
and the re-sent event regenerates the SAME _version, so the replayed
copy collapses onto the original instead of adding a row
(mysql=8 ch_raw=8 ch_live=8; per-row raw copies 1, distinct versions 1).
That property was undocumented, so nothing stopped a future change from
removing it. This commit records it where it is relied upon:
* DebeziumOffsetManagement#acknowledgeRecords now documents the flush
semantics, the measured replay, why it is currently harmless, and
the shape that would break it -- a version derived from wall-clock
time rather than the binlog coordinates, which would yield two
distinct versions for one event and stop collapsing.
* offset.flush.interval.ms is documented as what it actually is: the
replay window, with the tradeoff of lowering vs raising it.
Also fixes a latent trap found while tracing that path.
PreparedStatementFieldMapper#handleSignColumn tested the engine with
engine.getEngine() == TABLE_ENGINE.COLLAPSING_MERGE_TREE.getEngine()
which is reference equality on a String. It holds today only because
both sides resolve to the same interned literal from the enum. DBMetadata
derives the engine from SHOW CREATE TABLE, so a runtime-built String is
exactly the shape that makes it silently false -- and a false engine test
means the sign column is never bound, so no +1/-1 pair ever collapses.
Demonstrated:
OLD (==) interned side : true
OLD (==) jdbc side : false <- silently wrong
NEW (enum ==) both sides : true
Now compares the enum constant, which cannot degrade that way and matches
what PreparedStatementExecutor already does for the same engine. Behaviour
is unchanged today; this removes the trap rather than fixing a live miss.
Scope note: the connector never auto-creates CollapsingMergeTree --
auto-create emits only ReplacingMergeTree / ReplicatedReplacingMergeTree,
confirmed by reading the emitter and by the e2e run seeing RMT on all 10
tables. The additive-sign engine is reachable only for a pre-existing
table a user points the connector at.
Tests: ReplaySafetyTest (new, 2) pins that the engine test does not depend
on string interning, and that the auto-created engines stay replace-
semantics rather than additive -- an additive default would make the
observed replay corrupt instead of harmless.
minguyen9988
added a commit
that referenced
this pull request
Aug 25, 2026
…ity) (#1404) * fix: correct the replay-safety mechanism - it is version ORDERING, not equality #1403 documented WHY a replayed event is harmless and got the reason wrong on a load-bearing detail. It said the re-sent event "regenerates the SAME _version" so the copy collapses onto the original. It does not, and the real mechanism is stronger. On resume the sequence counter is seeded at SEQUENCE_START_INITIAL (500m) rather than SEQUENCE_START (1000m), precisely so a re-published event in the same source second is issued a strictly LOWER version than the pre-restart write of that same event. Combined with a stable row key (MySQL GIPK) as the ReplacingMergeTree sorting key, the replayed copy LOSES the version comparison and is discarded -- it never had a chance to overwrite the correct row. The measured outcome is unchanged (mysql=8 ch_raw=8 ch_live=8, per-row raw copies 1); the explanation for it was wrong. The distinction matters to anyone changing this path. "Same version" implies a tie, and a tie under ReplacingMergeTree leaves the winner up to merge order -- which would be a real hazard. The shipped design avoids ties entirely. A comment that describes a tie as the safety property invites someone to "simplify" the seeding away, which would convert a strict ordering into exactly the ambiguous case the 500m/1000m split exists to prevent. Also states the dependency that makes the ordering hold: the version is anchored to the SOURCE commit timestamp (source.ts_ms), identical on every re-delivery. Anchoring to any processing-side or wall-clock value would give the replayed copy a HIGHER version, so it would supersede the correct row silently, with row counts still matching. The anchoring rationale already exists in DebeziumChangeEventCapture#handleBatch; this cross-references it from the commit path where the consequence lands. The CollapsingMergeTree note is kept and sharpened: RMT resolves a duplicate by version so a losing replay is simply dropped, whereas Collapsing sign rows are additive (+1 twice sums to +2 and never cancels one -1). The connector never auto-creates that engine, so it is reachable only for a pre-existing table. Tests: ReplaySafetyTest 3 (1 new) now pins the ORDERING invariant -- a resumed version must rank strictly below a pre-restart version of the same source second, and must not tie with it. Existing coverage of the seeding itself (SourceTsVersionAnchorTest, 9) and the DDL guards (DdlReplayIdempotencyTest, 5; MySqlDDLParserListenerImplTest, 108) all still pass. * fix: withdraw the false ordering guarantee, and record the seed overflow The adversarial panel found a real defect in the previous commit on this branch, and it was right. That commit asserted a general ordering guarantee and added a test enforcing it. The guarantee does not hold, and the test cemented a data-loss bug as intended behaviour. The encoding is sourceTsMs * 1_000_000 + sequence, which leaves SIX decimal digits for the sequence. The seeds are TEN digits (SEQUENCE_START 1_000_000_000, SEQUENCE_START_INITIAL 500_000_000), so the addition carries into the timestamp field and acts as a ~1000 ms shift. Reproduced: older, pre-restart (T) -> T*1e6 + 1_000_000_000 = 1787635798000000000 newer, post-restart (T+1ms) -> (T+1)*1e6 + 500_000_000 = 1787635797501000000 newer < older => ReplacingMergeTree discards the NEWER row The diff > 1 second reset does not cover it: a 1 ms advance gives diff == 0, so the 500m seed is still in force. So a genuinely newer update landing just after a resume can be silently dropped in favour of stale data. What actually holds is narrower than what was written: for the SAME event re-delivered, the replayed copy ranks below the pre-restart write of that same event and loses, which is why the measured hard-kill test came out clean. That statement is kept. The generalisation to any ordering across a restart is withdrawn. Changes: * DebeziumOffsetManagement - the ordering paragraph is now scoped to a re-delivery of the same event, and a KNOWN DEFECT block records the overflow, the worked arithmetic, why the second-granularity reset does not catch it, and what fixing it requires (widen the multiplier or shrink the seeds, plus a migration story for existing versions). * ReplaySafetyTest - the test that pinned the false guarantee is REMOVED. In its place, testSequenceSeedsOverflowTheTimestampMultiplier asserts the defect EXISTS, and is written to start failing the moment the encoding is fixed so it is converted into the real guarantee then rather than being forgotten. Not fixed here deliberately: correcting the encoding changes every emitted _version, so it needs its own review and an upgrade path. This commit stops the wrong claim from shipping and makes the gap impossible to miss. Tests: ReplaySafetyTest 3 green (engine-identity, auto-create engines, and the new overflow record). * test: bind the overflow guard to the real constants instead of copies Both remaining judges made the same point and it was correct: the overflow test asserted arithmetic over its own hard-coded literals, so the expression 1_000_000_000L >= 1_000_000L is immutable and the test would keep passing even after someone fixed the encoding. It claimed to be a guard that fails when the defect is fixed, and it could not be. The constants live in DebeziumChangeEventCapture, in the lightweight module, and sink-connector cannot see them -- the module dependency runs the other way, so referencing them there is impossible, not merely inconvenient. The test therefore MOVES to the lightweight module as SequenceSeedOverflowTest, where it reads DebeziumChangeEventCapture.SEQUENCE_START and SEQUENCE_START_INITIAL directly and genuinely tracks them. Three assertions, all bound to production values: * the seed still overflows the multiplier (fails once the encoding is widened or the seeds shrunk -- the signal to invert the next one); * a newer post-resume event currently ranks BELOW an older pre-restart one, which is the user-visible data loss; * the resume seed stays below the normal seed -- this part is INTENDED and must survive the eventual fix, so a fix that inverts it by accident is caught. ReplaySafetyTest keeps only what its own module can verify (engine identity, auto-created engines) and points at the new class for the overflow. GPT-5.5 also asked that the defect be 74DD fixed rather than documented. Not done here on purpose: changing the encoding changes every emitted _version and needs an upgrade path for versions already stored, which is a behaviour change deserving its own review. The alternative -- leaving the wrong ordering claim from the previous commit in place -- is worse than shipping an accurate record with a guard that fires when it is fixed. Tests: ReplaySafetyTest 2, SequenceSeedOverflowTest 3, SourceTsVersionAnchorTest 9 -- all green. --------- Co-authored-by: Jenkins Continuous Build server <jenkins@fpif1-dbal2.w2k.jumptrading.com>
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.
Follow-up to #1402. While verifying that fix end to end, a hard-kill restart showed that the
IF EXISTSguards stop a replayed DDL from stalling the stream — but they do not stop the replay itself. This PR records what the delivery semantics actually are, and removes a latent trap on the one write path where a replay would not be harmless.Delivery is at-least-once, and that is load-bearing
markBatchFinished()only requests an offset flush. Debezium honours it no more often thanoffset.flush.interval.ms(5000 in the shipped configs), so the committed position inaltinity_sink_connector.replica_source_infolags the data already written to ClickHouse. A crash in that window re-delivers every event after the last flushed position.Measured on ClickHouse 24.8.14 by hard-killing the connector mid-flight (no graceful flush). The same batch and the same DDL were re-executed after restart:
Nothing was corrupted — and the reason matters, because it is the property everything rests on: the apply is idempotent, not the delivery. Each row carries a stable key (MySQL's generated invisible primary key) as the ReplacingMergeTree sorting key, and the re-sent event regenerates the same
_version, so the replayed copy collapses onto the original rather than adding a row:ch_raw == ch_liveis the part worth noting — no duplicate was even physically stored.That property was undocumented, so nothing stopped a future change from removing it. This PR records it at the two places it is relied upon:
DebeziumOffsetManagement#acknowledgeRecords— the flush semantics, the measured replay, why it is currently harmless, and the shape that would break it: a version derived from wall-clock time rather than the binlog coordinates would produce two distinct versions for one event and stop collapsing.offset.flush.interval.ms— documented as what it actually is, the replay window, with the tradeoff of lowering versus raising it.A latent trap on the sign-column path
Found while tracing that path.
PreparedStatementFieldMapper#handleSignColumntested the engine with:That is reference equality on a
String. It holds today only because both sides resolve to the same interned literal from the enum.DBMetadataderives the engine fromSHOW CREATE TABLE, so a runtime-builtStringis exactly the shape that makes it silently false — and a false engine test means the sign column is never bound, so no+1/-1pair ever collapses.Demonstrated:
Now compares the enum constant, which cannot degrade that way and matches what
PreparedStatementExecutoralready does for the same engine. Behaviour is unchanged today — this removes the trap rather than fixing a live miss.Scope note
The connector never auto-creates
CollapsingMergeTree. Auto-create emits onlyReplacingMergeTree/ReplicatedReplacingMergeTree— confirmed by reading the emitter and by the end-to-end run seeing RMT on all 10 tables. The additive-sign engine is reachable only for a pre-existing table a user points the connector at. That is precisely why the replay observed above was harmless, and why it would not be on an additive engine:+1delivered twice sums to+2and never cancels against a single-1.Tests
ReplaySafetyTest(new, 2) pins the two invariants idempotency rests on:Stringis a distinct reference — the precondition that makes==unsafe — while the enum comparison stays correct);What this PR does not do
It does not eliminate the replay window. Closing it properly means driving restart strictly from the committed position and shrinking or removing the gap between apply and offset flush — a larger change to the commit path that deserves its own review. This PR makes the current semantics explicit and safe to build on; it does not pretend the window is gone.