New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
storage/engine: invalidate cached iterator state when necessary #28794
Conversation
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.
Assume you're planning to backport this?
Reviewed 2 of 2 files at r1.
Reviewable status: complete! 1 of 0 LGTMs obtained
pkg/storage/engine/mvcc_test.go, line 1323 at r1 (raw file):
} // Perform a Get or Scan.
nit: "or FindSplitKey or Compute Stats". Maybe rephrase to "perform the operation"? Or maybe just drop the comment entirely.
`Iterator.{MVCCGet,MVCCScan,FindSplitKey,ComputeStats}` need to invalidate the cached iterator state. They were previously failing to do so which could lead to rare scenarios where a key could be considered present in the database which had never been written. Fixes cockroachdb#28025 Release note (bug fix): Fix rare scenario where the value written for one system key was seen when another system key was read leading to the violation of internal invariants.
f69e21e
to
b1aaee4
Compare
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.
Yes, I'm planning on backporting this.
Reviewable status: complete! 1 of 0 LGTMs obtained
pkg/storage/engine/mvcc_test.go, line 1323 at r1 (raw file):
Previously, benesch (Nikhil Benesch) wrote…
nit: "or FindSplitKey or Compute Stats". Maybe rephrase to "perform the operation"? Or maybe just drop the comment entirely.
Removed. I was diligently commenting while developing and then extended the test.
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.
Reviewed 1 of 1 files at r2.
Reviewable status: complete! 1 of 0 LGTMs obtained
bors r=benesch |
28794: storage/engine: invalidate cached iterator state when necessary r=benesch a=petermattis `Iterator.{MVCCGet,MVCCScan,FindSplitKey,ComputeStats}` need to invalidate the cached iterator state. They were previously failing to do so which could lead to rare scenarios where a key could be considered present in the database which had never been written. Fixes #28025 Release note (bug fix): Fix rare scenario where the value written for one system key was seen when another system key was read leading to the violation of internal invariants. Co-authored-by: Peter Mattis <petermattis@gmail.com>
Can't review now and don't wait for me, just wanted to mention that you're
linking the wrong issue.
On Sat, Aug 18, 2018, 9:43 PM Peter Mattis ***@***.***> wrote:
bors r=benesch
—
You are receiving this because your review was requested.
Reply to this email directly, view it on GitHub
<#28794 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AE135K8Cw_YWYgAO75hS8QASIK3TbkJLks5uSG5LgaJpZM4WCqgq>
.
--
…-- Tobias
|
Build succeeded |
Gah! Too late! |
28798: release-2.0: storage/engine: invalidate cached iterator state when necessary r=benesch a=petermattis Backport 1/1 commits from #28794. /cc @cockroachdb/release --- `Iterator.{MVCCGet,MVCCScan,FindSplitKey,ComputeStats}` need to invalidate the cached iterator state. They were previously failing to do so which could lead to rare scenarios where a key could be considered present in the database which had never been written. Fixes #28025 Release note (bug fix): Fix rare scenario where the value written for one system key was seen when another system key was read leading to the violation of internal invariants. Co-authored-by: Peter Mattis <petermattis@gmail.com>
Well color me confused. Could have sworn I clicked on that issue link when
I reviewed this.
…On Sat, Aug 18, 2018 at 4:10 PM Peter Mattis ***@***.***> wrote:
Can't review now and don't wait for me, just wanted to mention that you're
linking the wrong issue.
Gah! Too late!
—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
<#28794 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AA15IAtYo4F4SNQT_y2UUgD_UQLDDFBOks5uSHTMgaJpZM4WCqgq>
.
|
Iterator.{MVCCGet,MVCCScan,FindSplitKey,ComputeStats}
need toinvalidate the cached iterator state. They were previously failing to do
so which could lead to rare scenarios where a key could be considered
present in the database which had never been written.
Fixes #28025
Release note (bug fix): Fix rare scenario where the value written for
one system key was seen when another system key was read leading to the
violation of internal invariants.