fix(wal): fix symbol value corruption when skipping replaced transactions across DDL

[jovfer]

Problem

ApplyWal2TableJob.calculateSkipTransactionCount lets a future replace-range commit justify skipping an earlier data transaction. The scan walked past non-data transactions (structural metadata changes), so a skip decision could cross a DDL boundary. Which transactions get skipped depends on the apply lookahead window, so two appliers of the same WAL stream (e.g. a replication primary and replica, or a node replaying the full log) can skip different transaction sets and reach a DDL point with different intermediate table content.

ALTER COLUMN ... TYPE SYMBOL seeds the new column's symbol map from the rows present at apply time, and the WAL protocol treats symbol keys below cleanSymbolCount as table-global keys identical on every applier (remapWalSymbols only remaps keys at or above it). When the conversion sees different intermediate content on two appliers, they assign different keys to the same strings, and rows written against one map silently resolve to wrong symbol values on the other applier. A replication fuzz test caught this: the same row read SKXOL on one node and TRILVF on the other.

The TRUNCATE fast path had the same exposure: WAL TRUNCATE applies as a soft truncate (removeAllPartitions keeps symbol maps), and the old firstNonSkippableTxn logic allowed the skip-to-truncate jump to be computed across an intervening structural transaction.

Change

The future-transaction scan now stops at the first non-data transaction of any kind (structural metadata change or SQL). A replace-range can only justify skipping transactions within the same structural-change-free window, which guarantees every applier holds identical table content at each DDL point, restoring the invariant that symbol key assignment is independent of the WAL apply method. The TRUNCATE early exit remains, and is now only reachable when the whole window before it consists of data transactions, which makes firstNonSkippableTxn dead code (removed).

Effects

• Fixes silent symbol value divergence between appliers when replace-range commits interleave with ALTER COLUMN ... TYPE SYMBOL or TRUNCATE.
• The skip optimization becomes more conservative: a data transaction whose replacer sits beyond an interleaved DDL/SQL/TRUNCATE transaction is now applied normally instead of skipped. Mat view refresh and replace-range ingestion without interleaved DDL keep the previous skip behavior unchanged (testManyTransactionsSkippedWhenTruncateIfFound still passes with the same skip count).
• The inner scan now terminates earlier, never later, than before.

Test plan

• New testSkipDoesNotCrossSymbolConversion: applies the same commit sequence (data commits, STRING to SYMBOL conversion, more data, replace-range covering the early commits) under apply lookahead 1 and 10000 and asserts identical query results and identical symbol maps (key-to-string). Fails before the fix with symbol counts differ expected:<4> but was:<3>; passes after.
• New testSkipDoesNotCrossTruncate: same shape with TRUNCATE instead of the conversion; guards the soft-truncate path as an invariant test (it passes pre-fix because the old code computed the same skip count in this scenario).
• WalWriterReplaceRangeTest (45 tests), MatViewIdenticalReplaceTest (20), MatViewTest matches (292, 1 skipped), WalTableWriterFuzzTest (23) all pass.

🤖 Generated with Claude Code

[coderabbitai]

Important

Review skipped

Auto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: 14c3aa8f-5723-4a3b-9474-dd9abc7101fb

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

• 🔍 Trigger review

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch sm_fix_wal_skip_symbol_divergence

---

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

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[bluestreak01]

Approved.

The fix is correct, complete across all transaction types, and strictly more conservative (new skip count ≤ old). It closes a real apply-lookahead-dependence bug: ALTER COLUMN ... TYPE SYMBOL (walId = STRUCTURAL_CHANGE_WAL_ID = -1) was crossed by the old else { continue; } path, letting a future replace-range justify skipping pre-conversion rows and making the converted symbol map's key assignment depend on the lookahead window. Verified end-to-end: testSkipDoesNotCrossSymbolConversion fails pre-fix with 'symbol counts differ expected:<4> but was:<3>' and passes post-fix.

Non-blocking follow-ups:

• testSkipDoesNotCrossTruncate is an always-green guard (passes pre- and post-fix) because no structural txn sits between the data run and the TRUNCATE, so old Math.min and new futureSeqTxn-initialSeqTxn compute the same value. Consider inserting a structural txn (e.g. ALTER ... TYPE SYMBOL) before the TRUNCATE to make it a genuine regression test, or re-document it as a soft-truncate invariant guard.
• No coverage for the continue->break change on MAT_VIEW_INVALIDATE / VIEW_DEFINITION.
• Two minor comment imprecisions: the walId>0 non-data branch comment lists 'structural metadata change' (those are walId=-1, handled in the else branch); the 'walId <= 0' comment omits DROP_TABLE_WAL_ID=-2.

[bluestreak01]

/azp run macwin

[azure-pipelines]

Azure Pipelines successfully started running 1 pipeline(s).

[mtopolnik]

[PR Coverage check]

😍 pass : 2 / 2 (100.00%)

file detail

	path	covered line	new line	coverage
🔵	io/questdb/cairo/wal/ApplyWal2TableJob.java	2	2	100.00%