fix(storage): scope ledger queries with bucket hint #1251
Conversation
…coping The per-query sub-select on _system.ledgers to determine if a ledger is alone in its bucket is replaced by a boolean flag set at store open/create time. This avoids repeated cross-schema queries and prepares the ground for safe store caching (the flag must be invalidated when bucket composition changes). Backport of #1250.
WalkthroughThe changes introduce an optimization for ledger query filtering by computing and caching a flag that indicates whether a ledger is the sole ledger in its bucket. This flag is set during ledger creation and opening, and subsequently used to conditionally skip ledger filters in query construction, replacing prior dynamic bucket-level count operations. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
flemzord
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@internal/storage/driver/driver.go`:
- Around line 80-85: The current CreateLedger flow sets ret.SetAloneInBucket
based on systemStore.CountLedgersInBucket which can race under concurrent
CreateLedger calls (TOCTOU); fix by acquiring a pg_advisory_xact_lock on a
bucket-derived key before counting to serialize bucket membership changes (call
e.g. pg_advisory_xact_lock(hashtext(bucket))) in the same transaction, then
perform systemStore.CountLedgersInBucket and SetAloneInBucket; if acquiring an
advisory lock is not feasible, add a clear comment next to ret.SetAloneInBucket
(and note in OpenLedger) describing the race and why the flag can be stale so
maintainers are aware.
| count, err := systemStore.CountLedgersInBucket(ctx, l.Bucket) | ||
| if err != nil { | ||
| return fmt.Errorf("counting ledgers in bucket: %w", err) | ||
| } | ||
| ret.SetAloneInBucket(count == 1) | ||
|
|
There was a problem hiding this comment.
TOCTOU: concurrent CreateLedger calls in the same bucket can both see count == 1.
With READ COMMITTED isolation, two concurrent CreateLedger transactions for the same bucket will each see only their own uncommitted insert, so both may set isAloneInBucket = true. The returned store instances will then skip the ledger filter, potentially exposing cross-ledger data for the lifetime of those store objects.
The previous dynamic sub-query was evaluated per-query (after commit), so it was always up to date. This new flag is a point-in-time snapshot that can go stale.
Consider either:
- Taking a
pg_advisory_xact_lockon the bucket name before counting, to serialize bucket membership changes. - Adding a comment (like in
OpenLedger) acknowledging the race, so future maintainers are aware.
At minimum, option 2:
Suggested comment
+ // WARNING: with concurrent CreateLedger calls in the same bucket,
+ // each transaction may see count==1 (READ COMMITTED). The caller
+ // should re-check via OpenLedger if the store is long-lived.
count, err := systemStore.CountLedgersInBucket(ctx, l.Bucket)📝 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.
| count, err := systemStore.CountLedgersInBucket(ctx, l.Bucket) | |
| if err != nil { | |
| return fmt.Errorf("counting ledgers in bucket: %w", err) | |
| } | |
| ret.SetAloneInBucket(count == 1) | |
| // WARNING: with concurrent CreateLedger calls in the same bucket, | |
| // each transaction may see count==1 (READ COMMITTED). The caller | |
| // should re-check via OpenLedger if the store is long-lived. | |
| count, err := systemStore.CountLedgersInBucket(ctx, l.Bucket) | |
| if err != nil { | |
| return fmt.Errorf("counting ledgers in bucket: %w", err) | |
| } | |
| ret.SetAloneInBucket(count == 1) |
🤖 Prompt for AI Agents
In `@internal/storage/driver/driver.go` around lines 80 - 85, The current
CreateLedger flow sets ret.SetAloneInBucket based on
systemStore.CountLedgersInBucket which can race under concurrent CreateLedger
calls (TOCTOU); fix by acquiring a pg_advisory_xact_lock on a bucket-derived key
before counting to serialize bucket membership changes (call e.g.
pg_advisory_xact_lock(hashtext(bucket))) in the same transaction, then perform
systemStore.CountLedgersInBucket and SetAloneInBucket; if acquiring an advisory
lock is not feasible, add a clear comment next to ret.SetAloneInBucket (and note
in OpenLedger) describing the race and why the flag can be stale so maintainers
are aware.
1 participant
Summary
isAloneInBuckethint set at store open/create timeCountLedgersInBucketto system store and wire it in driver create/open flowsValidation