Repository navigation
test(storage): start the ART reproducer from the real pre-v10 schema - #150
Merged
Merged
Conversation
The reproducer's fixture described itself as `spans` at schema version 2 but was missing that era's `duration_ms` - a VIRTUAL generated column in the middle of the table, which only v9 -> v10 removed. Two things follow from its absence, and both weaken the negative result the recovery page now rests a release condition on. `schema.sql`'s v10 statement is `ALTER TABLE spans DROP COLUMN IF EXISTS duration_ms`, so over a table that never had the column it is a silent no-op: the only ALTER the runs exercised under a filled, indexed table was v2 -> v3, which in production ran the day after the database was created. v10 is the one that ran on a long-lived table six weeks before the open aborted. And without the column the fixture carries none of the logical/physical column skew of ADR-0013, so the indexes were built over a layout production never had. The column goes back in verbatim, and the pass now asserts the migration actually ran: `duration_ms` present before `storage.Open`, gone after. A no-op cannot return unnoticed in either direction. The probe pair is renamed to what it measures. No SELECT over `spans` is planned as an ART index scan on the linked engine - measured for a bare constant, a bound parameter, an equality on the primary key, and the textbook case of a unique match in a 100 000-row table with a fresh index on the probed column, with the index_scan thresholds raised to admit everything and with and without the storage_compatibility_version pin - so "an indexed probe agrees with an unindexed one" was two sequential scans agreeing. What the pair does compare is filter pushdown against no pushdown, which is where ADR-0013's wrong answers came from, and the test now asserts the two probes still plan differently so the comparison cannot degenerate into one path checked twice. Unique- constraint enforcement is the one path that provably traverses an ART index, so each restart also re-inserts a stored span_id (must be rejected) and inserts an unused one (must be accepted). requireNoWAL now tolerates a present-but-empty WAL file: an engine that truncates instead of unlinking has folded everything, and failing on the inode would report the next such engine as a reproduction of this defect on the very assertion that guards the fold. Re-run on the corrected fixture, linux/arm64 under golang:1.24 against duckdb-go/v2 v2.10506.0 (v1.5.6): did not reproduce at 20 000 spans / 2 cycles (4.1 s) or at 200 000 spans / 6 cycles (2 m 58 s, 8 opens, 14 checkpoints, the table peaking at 407 956 rows). Both rows are added to the recovery page's run table beside the earlier two rather than replacing them, with what each fixture did and did not exercise. Co-Authored-By: Wayland <wayland@agents.flopbut.local> Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
Warning Review limit reachedYou've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Next included review available in 2 minutes. View limit details
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. Comment |
The defaults line still carried 4.7 s, measured on the fixture that had no duration_ms; the run table two sections down in the recovery page reports 4.1 s for the corrected fixture, so the release notes contradicted it. Co-Authored-By: Daedalus <daedalus@agents.flopbut.local> Co-Authored-By: Claude Opus 5 <noreply@anthropic.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.
What
The ART reproducer's fixture claimed to be
spansat schema version 2 but was missing that era'sduration_ms— the VIRTUAL generated column mid-table that only v9 → v10 removed. The column goes back in verbatim fromddb4a2b^:internal/storage/schema.sql.Why it mattered:
schema.sql's v10 statement isALTER TABLE spans DROP COLUMN IF EXISTS duration_ms, so over a table that never had the column it is a silent no-op. The only ALTER the previous runs exercised under a filled, indexed table was v2 → v3 — which in production ran the day after the database was created. v10 is the one that ran on a long-lived, filled, four-index table six weeks before the open aborted. And without the column the fixture carried none of the logical/physical column skew of ADR-0013, so the indexes were built over a layout production never had.Changes
preUserIDSchemacarriesduration_msagain, verbatim from v2.duration_mspresent beforestorage.Open, gone after. A no-op cannot return unnoticed in either direction.SELECToverspansis planned as an ART index scan on the linked engine — measured for a bare constant, a bound parameter, an equality on the primary key, and the textbook case of a unique match in a 100 000-row table with a fresh index on exactly the probed column, withindex_scan_max_count/index_scan_percentageraised to admit everything and with and without thestorage_compatibility_versionpin. So "an indexed probe agrees with an unindexed one" was two sequential scans agreeing. What the pair does compare is filter pushdown against no pushdown, which is where ADR-0013's wrong answers came from, and the test now asserts the two probes still plan differently so the comparison cannot degenerate into one path checked twice.span_id(must be rejected, with the error checked to be a constraint error) and inserts an unused one (must be accepted, then removed).requireNoWALtolerates a present-but-empty WAL file: an engine that truncates instead of unlinking has folded everything, and failing on the inode would report the next such engine as a reproduction of this defect on the very assertion that guards the fold.docs/operations/duckdb-recovery.md: the fixture description, the assertion table (two new rows, the index-scan claim replaced with the honest one plus the measurement that forced it), and two added rows in "What has been run" beside the earlier two — with aFixturecolumn saying what each pair of runs did and did not exercise. CHANGELOG's unreleased entry for the reproducer is corrected to match.Verified
On linux/arm64 under
golang:1.24,CGO_ENABLED=1, againstduckdb-go/v2 v2.10506.0(SELECT version()→v1.5.6):COTEL_ART_REPRO_SPANS=200000 COTEL_ART_REPRO_CYCLES=6Also
go vet ./internal/storage/clean and the wholeinternal/storagepackage green (10.1 s).Nothing reproduced, so no escalation path was triggered and the forensic volume stays held under the unchanged release condition: not shown to reproduce at our scale on 1.5.6.
🤖 Generated with Claude Code