-
Notifications
You must be signed in to change notification settings - Fork 1.7k
kvdb-rocksdb: remove buffered operations when committing transaction #7950
Conversation
@@ -491,6 +491,9 @@ impl Database { | |||
let batch = WriteBatch::new(); | |||
let ops = tr.ops; | |||
for op in ops { | |||
// remove any buffered operation for this key | |||
self.overlay.write()[Self::to_overlay_column(op.col())].remove(op.key()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure if I should take a read lock first to check if there's any entry, instead of always locking for write.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No sure either; is there any reliable way to just benchmark this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe, we always lock for write
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Other than that possible lock improvement, LGTM.
@@ -491,6 +491,9 @@ impl Database { | |||
let batch = WriteBatch::new(); | |||
let ops = tr.ops; | |||
for op in ops { | |||
// remove any buffered operation for this key | |||
self.overlay.write()[Self::to_overlay_column(op.col())].remove(op.key()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No sure either; is there any reliable way to just benchmark this?
When committing a transaction any keys associated with the transaction are dropped from the overlay.
The test shows the expected semantics for buffered operations.