Ace 205 mtree table diff misses differences same pk different data#150
Open
zaidshabbir25 wants to merge 2 commits into
Open
Ace 205 mtree table diff misses differences same pk different data#150zaidshabbir25 wants to merge 2 commits into
zaidshabbir25 wants to merge 2 commits into
Conversation
📝 WalkthroughWalkthroughChangesMerkle tree correctness fixes
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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 |
Up to standards ✅🟢 Issues
|
| Metric | Results |
|---|---|
| Complexity | 18 |
| Duplication | 4 |
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.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
internal/consistency/mtree/merkle.go (1)
1532-1544: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider removing redundant pool cleanup.
The explicit pool-closing loops here are redundant because there is already a deferred cleanup function at line 1477 that safely closes all pools upon return. You can simplify these error paths by returning the errors directly.
♻️ Proposed refactor
if successfulEstimates == 0 { - for _, p := range pools { - p.Close() - } return fmt.Errorf("could not determine a reference node; failed to get row estimates from all nodes") } if maxRows == 0 { - for _, p := range pools { - p.Close() - } return fmt.Errorf("table %s has 0 rows on all nodes; add data before building a Merkle tree", m.QualifiedTableName) }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/consistency/mtree/merkle.go` around lines 1532 - 1544, Remove the redundant pool-closing loops from the successfulEstimates == 0 and maxRows == 0 error branches in the surrounding Merkle tree-building function. Return the existing errors directly and rely on the deferred cleanup established near the function’s setup.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@internal/consistency/mtree/merkle.go`:
- Around line 1532-1544: Remove the redundant pool-closing loops from the
successfulEstimates == 0 and maxRows == 0 error branches in the surrounding
Merkle tree-building function. Return the existing errors directly and rely on
the deferred cleanup established near the function’s setup.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 331919c5-687b-4029-93a0-fa72bfc2f234
📒 Files selected for processing (5)
db/queries/queries.godocs/CHANGELOG.mdinternal/consistency/mtree/merkle.gotests/integration/mtree_empty_table_test.gotests/integration/mtree_update_conflict_test.go
…es (ACE-205)
mtree table-diff could report divergent nodes as identical: the same primary
key holding different non-key data on two nodes produced matching Merkle-tree
root hashes, so the diff printed "Merkle trees are identical" while table-diff
correctly found the difference. This is the core UPDATE-UPDATE conflict that
active-active clusters produce, silently reported as consistent.
Root cause: leaf hashing used a closed ("<=") upper bound on block ranges, but
a block's range_end is the EXCLUSIVE start of the next block -- the build
offsets query pairs bounds with LEAD (each range_end is the following block's
range_start) and splitBlocks sets range_end to a split point that
GetBulkSplitPoints returns as the first row of the next chunk. The closed bound
double-counted every boundary row into two adjacent leaves. When a table's rows
collapsed into overlapping leaves that hashed identically (most visibly a
single-row table, where the row lands in both [pk,pk] and [pk,inf)), the
XOR-based parent hash cancelled the duplicate siblings to zero on every node,
so divergent data yielded matching roots.
Leaf hashing now uses an exclusive ("<") upper bound, matching the boundaries
the build and split paths already produce, so each row belongs to exactly one
leaf. Merkle trees built before this fix must be rebuilt (mtree build) to pick
up the corrected layout.
Adds an integration regression test (single-row and multi-row-boundary cases)
asserting mtree diff agrees with table-diff. Verified against the full mtree
integration suite plus db/queries and consistency unit tests.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
zaidshabbir25
force-pushed
the
ACE-205-mtree-table-diff-misses-differences-same-pk-different-data
branch
from
July 17, 2026 13:01
ee21deb to
8bce136
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tests/integration/mtree_update_conflict_test.go`:
- Around line 74-76: Update the local seed function to call t.Helper() and defer
tx.Rollback(ctx) immediately after Begin succeeds, ensuring the transaction is
aborted and its connection returned even when a require.NoError assertion exits
the helper.
- Around line 39-55: Update runMtreeUpdateConflictCase to accept an explicit
divergentID argument, pass 3 from the MultiRowBoundary subtest and the
appropriate existing ID from SingleRow, and remove the helper’s ids[len(ids)-1]
selection. Add t.Helper() at the start of runMtreeUpdateConflictCase so
assertion failures point to the calling t.Run block.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 403eb9a4-3ddf-461d-9e99-f472d2224a84
📒 Files selected for processing (3)
db/queries/queries.godocs/CHANGELOG.mdtests/integration/mtree_update_conflict_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- db/queries/queries.go
Comment on lines
+39
to
+55
| t.Run("SingleRow", func(t *testing.T) { | ||
| runMtreeUpdateConflictCase(t, "mtree_uu_conflict_1", []int{1}) | ||
| }) | ||
| t.Run("MultiRowBoundary", func(t *testing.T) { | ||
| // The divergent row (id=3) is the middle block boundary under BlockSize=2. | ||
| runMtreeUpdateConflictCase(t, "mtree_uu_conflict_n", []int{1, 2, 3, 4, 5}) | ||
| }) | ||
| } | ||
|
|
||
| // runMtreeUpdateConflictCase seeds ids on both nodes, diverging the LAST id's | ||
| // non-key column, then asserts both table-diff and mtree diff report exactly | ||
| // one divergent row. | ||
| func runMtreeUpdateConflictCase(t *testing.T, tableName string, ids []int) { | ||
| ctx := context.Background() | ||
| qualifiedTable := fmt.Sprintf("%s.%s", testSchema, tableName) | ||
| nodes := []string{serviceN1, serviceN2} | ||
| divergentID := ids[len(ids)-1] |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Align test logic with the intended boundary test case.
The comment on line 43 indicates the MultiRowBoundary test is meant to diverge id=3 (the middle block boundary). However, runMtreeUpdateConflictCase hardcodes the divergent ID to the last element (ids[len(ids)-1]), which is 5. This causes the test to diverge the final row rather than the middle one, missing the intended boundary scenario.
Consider passing the divergentID explicitly to align the test logic with its intent. Additionally, adding t.Helper() ensures that any test assertion failures within this function are attributed to the exact t.Run block rather than the helper's internal lines.
🛠️ Proposed fix to explicitly pass `divergentID`
t.Run("SingleRow", func(t *testing.T) {
- runMtreeUpdateConflictCase(t, "mtree_uu_conflict_1", []int{1})
+ runMtreeUpdateConflictCase(t, "mtree_uu_conflict_1", []int{1}, 1)
})
t.Run("MultiRowBoundary", func(t *testing.T) {
// The divergent row (id=3) is the middle block boundary under BlockSize=2.
- runMtreeUpdateConflictCase(t, "mtree_uu_conflict_n", []int{1, 2, 3, 4, 5})
+ runMtreeUpdateConflictCase(t, "mtree_uu_conflict_n", []int{1, 2, 3, 4, 5}, 3)
})
}
-// runMtreeUpdateConflictCase seeds ids on both nodes, diverging the LAST id's
+// runMtreeUpdateConflictCase seeds ids on both nodes, diverging the specified id's
// non-key column, then asserts both table-diff and mtree diff report exactly
// one divergent row.
-func runMtreeUpdateConflictCase(t *testing.T, tableName string, ids []int) {
+func runMtreeUpdateConflictCase(t *testing.T, tableName string, ids []int, divergentID int) {
+ t.Helper()
ctx := context.Background()
qualifiedTable := fmt.Sprintf("%s.%s", testSchema, tableName)
nodes := []string{serviceN1, serviceN2}
- divergentID := ids[len(ids)-1]📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| t.Run("SingleRow", func(t *testing.T) { | |
| runMtreeUpdateConflictCase(t, "mtree_uu_conflict_1", []int{1}) | |
| }) | |
| t.Run("MultiRowBoundary", func(t *testing.T) { | |
| // The divergent row (id=3) is the middle block boundary under BlockSize=2. | |
| runMtreeUpdateConflictCase(t, "mtree_uu_conflict_n", []int{1, 2, 3, 4, 5}) | |
| }) | |
| } | |
| // runMtreeUpdateConflictCase seeds ids on both nodes, diverging the LAST id's | |
| // non-key column, then asserts both table-diff and mtree diff report exactly | |
| // one divergent row. | |
| func runMtreeUpdateConflictCase(t *testing.T, tableName string, ids []int) { | |
| ctx := context.Background() | |
| qualifiedTable := fmt.Sprintf("%s.%s", testSchema, tableName) | |
| nodes := []string{serviceN1, serviceN2} | |
| divergentID := ids[len(ids)-1] | |
| t.Run("SingleRow", func(t *testing.T) { | |
| runMtreeUpdateConflictCase(t, "mtree_uu_conflict_1", []int{1}, 1) | |
| }) | |
| t.Run("MultiRowBoundary", func(t *testing.T) { | |
| // The divergent row (id=3) is the middle block boundary under BlockSize=2. | |
| runMtreeUpdateConflictCase(t, "mtree_uu_conflict_n", []int{1, 2, 3, 4, 5}, 3) | |
| }) | |
| } | |
| // runMtreeUpdateConflictCase seeds ids on both nodes, diverging the specified id's | |
| // non-key column, then asserts both table-diff and mtree diff report exactly | |
| // one divergent row. | |
| func runMtreeUpdateConflictCase(t *testing.T, tableName string, ids []int, divergentID int) { | |
| t.Helper() | |
| ctx := context.Background() | |
| qualifiedTable := fmt.Sprintf("%s.%s", testSchema, tableName) | |
| nodes := []string{serviceN1, serviceN2} |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/integration/mtree_update_conflict_test.go` around lines 39 - 55, Update
runMtreeUpdateConflictCase to accept an explicit divergentID argument, pass 3
from the MultiRowBoundary subtest and the appropriate existing ID from
SingleRow, and remove the helper’s ids[len(ids)-1] selection. Add t.Helper() at
the start of runMtreeUpdateConflictCase so assertion failures point to the
calling t.Run block.
Comment on lines
+74
to
+76
| seed := func(pool *pgxpool.Pool, divergentName string) { | ||
| tx, err := pool.Begin(ctx) | ||
| require.NoError(t, err) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Prevent connection leaks on test failure.
If an assertion within seed fails, require.NoError immediately exits the goroutine. Adding defer tx.Rollback(ctx) ensures the transaction is safely aborted and the connection is returned to the pool, preventing resource exhaustion in the CI environment.
Additionally, adding t.Helper() will improve failure readability.
🔌 Proposed fix to add defer tx.Rollback
seed := func(pool *pgxpool.Pool, divergentName string) {
+ t.Helper()
tx, err := pool.Begin(ctx)
require.NoError(t, err)
+ defer tx.Rollback(ctx) // Safe to call even after Commit() in pgx📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| seed := func(pool *pgxpool.Pool, divergentName string) { | |
| tx, err := pool.Begin(ctx) | |
| require.NoError(t, err) | |
| seed := func(pool *pgxpool.Pool, divergentName string) { | |
| t.Helper() | |
| tx, err := pool.Begin(ctx) | |
| require.NoError(t, err) | |
| defer tx.Rollback(ctx) // Safe to call even after Commit() in pgx |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/integration/mtree_update_conflict_test.go` around lines 74 - 76, Update
the local seed function to call t.Helper() and defer tx.Rollback(ctx)
immediately after Begin succeeds, ensuring the transaction is aborted and its
connection returned even when a require.NoError assertion exits the helper.
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.
mtree build on an empty table fails with a confusing internal error ("could not determine a reference node") instead of a clear "table has no rows" message