Add mtree vs table-diff performance benchmark (not run in CI)#127
Open
mason-sharp wants to merge 1 commit into
Open
Add mtree vs table-diff performance benchmark (not run in CI)#127mason-sharp wants to merge 1 commit into
mason-sharp wants to merge 1 commit into
Conversation
Add TestMtreeVsTableDiffPerformance, an integration benchmark comparing the wall-clock cost of mtree table-diff (DiffMtree) against brute-force table-diff on a 1M-row table as divergence grows. It validates that the concurrent CDC drain and prompt LSN-based stop keep mtree competitive on large tables. The test builds a merkle tree on synced data, then runs three rounds of deterministic, evenly-scattered divergence (0.01% / 0.1% / 1%, selected via (index * 2654435761) % 1000000 < threshold). For each it times both diff paths and logs the speedup. Only the 0.01% round asserts mtree is faster; 0.1% and 1% are informational, since scattered divergence saturates most leaf blocks at higher rates. This test is intentionally NOT wired into .github/workflows/test.yml: it is a benchmark whose pass/fail at low divergence rides on a narrow timing margin (~1.15x), which would be flaky on shared CI runners. Run it on demand with: go test -count=1 -v ./tests/integration -run 'TestMtreeVsTableDiffPerformance$' It is gated by testing.Short() so it never runs in short-mode passes.
📝 WalkthroughWalkthroughAdded ChangesMtree diff performance test
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
Not up to standards ⛔🔴 Issues
|
| Category | Results |
|---|---|
| Security | 3 critical (3 false positives) |
| Complexity | 2 medium |
🟢 Metrics 19 complexity · 2 duplication
Metric Results Complexity 19 Duplication 2
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.
Actionable comments posted: 3
🧹 Nitpick comments (1)
tests/integration/mtree_perf_test.go (1)
65-76: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winTie
scatterModdirectly totargetRows.The comments say
scatterModmust matchtargetRows, but the constants can drift independently and silently invalidate the deterministic row-count assumption.Proposed change
- scatterMod = 1000000 + scatterMod = targetRows🤖 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_perf_test.go` around lines 65 - 76, The deterministic row-selection setup in the performance test can drift because scatterMod is hardcoded separately from targetRows. Update the constants in mtree_perf_test.go so scatterMod is derived directly from targetRows in the same const block, and keep the existing scatterMult/scatterMod comment aligned with targetRows to preserve the exact row-count assumption.
🤖 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_perf_test.go`:
- Around line 222-224: The current assertions in runComparison only check that
mtree and brute-force detect some divergence, but they do not verify the
expected number of differing rows; update the test to assert the exact expected
diff counts for both mc and bc in mtree_perf_test.go using runComparison and the
r.label-driven predicate, so the test fails if either implementation misses most
divergent rows.
- Around line 120-123: The cleanup in the mtree performance test is too broad
and can delete unrelated diff reports. Update the cleanup logic in the test
around the filepath.Glob and os.Remove loop to only target diff files created by
this schema/table, or better, use the exact report paths returned by the diff
writer functions so the test only removes its own artifacts.
- Around line 88-111: Register cleanup before any setup that can fail in
mtree_perf_test: the current cleanup for mtreeTask is added only after table
creation, inserts, RunChecks, and MtreeInit, so failures can leave
customers_perf_test or partial merkle-tree state behind. Move the MtreeTeardown
registration to immediately after newMerkleTreeTask creates mtreeTask, before
calling RunChecks or MtreeInit, so the task is always cleaned up even if setup
aborts early.
---
Nitpick comments:
In `@tests/integration/mtree_perf_test.go`:
- Around line 65-76: The deterministic row-selection setup in the performance
test can drift because scatterMod is hardcoded separately from targetRows.
Update the constants in mtree_perf_test.go so scatterMod is derived directly
from targetRows in the same const block, and keep the existing
scatterMult/scatterMod comment aligned with targetRows to preserve the exact
row-count assumption.
🪄 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: 8e930132-d08f-42be-8d2e-e393e274a34b
📒 Files selected for processing (1)
tests/integration/mtree_perf_test.go
Comment on lines
+88
to
+111
| for i, pool := range []*pgxpool.Pool{env.N1Pool, env.N2Pool} { | ||
| nodeName := pgCluster.ClusterNodes[i]["Name"].(string) | ||
| if err := createTestTable(ctx, pool, testSchema, tableName); err != nil { | ||
| t.Fatalf("failed to create table on %s: %v", nodeName, err) | ||
| } | ||
| _, err := pool.Exec(ctx, fmt.Sprintf( | ||
| `INSERT INTO "%s"."%s" SELECT * FROM "%s"."customers_1M" WHERE index <= %d`, | ||
| testSchema, tableName, testSchema, targetRows, | ||
| )) | ||
| if err != nil { | ||
| t.Fatalf("failed to populate table on %s: %v", nodeName, err) | ||
| } | ||
| log.Printf("Populated %s with %d rows on %s", qualifiedTableName, targetRows, nodeName) | ||
| } | ||
|
|
||
| // --- Build the merkle tree on the synced data (one-time setup) ---------- | ||
| mtreeTask := env.newMerkleTreeTask(t, qualifiedTableName, nodes) | ||
| mtreeTask.BlockSize = blockSize | ||
| mtreeTask.MaxCpuRatio = 1.0 // parallelise build/recompute across CPUs | ||
|
|
||
| require.NoError(t, mtreeTask.RunChecks(false), "mtree RunChecks should succeed") | ||
| require.NoError(t, mtreeTask.MtreeInit(), "MtreeInit should succeed") | ||
|
|
||
| t.Cleanup(func() { |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Register cleanup before setup can fail.
Cleanup is registered only after table creation, bulk inserts, RunChecks, and MtreeInit. Any earlier failure can leave customers_perf_test or partial mtree state behind and poison later on-demand runs.
Suggested structure
tableName := "customers_perf_test"
qualifiedTableName := fmt.Sprintf("%s.%s", testSchema, tableName)
nodes := []string{env.ServiceN1, env.ServiceN2}
+
+ t.Cleanup(func() {
+ for _, pool := range []*pgxpool.Pool{env.N1Pool, env.N2Pool} {
+ if _, err := pool.Exec(ctx, fmt.Sprintf(`DROP TABLE IF EXISTS "%s"."%s" CASCADE`, testSchema, tableName)); err != nil {
+ t.Logf("cleanup: failed to drop table: %v", err)
+ }
+ }
+ })
// --- Seed both nodes with identical data -------------------------------Then register MtreeTeardown immediately after mtreeTask is created, before RunChecks/MtreeInit.
🤖 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_perf_test.go` around lines 88 - 111, Register cleanup
before any setup that can fail in mtree_perf_test: the current cleanup for
mtreeTask is added only after table creation, inserts, RunChecks, and MtreeInit,
so failures can leave customers_perf_test or partial merkle-tree state behind.
Move the MtreeTeardown registration to immediately after newMerkleTreeTask
creates mtreeTask, before calling RunChecks or MtreeInit, so the task is always
cleaned up even if setup aborts early.
Comment on lines
+120
to
+123
| files, _ := filepath.Glob("*_diffs-*.json") | ||
| for _, f := range files { | ||
| os.Remove(f) | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
Scope diff-file cleanup to this test.
*_diffs-*.json can remove unrelated diff reports from the working directory. Limit the glob to this schema/table, or track the exact report paths returned by the diff writers.
Example scoped glob
- files, _ := filepath.Glob("*_diffs-*.json")
+ files, _ := filepath.Glob(fmt.Sprintf("*%s*%s*_diffs-*.json", testSchema, tableName))📝 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
| files, _ := filepath.Glob("*_diffs-*.json") | |
| for _, f := range files { | |
| os.Remove(f) | |
| } | |
| files, _ := filepath.Glob(fmt.Sprintf("*%s*%s*_diffs-*.json", testSchema, tableName)) | |
| for _, f := range files { | |
| os.Remove(f) | |
| } |
🤖 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_perf_test.go` around lines 120 - 123, The cleanup in
the mtree performance test is too broad and can delete unrelated diff reports.
Update the cleanup logic in the test around the filepath.Glob and os.Remove loop
to only target diff files created by this schema/table, or better, use the exact
report paths returned by the diff writer functions so the test only removes its
own artifacts.
Comment on lines
+222
to
+224
| m, b, mc, bc := runComparison(r.label) | ||
| require.Greater(t, mc, 0, "[%s] mtree should detect the divergence", r.label) | ||
| require.Greater(t, bc, 0, "[%s] brute-force should detect the divergence", r.label) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Assert exact diff counts, not just non-zero detection.
A diff path could miss most divergent rows and still pass. Since the predicate is deterministic for a 1M-row table, assert the expected threshold for both implementations.
Proposed assertion tightening
- require.Greater(t, mc, 0, "[%s] mtree should detect the divergence", r.label)
- require.Greater(t, bc, 0, "[%s] brute-force should detect the divergence", r.label)
+ require.Equal(t, r.threshold, mc, "[%s] mtree should detect every divergent row", r.label)
+ require.Equal(t, r.threshold, bc, "[%s] brute-force should detect every divergent row", r.label)📝 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
| m, b, mc, bc := runComparison(r.label) | |
| require.Greater(t, mc, 0, "[%s] mtree should detect the divergence", r.label) | |
| require.Greater(t, bc, 0, "[%s] brute-force should detect the divergence", r.label) | |
| m, b, mc, bc := runComparison(r.label) | |
| require.Equal(t, r.threshold, mc, "[%s] mtree should detect every divergent row", r.label) | |
| require.Equal(t, r.threshold, bc, "[%s] brute-force should detect every divergent row", r.label) |
🤖 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_perf_test.go` around lines 222 - 224, The current
assertions in runComparison only check that mtree and brute-force detect some
divergence, but they do not verify the expected number of differing rows; update
the test to assert the exact expected diff counts for both mc and bc in
mtree_perf_test.go using runComparison and the r.label-driven predicate, so the
test fails if either implementation misses most divergent rows.
danolivo
approved these changes
Jun 26, 2026
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
2 participants
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.
Add TestMtreeVsTableDiffPerformance, an integration benchmark comparing the wall-clock cost of mtree table-diff (DiffMtree) against brute-force table-diff on a 1M-row table as divergence grows. It validates that the concurrent CDC drain and prompt LSN-based stop keep mtree competitive on large tables.
The test builds a merkle tree on synced data, then runs three rounds of deterministic, evenly-scattered divergence (0.01% / 0.1% / 1%, selected via (index * 2654435761) % 1000000 < threshold). For each it times both diff paths and logs the speedup. Only the 0.01% round asserts mtree is faster; 0.1% and 1% are informational, since scattered divergence saturates most leaf blocks at higher rates.
This test is intentionally NOT wired into .github/workflows/test.yml: it is a benchmark whose pass/fail at low divergence rides on a narrow timing margin (~1.15x), which would be flaky on shared CI runners. Run it on demand with:
go test -count=1 -v ./tests/integration -run 'TestMtreeVsTableDiffPerformance$'
It is gated by testing.Short() so it never runs in short-mode passes.