Repository navigation
Conversation
MarkBlockDirty, GetBlockCountSimple and GetBlockCountComposite have no callers. Remove them with their SQL templates and the BlockCountSimple and BlockCountComposite types, so that later changes to block bounds do not have to keep dead code in step.
The check "this bound is empty or all NULL" was written four times: as sliceAllNil in db/queries, as allNil in the mtree package, as rangeSliceAllNil and as an inline loop in table-diff. Keep one exported copy, queries.AllNil, and use it everywhere. The callers that also tested len() > 0 before the call no longer need to, because AllNil already treats an empty slice as open.
GetBlockRowCount, GetBulkSplitPoints and the max-value queries now treat a nil or all-NULL start or end as "no bound on this side". Before, the composite branch of GetBlockRowCount and GetBulkSplitPoints added a condition for an all-NULL bound, which compares the key with NULL and selects no rows. GetMaxValSimple and GetMaxValComposite search the whole table when they get no start key, and GetMaxValSimple returns nil when no row matches, like the composite variant. No caller passes an open start yet, so the behaviour does not change. The next commit gives the first block of a Merkle tree no lower bound and needs these helpers.
The block bounds come from one node only, the reference node. The first block started at the smallest key of that node. Rows on other nodes with a smaller key were in no block: no leaf hash covered them, and the diff reported the nodes as equal. Rows inserted after the build were handled, because CDC moved the start of the first block down, but rows that already differed at build time were never found. The first block (node_position 0) now has no lower bound, in the same way as the last block has no upper bound: - Leaf hashes, CDC dirty marking and the diff row fetch use no lower bound for this block. Every function that reads block ranges returns a nil start for it. - Row counts for merges add the rows below the stored range_start with a separate subquery, so the join keeps an index scan on the lower bound. - A split of the first block sets its stored range_start to the smallest key, so the block stays first when the leaves are numbered again by range_start. - Leaves with the same range_start keep their old order when they are numbered again. A reference node with one row gives two such leaves, and without the tie-break the second one could take position 0. - CDC no longer moves range_start. That code also took the smallest key as MIN() over text, which is wrong for numeric keys. - The hash version is now 3, so mtree update computes all leaf hashes of an older tree again.
Add an "Open-ended first block handling" item next to the one for the last block: why the first block has no lower bound, which code paths follow this rule, and why its stored range_start still matters for the order of the leaves.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configuration
📒 Files selected for processing (11)
💤 Files with no reviewable changes (1)
Included review availability: This review used your included allowance. Your plan provides up to 1 included review per hour; 0 remain after this review. 📝 WalkthroughWalkthroughPosition zero now has no lower bound for leaf hashing, row counts, and diff operations. Query templates handle absent starts, and position resets preserve order when leaf starts tie. Hash metadata advances to version 3. ChangesFirst-leaf bounds and Merkle diffs
Priority: ➖ Normal Severity of issue fixed: Medium Merge Risk: ⚪ Minimal · up to The inspected paths preserve first-leaf coverage during splitting and hash upgrades. No issue requiring a fix before merge was established. Security Architecture ReviewSecurity architecture risk: 🟡 Moderate · up to The normal upgrade path restores comparison coverage without expanding database permissions. However, running an older version against an upgraded tree can recreate incomplete hashes while leaving the tree marked as upgraded, potentially hiding differences after returning to the new version. Retained concerns
Security review detailsSecurity Blast Radius
Trust Boundaries and Controls
Resilience and Maintainability Implications
Hardening Proposals
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Docstring CoverageExplanation Docstring coverage is 42.11% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 38 functions across 8 files. (2 skipped: 2 unsupported.)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
A rabbit checks the leaf below, Comment |
Up to standards ✅🟢 Issues
|
| Metric | Results |
|---|---|
| Complexity | -9 |
| Duplication | -10 |
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.
Contributor
Author
|
We will raise the hash_version once before the release. |
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
1 participant
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.
Problem
A Merkle tree takes its block bounds from one node only: the reference node. The first block starts at that node's smallest key. Rows on other nodes with a smaller key were in no block. No leaf hash covered them, and mtree table-diff reported the nodes as equal.
Example: n1 has keys 100..1000, n2 has keys 1..1000. Rows 1..99 on n2 were never compared.
The tool handled rows inserted after the build because CDC moved the start of the first block down. Rows that already differed at build time were never found, and finding them is the tool's main use case.
Change
The first block (node_position = 0) now has no lower bound, just as the last block has no upper bound.
Leaf hashes, CDC dirty marking, merge row counts, and the diff row fetch all follow this rule. Every function that reads block ranges returns a nil start for the first block.
Merge row counts add the rows below the stored range_start with a separate subquery. An OR in the join condition would stop the index scan on the lower bound for every block.
The stored range_start of the first block is still a real key, because leaves are numbered by range_start. A split of the first block sets it to the table's smallest key. Leaves with the same range_start keep their old order, so the first leaf always stays at position 0.
CDC no longer moves range_start. That code also took the smallest key as MIN() over text, which is wrong for numeric keys.