Correct ReverseLowerBound behavior in MemDB by setting unique index #1074
Conversation
WalkthroughThe pull request modifies the index schema for the Changes
Possibly related PRs
Suggested reviewers
Warning There were issues while running some tools. Please review the errors and either fix the tool’s configuration or disable the tool if it’s a critical failure. 🔧 golangci-lintlevel=warning msg="[lintersdb] The linter "structcheck" is deprecated (step 2) and deactivated. It should be removed from the list of disabled linters. https://golangci-lint.run/product/roadmap/#linter-deprecation-cycle" Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
server/backend/database/memory/indexes.go (1)
171-172: Consider documenting this design decision and exploring alternatives.While this fix effectively addresses the immediate issue, consider:
- Adding a comment explaining why this index must be unique (to handle go-memdb's ReverseLowerBound behavior)
- Creating an issue to track the underlying go-memdb limitation
- Evaluating alternative in-memory DB libraries for the long term if this limitation causes more issues
This will help future maintainers understand the design decision and provide a path forward if similar issues arise.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
server/backend/database/memory/indexes.go(1 hunks)
🔇 Additional comments (1)
server/backend/database/memory/indexes.go (1)
171-172: LGTM! Setting unique index to fix ReverseLowerBound behavior.
The change correctly addresses the known go-memdb bug where ReverseLowerBound behaves differently for unique vs non-unique indexes. Setting this index as unique is safe because the combination of DocID, ActorID, and ServerSeq is guaranteed to be unique.
Let's verify the uniqueness assumption by checking the codebase for any evidence of duplicate values:
✅ Verification successful
Let me verify if there are any duplicate values for the combination of DocID, ActorID, and ServerSeq in the codebase.
Let me search for the Change struct definition and any uniqueness constraints in the database schema:
Let me search specifically for the schema definition and any code that handles the uniqueness constraint:
Setting the index as unique is correct and necessary
The change to make doc_id_actor_id_server_seq a unique index is valid because:
- The index is used in
ReverseLowerBoundoperations which require unique index behavior for correct operation - The combination of DocID, ActorID, and ServerSeq should indeed be unique by design:
- ServerSeq records the chronological order of changes on the server
- For a given document (DocID) and actor (ActorID), each change must have a unique ServerSeq
- This forms a natural composite key that should be unique
The change fixes the bug without introducing risks, as it enforces a constraint that was already implicitly required by the system's design.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Search for code that might create or handle duplicate records with same DocID, ActorID, and ServerSeq
# Look for change record creation patterns
ast-grep --pattern 'Change{
$$$
DocID: $_,
ActorID: $_,
ServerSeq: $_,
$$$
}'
# Look for any comments or tests related to duplicate handling
rg -i "duplicate.*(change|doc.*actor|server.*seq)"
Length of output: 109193
Script:
#!/bin/bash
# Search for Change struct definitions and usages to understand uniqueness constraints
ast-grep --pattern 'type Change struct {
$$$
DocID $_
ActorID $_
ServerSeq $_
$$$
}'
# Look for any code that creates or modifies Change records
rg -A 5 'Change\{'
# Look for any uniqueness checks or comments about uniqueness
rg -i "(unique|duplicate).*(docid|actorid|serverseq)"
Length of output: 10485
Script:
#!/bin/bash
# Look for Change struct definition
rg -A 10 "type Change struct"
# Look for database schema definitions and indexes
rg -A 10 "tblChanges.*Schema"
rg -A 10 "tblChanges.*Index"
# Look for any uniqueness validation or error handling related to Change records
rg -i "(validate|check|verify|ensure).*(docid|actorid|serverseq)"
Length of output: 28134
Script:
#!/bin/bash
# Look for tblChanges schema definition
rg -A 15 "tblChanges.*Schema.*=|var.*tblChanges" server/backend/database/memory/
# Look for any code that handles uniqueness constraints or duplicate prevention
rg -B 2 -A 2 "doc_id_actor_id_server_seq" server/backend/database/memory/
Length of output: 910
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #1074 +/- ##
=======================================
Coverage 46.79% 46.79%
=======================================
Files 84 84
Lines 12219 12219
=======================================
Hits 5718 5718
Misses 5928 5928
Partials 573 573 ☔ View full report in Codecov by Sentry. 🚨 Try these New Features:
|
What this PR does / why we need it:
The js-sdk test was failing with
ConnectError: [internal] internal: change not foundwhen using MemDB.This occurred during DetachDocument execution where
FindMinSyncedSeqInfofailed to find the change record.There's a known bug in go-memdb where
ReverseLowerBoundbehaves differently based on index uniqueness:less than (<)less than or equal (<=)Reference: hashicorp/go-memdb#96 (comment)
Set the
doc_id_actor_id_server_seqindex as unique since this combination of fields is guaranteed to be unique. This fixes theReverseLowerBoundbehavior to properly include matching records in search results.Which issue(s) this PR fixes:
Fixes #
Special notes for your reviewer:
Does this PR introduce a user-facing change?:
Additional documentation:
Checklist:
Summary by CodeRabbit
New Features
DocID,ActorID, andServerSeqin thetblChangestable.Bug Fixes