[OPIK-7315] harden the traces-cutover drivers: UTC literals, client timeouts, and an undecidable-compare verdict - #8081
Conversation
…imeouts, and an undecidable-compare verdict
Three defects in the traces-local-v2 cutover tooling. Each fails quietly: the run
completes, the driver exits 0, and what is wrong is either invisible or attributed to
the data.
1. TIMEZONES. A DateTime64 literal with no timezone is parsed in the SERVER timezone,
while every column it is compared against is DateTime64(n, 'UTC'). Twenty literals
across the reference SQL were unpinned; three already were, so the inconsistency
also hid the intended convention.
The epoch sentinel is the clearest case. Unpinned it writes the instant
"1970-01-01 00:00:00 local" rather than epoch 0, and nothing fails at write time:
the rollback's sentinel repair matches epoch exactly, so it matches nothing and
reports a clean table, while the fidelity compare normalizes an absent end_time to
0 and instead flags every migrated row that had one.
Captured bounds are pairs, and pinning one half is worse than pinning neither.
backfill.sh minted backfill_start with now64(6) -- server-local -- and 000002 read
it back server-local, which agreed. Pinning only the literal reinterprets a local
wall clock as UTC and moves the anchor by the server's offset; a LATER anchor drops
the rows written in the gap, and the delta and the deletion replay share that bound,
so neither sees them. Both halves now pin UTC:
backfill.sh now64(6, 'UTC') -> 000002 ${BACKFILL_START}
exchange_and_wrap.sh now64(6, 'UTC') -> 000004 ${CUTOVER_START}
Recorded at the capture and in the runbook: a backfill_start persisted in
--state-file is only reusable by the revision that wrote it.
000005's window bounds are pinned for a second reason beyond consistency: they are
derived from a UTC calendar date, and 000001 copies the same week under bounds that
were already pinned. Unpinned, both sides of the compare shift together --
self-consistent, so no mismatch is reported, while the first and last windows quietly
stop covering what the backfill copied.
No behaviour change on a UTC server. The value is that the statements mean what they
say on any server.
2. CLIENT TIMEOUTS. ClickHouse's receive_timeout is 300s and bounds the gap between
packets rather than total query time, so a step that goes quiet while the server
works is abandoned while healthy. verify.sh already raised it; the other five drivers
did not, and they carry the longest-running statements in the cutover -- the
backfill's per-window INSERT, the delta and deletion replay, the EXCHANGE and its
final replay, and finalize's DROP of a large table.
Each now takes --receive-timeout (default 1800, matching verify.sh) and routes every
invocation through one CH_ARGS array, so host, port, database and the timeout cannot
drift between a driver's call sites. Five sites had built the connection by hand and
three omitted log_comment, so their statements reached query_log unattributable while
every other step's was tagged.
exchange_and_wrap.sh keeps log_comment out of CH_ARGS deliberately: its final
deletion replay is tagged separately so it can be found on its own, and
clickhouse-client rejects a setting passed twice, so a default here could not be
overridden there. rollback.sh omits it for a different reason -- its .sql files all
carry their own tag. The two command lines exchange_and_wrap.sh prints for an operator
to run by hand still expand the connection inline: they are copy-paste text.
3. AN UNDECIDABLE COMPARE NOW SAYS SO. verify.sh re-checks a differing window on the
sorting key and reports "OK -- superseded-version artifact" when no key genuinely
differs. That is sound only where each key has a unique newest last_updated_at: under
a tie FINAL picks arbitrarily per side, so the two sides can agree by luck --
including where one side is MISSING a version, which is a real copy gap reading as a
pass.
A new version-ties block counts, per side, the keys in the window whose newest
version is shared by more than one row. verify.sh runs it where the answer decides
the verdict -- when the re-check returns 0 -- and reports the window INCONCLUSIVE,
exiting non-zero, when either count is non-zero. A fidelity gate that cannot decide
must not pass, which is the rule this script already applied to an empty compare
range.
It is a separate statement, and window-scoped rather than scoped to the differing
keys, for one reason: ClickHouse inlines rather than materializes the CTEs the
re-check builds its answer from, so an aggregate referencing them re-runs its FULL
OUTER JOIN per reference. Keeping it independent also means it is only paid for when
the question arises. The counts are therefore an upper bound, which errs toward
refusing to certify.
Also: --drill-down fired only on the MISMATCH branch, so the artifact verdict -- the
one the runbook tells operators to distrust -- printed no keys at all. It now fires on
any differing window, while the "re-run with --drill-down" hint stays on the verdicts
that need action. And assert_single_shard proceeded with a note when the shard count
was unreadable, defeating its own purpose: it exists to stop a whole-table rewrite
whose postcondition cannot be satisfied, and an unverifiable topology is exactly that
run. Proceeding also bought nothing, because the postcondition reads
clusterAllReplicas('{cluster}', ...) and needs the same system.macros the count needs.
Now fatal, with --confirm-single-shard where those reads are genuinely unavailable; it
does not override a count greater than one, and is rejected outside
--sentinel-repair-only, the only mode that asserts it.
TESTS, in this suite's pattern -- statements reimplemented inline, no shipped file read:
- backfillIsUnaffectedByTheSessionTimezone runs the copy under a westward session and
pins both of the backfill's literals: the row is seeded an hour into the week, so an
unpinned bound starts the window after it and the copy skips it; and the sentinel is
read back as microseconds, so the assertion does not depend on the parsing it pins.
- deletionReplayWindowIsUnaffectedByTheSessionTimezone covers the highest-consequence
literal in the runbook. A lightweight DELETE does not bump the version column, so the
delta cannot see it and the replay is the only thing stopping the deletion leaking
across the swap; a bound resolved westward lands after the recorded event and the
replay matches nothing. Asserted on the bound's selection, because session_timezone
does not reach a mutation's literal parsing. The reverse replay and its postcondition
carry the same bound against cutover_start.
- versionTiesDoesNotCountAKeyWhoseNewestVersionIsUnique pins that the tie aggregate
ranks by version rather than totalling rows, using a key written twice with distinct
versions -- unique whether or not a merge has collapsed it, so the assertion does not
depend on timing. The opposite case is not constructed: holding two rows with an
identical version in a ReplacingMergeTree means racing a merge. That boundary is
stated in the test, and the absence of FINAL is argued in the version-ties block.
- The suite's inline mirrors pin 'UTC' wherever the shipped SQL does, and nowMicros()
captures in UTC like the drivers it stands in for.
Not covered, deliberately: the drivers' own guards (--receive-timeout plumbing,
verify.sh's verdict branching, assert_single_shard) are shell, which this suite does not
reach; its scope statement assigns them to the staging rehearsal.
Verified: 17/17 green; every new assertion mutation-checked, each failing when its
invariant is reverted; all four rendered blocks executed against seeded tables; bash -n
clean across all seven drivers.
github-actions
Bot
added
documentation
⏱️ pre-commit per-hook timing
⏭️ 43 skipped (no matching files changed)
|
|
Already covered by a test in this PR. No e2e test proposed, and none is owed. The half with data-fidelity consequence — the UTC-pinned DateTime64 literals in 000001/000002/000005 and the new Also considered. The shell-driver hardening (verify.sh's four-verdict compare and its non-zero exit on INCONCLUSIVE/UNCERTIFIABLE, backfill.sh's required ' UTC' anchor marker, the CH_ARGS/--receive-timeout consolidation across all six drivers) has no automated coverage — the Java gate test asserts SQL, not bash. It is not a coverage gap for QA: these are one-off operator scripts run by hand against a production ClickHouse, outside anything the e2e estate can drive. also touches Backend (Java API / internal) Advisory, from the QA test radar. Nothing here blocks this PR, and anything it proposes is a draft for review. Re-checked after a push on 01 Sep 09:05 UTC — nothing the verdict depends on changed. |
![baz-reviewer[bot]](https://avatars.githubusercontent.com/in/933528?s=60&v=4){kind=small}
… enforce the anchor's timezone Two correctness fixes from review, plus coverage for a branch integration cannot reach. version-ties scoped its version scan by the created_at window while confirm-keys picks candidate keys from the window and then reads each key's versions with NO window at all. So the tie that can mislead the re-check is any tie among a candidate key's versions, wherever they landed -- and a key re-sent far enough apart puts equal-version rows in different weeks, where a window-scoped scan sees one row per version, reports no tie, and certifies a verdict FINAL had drawn arbitrarily. Candidates now come from the window and the sample; the version scan does not. Verified on a straddling fixture: the window-scoped form returns 0 where the corrected form returns 1. The counts remain an upper bound on ties among the DIFFERING keys, the candidates being every key in the window rather than only those that differed. That direction refuses to certify a decidable window and never the reverse. backfill_start is now stored with an explicit ' UTC' marker and only accepted with it. This change made the SQL parse that anchor as UTC, which silently reinterprets one captured server-local by an earlier revision: east of UTC the anchor moves LATER, and the delta and the deletion replay share that bound, so both miss the rows written in the gap. A bare timestamp cannot be attributed to a timezone, so it is refused with the two ways out rather than reinterpreted. Both drivers also print their anchors labelled UTC, so a value pasted into --backfill-start or --cutover-start says which zone it is in. versionTieAggregateCountsOnlyASharedNewestVersion covers the tie aggregate's positive branch over a literal relation. That branch cannot be reached against these tables: holding two rows at one version needs a ReplacingMergeTree not to merge them, and traces is unpartitioned while the successor's partition key is MATERIALIZED from the row's own id, so two rows for a key always share a partition. Three keys separate ranking from totalling -- one tied at its newest version, one with more rows but a unique newest, one single -- so sum or a plain max instead of argMax fails. Both mutations checked. 18/18 green; bash -n clean across all seven drivers.
Backend Tests - Integration Group 14 43 files 43 suites 3m 4s ⏱️ Results for commit 2293b83. ♻️ This comment has been updated with latest results. |
…he recovery command it broke Two follow-ups from review, both consequences of enforcing the marker on the state file alone. The recovery command backfill.sh prints when the anchor is missing but the destination already holds rows told the operator to write a bare timestamp, which the new resume validation then refuses -- so following the driver's own instruction produced an abort. It now writes the marker. The marker was also enforced on the state file only, leaving --backfill-start and --cutover-start accepting a bare timestamp: the guard was bypassable by supplying the anchor by hand, which is the same path an operator takes when pasting from an older run's log. All three anchor arguments now require it -- delta_replay.sh, exchange_and_wrap.sh and rollback.sh -- and the value is stripped before use. The command rollback.sh prints for a follow-up reverse replay carries the marker too, so it stays runnable, and the runbook's six examples are updated. Verified: a bare anchor exits 2 on all three drivers with the marker error; a marked one passes validation and proceeds to the connection. 18/18 green; bash -n clean across all seven drivers.
The comment read "carries an explicit ' UTC' marker and is required to", an ellipsis that scans as a truncated sentence and says the same thing twice. It also pointed at "the SQL below", which is loose in rollback.sh, where the statements live in .sql files. Rewritten to carry only what the option doc in the same file does not: that the marker is stripped here, and that for these bounds a wrong zone is worse than a wrong shape -- a LATER value drops rows from the delta and the replay rather than failing. Guard re-verified after the edit: bare anchors still exit 2 on all three drivers, a marked one still passes validation. 18/18 green.
thiagohora
left a comment
thiagohora
left a comment
There was a problem hiding this comment.
🧪 PILOT — experimental code-review assistant (not a required gate).
Grounded in the team's own merged-PR review history; each finding was empirically
verified before posting. We're calibrating it — please 👍/👎 each comment so it learns.
Opik reviewer (mined from your team's review history)
32 findings — 1 high · 20 medium · 11 low. Suppressed by team conventions: see suppressed.md.
React 👍/👎 on each comment — your feedback helps tune what it flags.
…f them The version-ties gate would have failed healthy runs. It counted physical rows at the newest last_updated_at, and the cutover produces exactly that state on the successor by construction: 000002's delta re-copies every row written during the backfill window, and an unmodified row keeps its last_updated_at, so several identical rows sit at one version until a merge collapses them. verify.sh runs before the EXCHANGE, on the recent partitions where they are least likely to have merged, so dst_version_ties came back non-zero on a faithful copy, the window was reported INCONCLUSIVE and the run exited 1. The premise was wrong: FINAL picking either of two byte-identical rows changes no verdict. A tie is now two or more DISTINCT row contents at the newest version, measured with the same normalization `compare` uses, so schema and precision differences do not count as differences and identical re-copies collapse to one. Two limits are now stated rather than implied: - The read is window-scoped, so a tie whose differing rows sit in different created_at weeks is not detected. Following confirm-keys exactly would mean reading every version of every candidate key with no time predicate, per artifact window, and artifact windows are the common outcome; reusing its diff_keys CTE instead does not plan. - The guarantee covers artifact windows only. The direction this block calls dangerous can also produce ok=1 directly, and such a window never reaches the check, so a plain PASSED does not carry a tie guarantee. Tests, and one of them was vacuous before this commit: - theRunbooksBackfillThenDeltaSequenceIsNotATie runs the backfill and then the delta over the same anchor and asserts no tie. Its first version passed for the wrong reason: the anchor was taken after the seed, so the delta selected nothing. The anchor now precedes the row and the fixture asserts two raw physical rows before asserting no tie, so the premise cannot silently disappear. Mutation-checked: reverting to a row count fails it 1 vs 0. - versionTiesDoesNotCountAKeyWhoseNewestVersionIsUnique asserted only isZero(), which an empty fixture satisfies. It now asserts the fixture landed on both tables first. - The aggregate lives in one VERSION_TIE_AGGREGATE constant, parameterised by its source relation, so the helper and the literal-relation test exercise the same expression instead of separate transcriptions. The helper takes the schema shape and reuses rowHash, the suite's single normalization, rather than a third copy.
…lay, and the binding timeout - backfill.sh read the state file only when not --dry-run, so a rehearsal passed clean while the real run aborted on the same file. The read and both guards are hoisted out; only minting and persisting stay behind DRY_RUN. That asymmetry is one this script already rejects twice, for --max-partitions-per-insert-block and the --mit assignment. - The resume path logged the anchor stripped of its marker. That is the only place a resumed run shows it, and it is the value step 2 and step 3 refuse without one. - Recovery commands printed without --host/--port target localhost, on a driver whose own header explains prod access goes through a port-forward. Both messages now propagate them, and the marker-less branch offers the fresh-mint route when the shadow is empty rather than sending the operator through a destructive-sounding no-op. - assert_single_shard treated 0 as a verified single shard. A missing 'cluster' macro makes the WHERE match nothing and uniqExact return 0, which is the unknown-topology state the guard exists for, and it also guarantees the postcondition cannot run. - The reverse replay carries the same scope mismatch as the repair — not ON CLUSTER, with a postcondition reading clusterAllReplicas — and had no shard assertion at all. Stages B and C and --reverse-replay-only now assert it too, and --confirm-single-shard is accepted in those modes. There the failure is worse than a confusing verdict: the rows left unmasked on other shards are user-deleted traces the rollback resurrects. - An anchor of just ' UTC' stripped to empty and was then reported as not supplied. - finalize.sh raises distributed_ddl_task_timeout with the client timeout. Its only long statements are ON CLUSTER DROP/TRUNCATE, whose wait is capped server-side, so raising the client side alone left a multi-TB DROP failing at the cap. - delta_replay.sh dropped its CH_ARGS log_comment: both statements in 000002 set their own in SETTINGS, which overrides the session value, so the tag never reached query_log. - verify.sh no longer encodes an unreadable tie count as a tie. It is its own UNCERTIFIABLE outcome, counted separately and in the exit condition, so nobody triages a tie that does not exist. And the drill-down is non-fatal: it runs on passing artifact windows now, so an unguarded failure under set -e would abort a run that was succeeding. - The ../../README.md pointer added in five drivers does not exist; the runbook is one level up.
…r-marker sweep The cutover pinned every literal it writes but not the ones the destination reads back. traces_local_v2 and spans_local_v2 declare end_time as DateTime64(6, 'UTC') while their DEFAULT and their duration MATERIALIZED expression parse a bare epoch literal in the SERVER's timezone, so a row whose end_time genuinely is epoch stops matching the "not ended yet" branch and gets a large duration where NaN is meant. 000119 pins both columns on both tables; they are empty, so it is metadata-only. The traces/spans source tables carry the same literal and are deliberately left alone: changing a MATERIALIZED expression does not recompute existing parts, so there it would create two sentinels in one table. The operator-facing text still told people to capture or pass anchors without the ' UTC' marker the drivers now require, in the runbook's smoke test and retry path, the reference SQL's own invocation example, and the load-test walkthrough. verify.sh's INCONCLUSIVE message still described a tie as several rows rather than several distinct rows, and --confirm-single-shard was documented as an escape hatch without saying the repair then cannot be certified. Tests: a westward-session test for the delta's window, the third of the three literal families and the only one an INSERT SELECT can show end to end; the duration sentinel asserted on the backfill's copy, which fails if the shipped DDL is unpinned; the bridge-window predicate extracted so the deletion tests exercise the replay's own predicate rather than a fresh copy; and the DDL mirrors in the benchmark tests pinned with it. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Backend Tests - Unit Tests1 927 tests 1 926 ✅ 54s ⏱️ Results for commit 00b7f14. ♻️ This comment has been updated with latest results. |
Backend Tests - Integration Group 4187 tests 187 ✅ 7m 24s ⏱️ Results for commit 00b7f14. ♻️ This comment has been updated with latest results. |
…ier rounds left stale Migration 000119 pinned 'UTC' on the destination tables' epoch sentinel. Out of scope here, and pulling it takes the driver-side half with it: the epoch sentinel is unpinned in the destination DDL, in the app's end_time comparisons, and everywhere else the value is read back, so pinning it in the backfill and delta alone would give migrated rows a sentinel no other reader matches. Both revert to the codebase's form, and 000001's header records why that literal is the deliberate exception and what a real fix has to move together. The runbook's Timezones section is retitled to match what the change actually guarantees: every window BOUND pins 'UTC'. Prose corrections, all of them claims that had drifted from the code: - verify.sh announced three verdicts and listed four, and described a version tie as several rows at one version in two places, which is what the tie block stopped counting when it moved to distinct content. - The runbook's tie note still said the gate counts keys carrying two or more rows with an identical last_updated_at. That is every key the delta re-copied, so as written the gate would call every healthy window undecidable. It now states the distinct-content rule and why row count is unusable before the EXCHANGE. - rollback.sh's --confirm-single-shard doc said it was accepted only with --sentinel-repair-only, contradicting both its own validation and the runbook; a second comment introduced four modes as "the two". - Four javadocs sat above the wrong member, having been separated from it by later insertions, and one documented a helper that no longer exists. - finalize.sh carried the same ON CLUSTER rationale twice; backfill.sh narrated which other flags share a guard. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…rd stages B/C before they promote
The version-ties block carried a fragment of confirm-keys spliced into its
dst_ties CTE -- `AS src_hash`, `FROM ${OLD_TABLE} FINAL`, `) AS s`,
`FULL OUTER JOIN (` -- and wrote `%%` where the other blocks write `%`. The
rendered statement was invalid, so verify.sh could never reach a verdict on an
artifact window: the gate the last round added has never run. dst_ties is now an
independent destination aggregate, with both hash shapes taken from confirm-keys
in the same file, the destination's own precision on the window bounds, and no
FINAL. Every block in 000005 and every other reference SQL file was rendered and
parsed against ClickHouse 26.3 to confirm it; the pre-fix block fails the same
check, and a deliberately broken control confirms the check is not vacuous.
Nothing in the repository could have caught this. The tests duplicate the
queries rather than reading the reference SQL, by design, and verify.sh's tie
path only runs on a window the compare reports as differing.
rollback.sh ran the stage B/C promote before assert_single_shard. On an unknown
or multi-shard topology that promoted the original, parked the successor, and
then failed the guard, leaving the post-cutover deletes never re-applied --
user-deleted traces resurrected -- and the stage no longer re-runnable. The
guard now runs before either promote.
Also: the tie block's SETTINGS comment described max_rows_to_read = 0 as a
backstop that fails loudly, when it removes a limit, and treated
max_bytes_before_external_group_by as a cap when it only spills.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
… state file in both modes Reverting the epoch-sentinel pin in 000001/000002 left the test's COPIED_SELECT still pinning it, so the mirror disagreed with the projection it exists to reproduce. Realigned; no assertion depended on the pin. backfill.sh gated the whole state-file read on -s, so an existing zero-byte file read as "no anchor". That state is reachable rather than theoretical: persisting the anchor is a truncating redirect, so a run killed between opening the file and writing to it leaves exactly this. It also put the two modes out of step on identical input, which is what hoisting the read out of the DRY_RUN guard was meant to end: a real run fell through to the mint branch and its destination-not-empty guard, while a dry run reached neither and planned against an anchor it never had. The gate is now -e with an explicit empty check, and the message gives the same two ways out as the marker branch. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
thiagohora
left a comment
thiagohora
left a comment
There was a problem hiding this comment.
🧪 PILOT — experimental code-review assistant (not a required gate).
Second pass, against the fixes pushed since the first review.
Opik reviewer — round 2 (head 5356476f)
5 findings on the new work — 2 medium · 3 low. The 31 findings fixed since round 1 all verify as resolved; nothing regressed there.
The one worth attention is exchange_and_wrap.sh: the ON CLUSTER timeout fix landed in finalize.sh but not here, and every statement this driver runs through run_block is ON CLUSTER DDL.
7 further findings (test-helper duplication, Javadoc drift, a runbook gap for the new UNCERTIFIABLE verdict) were confirmed but held back as nits. One finding was dropped as out of scope (the tiered-headroom abort predates this PR, from #7572).
React 👍/👎 on each comment — your feedback helps tune what it flags.
| echo "RECORD cutover_start=$CUTOVER_START UTC (pass the timestamp to rollback.sh --cutover-start if you roll back after this point)" | ||
|
|
||
| echo "Final deletion replay: masking deletes bridged since the last delta_replay so none leak across the swap..." | ||
| run_final_deletion_replay |
There was a problem hiding this comment.
[medium] Final deletion replay silently no-ops if 000002's block markers move
run_final_deletion_replay() extracts the -- >>> BEGIN deletion-replay .. -- >>> END deletion-replay block out of 000002_delta_and_deletion_replay.sql with a bare awk exact-line match (line 346) and hands the result straight to clickhouse-client (line 351). There is no post-condition on the extraction, and no guard that a placeholder was actually substituted -- unlike backfill.sh (mit_require_one_assignment + the 'placeholder survives' check at 443-447) and delta_replay.sh (197-217), which were hardened for exactly this class of failure against the same two SQL files. 000002 is a file this cluster actively edits (its header, its SETTINGS block and its replay predicates all changed here), so a marker rename, an extra blank/indented character on a marker line, or the block being split leaves sql empty. An empty --query is not an error the way a malformed statement is: the step prints its banner, produces no diagnostic, and the script proceeds to run_block exchange. The consequence is precisely what the step exists to prevent -- deletes bridged between delta_replay.sh and cutover_start are never masked on the successor, and after the EXCHANGE they are live again on traces, covered by neither the forward replay nor the rollback reverse-replay (which starts at cutover_start). verify.sh will not catch it either: it runs before this replay.
💡 After the awk extraction, refuse to continue on an empty or unsubstituted result, e.g. [[ -n "$sql" ]] || { echo "ERROR: deletion-replay block not found in $DELTA_SQL_FILE" >&2; exit 2; } plus a grep -qF '${' <<<"$sql" post-condition mirroring delta_replay.sh's. Apply the same guard to run_block() so an empty exchange or wrap block cannot print 'EXCHANGE done' having swapped nothing.
general finding
| CH_ARGS=() | ||
| [[ -z "$CH_HOST" ]] || CH_ARGS+=(--host "$CH_HOST") | ||
| [[ -z "$CH_PORT" ]] || CH_ARGS+=(--port "$CH_PORT") | ||
| CH_ARGS+=(--database "$DATABASE" --receive_timeout="$RECEIVE_TIMEOUT") |
There was a problem hiding this comment.
[medium] --receive-timeout does not bound the EXCHANGE/wrap ON CLUSTER DDL
The new --receive-timeout is wired into CH_ARGS as --receive_timeout only. finalize.sh, added in the same change, documents why that is not enough for ON CLUSTER DDL and maps the same flag onto distributed_ddl_task_timeout as well (finalize.sh:79-85: the ON CLUSTER wait 'is capped server-side at 180s by default with distributed_ddl_output_mode = throw', and a statement that outlives the cap 'raises TIMEOUT_EXCEEDED however high the client timeout is'). Everything exchange_and_wrap.sh executes through run_block is ON CLUSTER DDL (000003: EXCHANGE TABLES ... ON CLUSTER then RENAME ... ON CLUSTER in one --multiquery call; the wrap's CREATE ... Distributed then a multi-target RENAME), so raising --receive-timeout on this driver buys nothing on the one statement pair where a timeout is most expensive: a TIMEOUT_EXCEEDED between the two exchange statements leaves exactly the split state the script has a bespoke diagnostic for (assert_pre_exchange_topology, 236-239 -- 'the EXCHANGE already ran ... but traces_local_v2 still exists'), while the DDL keeps running in the background. README:856 ('Every driver takes --receive-timeout ... the default is raised across the board') states the coverage as uniform, which it is not.
💡 Add --distributed_ddl_task_timeout="$RECEIVE_TIMEOUT" to CH_ARGS here (and in rollback.sh, whose stage B/C promotes are also ON CLUSTER renames), matching finalize.sh, and note in the option doc that for this driver it is the binding limit.
general finding
| # --time prints the statement's elapsed seconds to stderr (a bare --query prints nothing). This replay sits inside | ||
| # the final-delta -> EXCHANGE gap the buffer hold has to cover, so its wall time is the number to record. | ||
| clickhouse-client ${CH_HOST:+--host $CH_HOST} ${CH_PORT:+--port $CH_PORT} --database "$DATABASE" --log_comment 'traces_local_v2_cutover:exchange_and_wrap:final_deletion_replay' --time --multiquery --query "$sql" | ||
| clickhouse-client "${CH_ARGS[@]}" --log_comment 'traces_local_v2_cutover:exchange_and_wrap:final_deletion_replay' --time --multiquery --query "$sql" |
There was a problem hiding this comment.
[low] final_deletion_replay log_comment tag is overridden by 000002's own SETTINGS
The new CH_ARGS comment (126-128) justifies keeping log_comment out of the shared args on the grounds that 'the final deletion replay is tagged separately so it can be found on its own in query_log', and run_final_deletion_replay passes --log_comment 'traces_local_v2_cutover:exchange_and_wrap:final_deletion_replay'. But the statement it runs is the deletion-replay block lifted verbatim from 000002, which ends with SETTINGS ... log_comment = 'traces_local_v2_cutover:deletion_replay' (000002:185). A query-level SETTINGS value wins over the client-supplied one -- the rule delta_replay.sh states as its own reason for omitting the flag ('A per-query value overrides the session one, so a tag added here would never reach query_log', delta_replay.sh:90-91). So the final pre-EXCHANGE replay lands in query_log under the same tag as delta_replay.sh's run and cannot be isolated, which is the one measurement (its wall time inside the final-delta -> EXCHANGE gap) the runbook asks the operator to record. Note run_block's added tag is harmless by contrast, because 000003 sets log_comment via a leading SET, not a trailing SETTINGS.
💡 Either drop the misleading justification and the ineffective flag, or parameterise the block's log_comment (e.g. substitute a ${LOG_COMMENT} placeholder in 000002's SETTINGS the way the other placeholders are substituted) so the final replay really is distinguishable in query_log.
general finding
| echo "RECORD cutover_start=$CUTOVER_START (pass to rollback.sh --cutover-start if you roll back after this point)" | ||
| # Captured in UTC; the reverse replay and its postcondition parse it as UTC (000004_rollback_reverse_replay.sql). | ||
| CUTOVER_START="$(clickhouse-client "${CH_ARGS[@]}" --log_comment 'traces_local_v2_cutover:exchange_and_wrap' --query "SELECT toString(now64(6, 'UTC'))")" | ||
| echo "RECORD cutover_start=$CUTOVER_START UTC (pass the timestamp to rollback.sh --cutover-start if you roll back after this point)" |
There was a problem hiding this comment.
[low] cutover_start RECORD line tells the operator to drop the marker rollback.sh requires
The RECORD line now prints cutover_start=<ts> UTC and instructs 'pass the timestamp to rollback.sh --cutover-start'. rollback.sh rejects a value without the ' UTC' marker (rollback.sh:175-189, 'must carry an explicit UTC marker, as the drivers print it'), and README:884-885 states that both anchor flags require it. The sibling producer gets this right -- backfill.sh:622 says 'pass this, marker included, to step 2'. As written the instruction directs the operator to strip exactly the token the consumer mandates, so the first rollback attempt (stage B/C, under time pressure, after an irreversible EXCHANGE) fails argument validation.
💡 Match backfill.sh's wording: 'pass this, marker included, to rollback.sh --cutover-start', or show the full quoted form --cutover-start '<ts> UTC'.
general finding
| -- Neither setting is a query-level cap. max_rows_to_read = 0 removes any row limit a settings profile imposes: this | ||
| -- read is not truncatable -- it either covers the window's physical versions or throws -- and a throw would fail a gate | ||
| -- that could otherwise answer. max_bytes_before_external_group_by lets the GROUP BY spill to disk rather than hit the | ||
| -- memory limit. What actually bounds the read is the window predicate above, which prunes partitions. |
There was a problem hiding this comment.
[low] version-ties lifts max_rows_to_read on a partition-pruning claim neither table supports
The new block disables any profile row limit (max_rows_to_read = 0) and justifies it with 'What actually bounds the read is the window predicate above, which prunes partitions.' Neither side prunes partitions on created_at. The source traces is unpartitioned (as TracesLocalV2CutoverTest's own version-tie Javadoc states at line 1571: 'traces is unpartitioned'), and the successor is PARTITION BY toYYYYMMDD(toDate32(id_at) - ...) (migration 000114) -- id_at derived, not created_at. A created_at window can only prune granules via the idx_traces_created_at minmax skip index, which is also why the sibling blocks in this same file have to pass use_skip_indexes_if_final = 1. The block runs without FINAL over every physical version in the window, is reached on the common outcome (an artifact window) rather than the rare one, and the cost model the comment offers to the operator understates what removing the row cap authorises on a busy week.
💡 Correct the comment to say the predicate prunes granules through the created_at minmax skip index (partitions on the successor are id_at-derived, and the source is unpartitioned), and state the resulting per-window cost so the max_rows_to_read override is an informed trade rather than one justified by pruning that does not happen.
general finding
3 participants
Details
Three defects in the traces-local-v2 cutover tooling. Each fails quietly: the run completes, the driver exits 0, and what is wrong is either invisible or attributed to the data.
1. Timezones. A
DateTime64literal with no timezone is parsed in the server timezone, while every column it is compared against isDateTime64(n, 'UTC'). Twenty literals across the reference SQL were unpinned; three already were, so the inconsistency also hid the intended convention.The epoch sentinel is the clearest case: unpinned it writes
1970-01-01 00:00:00 localrather than epoch 0, and nothing fails at write time — the rollback's sentinel repair matches epoch exactly, so it matches nothing and reports a clean table.Captured bounds are pairs, and pinning one half is worse than pinning neither.
backfill.shmintedbackfill_startwithnow64(6)(server-local) and000002read it back server-local, which agreed; pinning only the literal moves the anchor by the server's offset, and a later anchor drops the rows written in the gap from both the delta and the deletion replay, which share that bound. Both halves now pin UTC.2. Client timeouts. ClickHouse's
receive_timeoutis 300s and bounds the gap between packets, not total query time, so a step that goes quiet while the server works is abandoned while healthy.verify.shalready raised it; the other five drivers did not, and they carry the cutover's longest-running statements. Each now takes--receive-timeout(default 1800) applied through oneCH_ARGSarray, so a driver's call sites cannot drift — five had built the connection by hand and three omittedlog_comment.3. An undecidable compare now says so.
verify.shreportsOK — superseded-version artifactwhen a re-check finds no key genuinely differing. That is sound only where each key has a unique newestlast_updated_at: under a tieFINALpicks arbitrarily per side, so the sides can agree by luck — including where one side is missing a version, which is a real copy gap reading as a pass. A newversion-tiesblock counts those keys per side;verify.shruns it where the answer decides the verdict and reports the window INCONCLUSIVE, exiting non-zero. A fidelity gate that cannot decide must not pass.Also in 3:
--drill-downfired only on theMISMATCHbranch, so the artifact verdict — the one the runbook tells operators to distrust — printed no keys at all. Andassert_single_shardproceeded with a note when the shard count was unreadable, defeating its own purpose; now fatal, with--confirm-single-shardwhere those reads are genuinely unavailable.Full reasoning, including why
version-tiesis a separate window-scoped statement, is in the commit message and the runbook.Change checklist
Issues
AI-WATERMARK
AI-WATERMARK: yes
Testing
TracesLocalV2CutoverTest— 17/17 green (mvn test -Dtest=TracesLocalV2CutoverTest). Three tests added, in this suite's pattern: statements reimplemented inline, no shipped file read.backfillIsUnaffectedByTheSessionTimezone— runs the copy under a westward session and pins both of the backfill's literals. The row is seeded an hour into the week, so an unpinned bound starts the window after it and the copy skips it; the sentinel is read back as microseconds, so the assertion does not depend on the parsing it pins.deletionReplayWindowIsUnaffectedByTheSessionTimezone— the highest-consequence literal in the runbook. A lightweightDELETEdoes not bump the version column, so the delta cannot see it and the replay is the only thing stopping the deletion leaking across the swap. Asserted on the bound's selection, becausesession_timezonedoes not reach a mutation's literal parsing.versionTiesDoesNotCountAKeyWhoseNewestVersionIsUnique— pins that the tie aggregate ranks by version rather than totalling rows, using a key written twice with distinct versions, which is unique whether or not a merge has collapsed it.Every new assertion was mutation-checked: reverting each invariant fails its test (e.g. an unpinned epoch under a westward session yields
18000000000µsinstead of0; the replay bound yields1 → 0). All four rendered blocks of000005were executed against seeded tables.bash -nclean across all seven drivers.Not covered, deliberately. The drivers' own guards — the
--receive-timeoutplumbing,verify.sh's verdict branching,assert_single_shard— are shell, which this suite does not reach; its scope statement assigns them to the staging rehearsal. Twoversion-tiesproperties are also uncovered by design: a tie being counted would need two rows with an identical version held in aReplacingMergeTree, which races a background merge, and the absence ofFINALhas no fixture that can pin it. Both boundaries are stated in the test.Documentation
Runbook updated alongside the code: a new Timezones section (the convention and why both halves of a captured bound must agree), the shared
--receive-timeoutrationale, theINCONCLUSIVEverdict and where--drill-downnow fires, and--confirm-single-shard. The version-tie triage previously said the re-check "cannot resolve" a tie and left detection manual; detection is automatic now, and only resolving one is manual.