fix(pebble.go): implement flush threshold for Pebble batch operations #275
Conversation
Added a flush threshold to prevent Pebble batch from exceeding 4 GB limit during write operations.
StrathCole
changed the title
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
This PR is being reviewed by Cursor Bugbot
Details
Your team is on the Bugbot Free tier. On this plan, Bugbot will review limited PRs each billing cycle for each member of your team.
To receive Bugbot reviews on all of your PRs, visit the Cursor dashboard to activate Pro and start your 14-day free trial.
|
|
||
| // Prevent Pebble batch from exceeding 4 GB hard limit | ||
| if b.batch.Len() > flushThreshold { | ||
| if err := b.batch.Commit(pebble.Sync); err != nil { |
There was a problem hiding this comment.
Bug: Batch Flush Sync Inconsistency
The automatic batch flush uses pebble.Sync, which is inconsistent with the pebble.NoSync used by the Write() method. This introduces unexpected synchronous writes during batch building, potentially causing performance slowdowns and violating user expectations.
There was a problem hiding this comment.
I would think this is necessary due to the location the commit is called, as it "interrupts" a normal batch operation, this should not simply continue until the batch is committed.
| } | ||
| b.batch.Reset() | ||
| } | ||
|
|
There was a problem hiding this comment.
Bug: Batch Size Check Timing Issue
The batch size check in pebbleDBBatch.Set and Delete happens before adding the operation. This means a large operation can push the batch over Pebble's 4GB hard limit, even if the current size is below the flushThreshold.
Additional Locations (1)
There was a problem hiding this comment.
In my opinion this would be unlikely to happen. Single operations normally are much smaller, just batching them can lead to the issue.
1 participant
Added a flush threshold to prevent Pebble batch from exceeding 4 GB limit during write operations.
Cometbft-db just executes set and delete on the batches without checking for the size. Pebble has a hard-coded limit of batch sizes to 4GB
This can lead to node panic in case there is massive data compaction/migrations.
PR checklist
.changelog(we use unclog to manage our changelog)