fix(#6360): CHECK DATABASE gets a DEEP tier, a FIX that repairs what is derived, and a sealed block that knows where it lives - #6366
Conversation
…is derived, and a sealed block that knows where it lives Three follow-ups from #6340, two of them the design questions that issue deferred and one a latent inconsistency it had to work around. 1. DEEP. The default tier already reads every byte of every sealed store to verify its per-block CRC32s, which proves the bytes are the bytes that were written and nothing about whether they MEAN what the block claims. Three things every read path answers queries from, without ever looking at a value, were verified by nothing: sorted timestamps (the range iterator binary-searches them), the declared per-column min/max/sum (the aggregation push-down answers from them without decompressing), and the declared distinct tag values (block pruning SKIPS a block on them). Each is a wrong ANSWER rather than an error. CHECK DATABASE ... DEEP decodes every block and reconciles all three. #6360 item 2 asked the opposite question - whether the CRC pass should move behind the clause - and the answer is no. It is the same cost class as the record scan checkBuckets already runs over every bucket of every document type, and a default that skipped it would answer "clean" having read only the directory, which is the misleading-clean result the whole check exists to end. 2. FIX. It repairs DERIVED bookkeeping and never a sample: the mutable bucket's page-0 counters, the sealed header's block count and global bounds, and the tail of an interrupted sealed append. None of that is cosmetic - loadDirectory reads the global bounds out of the header instead of recomputing them, so a query pruned against a wrong bound silently misses data the file holds, and appendBlock writes at the END of the file, so a tail nothing can read makes every block appended after it unreadable too. The tail repair is deliberately narrow: only a tail that STARTS with a block magic is dropped, since that one is a block loadDirectory recognised and could not read to the end of. A tail that does not could equally be a COMPLETE block whose magic took a bit flip, and that one is reported and left where a hex editor can still reach it. A sealed block that fails its CRC is never discarded, repacked or rewritten. It is the only copy of the samples in it, so "repair" there means choosing which samples to throw away - an operator's decision, made with the operator's knowledge of the cluster, and a checker that made it for them would be deleting data to make its own report come out clean. The mutable header repair rides in the shard's existing append window and follows appendSamples' rule to the letter: on a replicated database the compaction read lock is released BEFORE the commit, because a leader's commit waits for the active recording session and a compaction holding one is waiting for that lock. A compaction slipping into the gap is reported as a repair that did not land, not retried. 3. BlockEntry.blockStartOffset and storedCRC were assigned in exactly one place, inside loadDirectory(). A block this process WROTE carried zero in both while the constructor pre-set crcValidated = true, and since commitTempCompactionFile installs a rewritten directory without re-reading it, that was the state of the entire live directory after every compaction, retention pass and downsampling cycle. Unreachable only because two independent facts lined up: clearing the flag produced "CRC mismatch in sealed store block at offset 0 (stored=0x0, ...)" on a perfectly healthy store, which is what the new test pins. Both fields are now set by every path that produces an entry, which also lets the check hold the directory in memory against the file - a comparison that was meaningless while one side was always zero. New result keys: repairedTimeSeries and timeSeriesRepairs. Neither folds into autoFix, which has always been the record-action count (#6136). Engine module green at 12,595 tests.
|
Tick the box to add this pull request to the merge queue (same as
|
Up to standards ✅🟢 Issues
|
| Metric | Results |
|---|---|
| Complexity | 67 |
🟢 Coverage 81.62% diff coverage · -6.60% coverage variation
Metric Results Coverage variation ✅ -6.60% coverage variation Diff coverage ✅ 81.62% diff coverage Coverage variation details
Coverable lines Covered lines Coverage Common ancestor commit (90b0959) 160706 127762 79.50% Head commit (84fa973) 193294 (+32588) 140910 (+13148) 72.90% (-6.60%) Coverage variation is the difference between the coverage for the head and common ancestor commits of the pull request branch:
<coverage of head commit> - <coverage of common ancestor commit>Diff coverage details
Coverable lines Covered lines Diff coverage Pull request (#6366) 272 222 81.62% Diff coverage is the percentage of lines that are covered by tests out of the coverable lines that the pull request added or modified:
<covered lines added or modified>/<coverable lines added or modified> * 100%
NEW Get contextual insights on your PRs based on Codacy's metrics, along with PR and Jira context, without leaving GitHub. Enable AI reviewer
TIP This summary will be updated as you push new changes.
ReviewReviewed the DEEP/FIX additions to Correctness1. 2. DEEP-mode block decode doesn't gate on a detected offset mismatch ( Design / robustness3. 4. 5. Minor duplication (nice-to-have, not blocking)
Efficiency
Test coverage gapThe new Nothing here blocks the overall approach - the DEEP/FIX semantics, the tail-truncation narrowness, and the sample-never-touched guarantee all look correctly implemented (verified by tracing every |
…decode stops where the offsets stop agreeing Both correctness findings from the review, plus the three robustness ones and the duplication the review is right that a checker cannot afford. 1. TimeSeriesShard.checkIntegrity appended the mutable bucket's repairs to its report BEFORE attempting the commit that carries them, so a ConcurrentModificationException on that commit produced a report which simultaneously counted the repair and told the operator to run the check again. The merge now happens after the commit resolves, so the CME arm is reached with the list untouched. The totals had the same shape of bug and are worse for it: they were read INSIDE the transaction, so they described the repaired header that never landed. They are re-read from what survived. The comment claiming the CME path "reports zero" described neither the old code nor the new one and is gone. 2. The DEEP decode ran whenever the CRC matched, including for a block whose in-memory offset had just been reported as disagreeing with the file. checkBlockContent slices columns at entry.columnOffsets[c] minus the file-derived blockStart, so it would have reported wrong statistics - or an undecodable block - for a block whose actual fault was the mismatch reported two lines earlier. It is now gated on both. 3. TimeSeriesIntegrity.Options is a positional two-boolean record and the compiler accepts the swap. On a repair command that is a mistake with consequences and no symptom, so the four combinations are named: of(deep, fix), deepOnly(), fixOnly(), deepAndFix(). No caller writes two booleans. 4. BlockEntry took its offset by assignment after construction, which is the shape of the bug this issue fixes: two of the three write sites simply never did it. The offset is now a constructor parameter, so a new write path cannot build an entry without deciding where its block is, and crcValidated starts FALSE - granted only by recordWrittenCRC(crc), together with the CRC it is a shortcut for. 5. Duplication the check cannot afford. reduceNumericStats is now ONE definition shared by the two write paths that produce a block's min/max/sum and by the DEEP check that verifies them; it was three copies, and a verification reducing differently from the writer reports a healthy block as damaged. Likewise writeHeaderCounters is one definition behind all five page-0 counter writes, including the repair whose javadoc claimed it wrote them "the same way" as the maintenance paths - a claim that was a comment and is now the same call. Also splits the generic catch that Codacy flagged for an instanceof on the caught exception into its own IOException clause. NOT done, with reasons rather than silence: - Sharing the lock/transaction protocol with appendSamples. The divergence the review found - appendSamples retries a compaction-race CME three times, this gives up on the first - is required, not drift: an append retries the SAME rows, while these counters came from a page walk the winning compaction has just invalidated, so a retry would write numbers that no longer describe the pages. Said so in the javadoc. - Fanning the shards out to shardExecutor for DEEP. aggregateMultiBlocks can because it is read-only; the FIX path opens a database transaction per shard and ArcadeDB transactions are thread-bound, so this is a materially different proposition and belongs in its own issue if the sequential cost ever bites. - A test for the CME arm. Forcing a deterministic MVCC conflict on page 0 mid-check needs a production seam that exists only for the test; after item 1 the inconsistent report is unreachable by construction rather than by assertion, which is the stronger guarantee. The gap is real and stated. Engine module green at 12,596 tests.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #6366 +/- ##
==========================================
- Coverage 70.81% 70.81% -0.01%
==========================================
Files 1852 1853 +1
Lines 160706 160907 +201
Branches 33763 33823 +60
==========================================
+ Hits 113806 113940 +134
- Misses 32983 33041 +58
- Partials 13917 13926 +9 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
…e statement, not two versions of one #6189 (CHECK DATABASE FIX RECLAIM UNREFERENCED FILES) landed on main while this branch was open and touched the same four places, so every conflict is the same shape: both sides added a clause where there was one. All four resolved by keeping both. - SQLLexer.g4 / SQLParser.g4's identifier-keyword list: RECLAIM, UNREFERENCED, FILES and DEEP all stay usable as identifiers. - checkDatabaseStatement: the clause order is FIX, DELETE ORPHANS, RECLAIM UNREFERENCED FILES, DEEP, COMPRESS - pinned by a parser test rather than left to be rediscovered, since the two new clauses now sit adjacent and the ordering is the part a resolution can silently get wrong. - SQLASTBuilder and CheckDatabaseStatement: both flags parsed, both round-tripped by toString, both passed to the checker. DatabaseChecker merged cleanly and carries both setters. The two features are independent: RECLAIM removes files a schema component was never built for and requires FIX, DEEP decodes sealed blocks and repairs nothing. Engine module green at 12,637 tests, which includes #6189's own reclaim and parser tests alongside this issue's.
…ns kept main advanced again while the first merge was building. The only conflict is docs/release-26.9.1.md, where both sides appended a new section at the end of the file; kept in landing order, main's #6359 sections first and this issue's #6360 section last. Everything else auto-merged, SQLASTBuilder included. Engine module green at 12,773 tests.
ReviewReviewed the diff (grammar, OverallThis is a well-scoped, carefully-reasoned change. A few things stood out positively:
Points worth a second look
None of the above are blockers — the design decisions in #1 and #3 both look intentional and defensible, they're just worth a maintainer's explicit sign-off given the stated "always bear in mind PERFORMANCE" project priority. Nice work on the amount of doc/test rigor here, especially given this closes out real latent defects (item 3) rather than just adding new surface area. |
1 participant
Closes #6360.
Three follow-ups from #6340, two of them the design questions that issue deferred and one a latent inconsistency it had to work around.
1.
CHECK DATABASE ... DEEPdecodes the data instead of reconciling what describes it (item 1)The default tier already reads every byte of every sealed store to verify its per-block CRC32s. That proves the bytes are the bytes that were written, and proves nothing about whether they mean what the block claims. Three things every read path answers queries from, without ever looking at a value, were verified by nothing:
iterateRangebinary-searches a block's timestamps withlowerBound/upperBound, so an unsorted block silently returns a subset of the rows that match;MIN/MAX/SUM/AVGstraight from them without decompressing anything;Each of those, when wrong, is a wrong answer rather than an error, which is the category of damage a checker exists for.
2. The per-block CRC pass stays in the DEFAULT tier (item 2)
The issue proposed the opposite - moving the CRC pass behind the new clause. Rejected, and stated so it is not re-litigated: it is the same cost class as the record scan
checkBucketsalready runs over every bucket of every document type,CHECK DATABASEis an explicitly-invoked maintenance operation, and a default tier that skipped it would answer "clean" having read only the directory. That is the misleading-clean result the whole of #6340 item 4 was about. What is opt-in is the part whose cost is decompression of the dataset rather than a sequential read of it.3.
FIXrepairs what is derived, and never a sample (item 1, second half)Three things a TimeSeries type stores are derived from data they merely describe, and all three are repaired: the mutable bucket's page-0 counters, the sealed header's block count and global timestamp bounds, and the tail of an interrupted sealed append. None of that is cosmetic:
loadDirectoryreads the sealed global bounds out of the header rather than recomputing them, so a range query pruned against a wrong bound silently misses data the file holds;appendBlockwrites at the end of the sealed file, so a tail nothing can read makes every block appended after it unreadable too.The tail repair is deliberately narrow. Only a tail that STARTS with a block magic is dropped - that one is a block
loadDirectoryrecognised and could not read to the end of, so it is incomplete by its own evidence. A tail that does not could equally be a complete block whose magic took a bit flip, which a hex editor can still recover, so it is reported and left exactly where it is.A sealed block that fails its CRC or its content check is never discarded, repacked or rewritten. It is the only copy of the samples in it, so "repair" there means choosing which samples to throw away. Under HA a sealed store is derived and a node can rebuild one by recompacting from its replicated mutable pages, which makes discarding one recoverable, not automatic.
The mutable header repair rides in the shard's existing append window and follows
appendSamples' rule to the letter: on a replicated database the compaction read lock is released BEFORE the commit, because a leader's commit waits for the active recording session and a compaction holding one is waiting for that lock. A compaction slipping into the gap is reported as a repair that did not land, not retried.Two new result keys,
repairedTimeSeries(a count) andtimeSeriesRepairs(one line each). Neither folds intoautoFix, which has always been the record-action count (#6136).4. A sealed block now records where it is and which CRC guards it (item 3)
BlockEntry.blockStartOffsetandstoredCRCwere assigned in exactly one place, insideloadDirectory(), so a block this process wrote carried zero in both while the constructor pre-setcrcValidated = true. SincecommitTempCompactionFileinstalls a rewritten directory without re-reading it, that was the state of the entire live directory after every compaction, retention pass and downsampling cycle.Unreachable only because two independent facts lined up. Clearing the flag on a perfectly healthy store produced exactly this, which is what the regression test pins:
Both fields are now set by every path that produces an entry, which also lets the check hold the directory in memory against the file - a comparison that was meaningless while one side was always zero. The
-1sentinel the comment promised and nothing ever wrote is gone from the comment.Tests
Issue6360SealedStoreIntegrityTest(12): the item-3 regression, and one case per DEEP finding - wrong stats, wrong sum, unsorted timestamps, a missing declared tag value, wrong declared bounds - each asserting the default tier is silent and the deep tier is not. Plus the FIX arms: the header rewrite, the tail that is dropped, the tail that is not, and the CRC-failed block the run refuses to touch.Issue6360CheckDatabaseTimeSeriesTest(6): the same through SQL, including that the mutable-header repair survives a reopen and that a report-only run changes nothing.CheckDatabaseStatementTestParserTest: theDEEPclause, its composition with the others, and that a schema with a type calleddeepkeeps parsing.Engine module green at 12,595 tests, 0 failures.