Skip to content
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: respect new Pebble iterator visibility #81662

Merged
merged 1 commit into from
May 24, 2022

Conversation

erikgrinaker
Copy link
Contributor

@erikgrinaker erikgrinaker commented May 23, 2022

Pebble recently tightened its iterator semantics such that it has a
static view of the underlying reader as of the time of its creation.
Previously, a batch iterator could see batch writes that occurred after
the iterator was created.

This patch prepares CRDB for this change.

Touches cockroachdb/pebble#1640.

Release note: None

@erikgrinaker erikgrinaker requested a review from jbowens May 23, 2022 15:26
@erikgrinaker erikgrinaker self-assigned this May 23, 2022
@cockroach-teamcity
Copy link
Member

This change is Reviewable

@erikgrinaker erikgrinaker force-pushed the mvcc-iterator-visibility branch from c06a99d to 4c35637 Compare May 23, 2022 15:33
Pebble recently tightened its iterator semantics such that it has a
static view of the underlying reader as of the time of its creation.
Previously, a batch iterator could see batch writes that occurred after
the iterator was created.

This patch prepares CRDB for this change.

Release note: None
@erikgrinaker erikgrinaker force-pushed the mvcc-iterator-visibility branch from 4c35637 to 804d06c Compare May 23, 2022 17:27
@erikgrinaker
Copy link
Contributor Author

erikgrinaker commented May 23, 2022

Not seeing any further test failures in CI when running this with Pebble at cockroachdb/pebble@3355a02e7cec.

I had a closer look at this by dumping a stack trace in pebbleBatch whenever the batch is mutated while pebbleBatch.*Iter.inuse is set, and filtered out known-good callers such as mvccPutUsingIter. Didn't find any suspect uses with a 3-node cluster running e.g. YCSB/B.

I think this is about as good as we can do for now. We can try to catch any issues here via improved MVCC metamorphic tests or kvnemesis tests.

@erikgrinaker erikgrinaker marked this pull request as ready for review May 23, 2022 17:28
@erikgrinaker erikgrinaker requested a review from a team as a code owner May 23, 2022 17:28
Copy link
Collaborator

@jbowens jbowens left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:lgtm_strong:

Reviewed 1 of 1 files at r1, all commit messages.
Reviewable status: :shipit: complete! 1 of 0 LGTMs obtained (waiting on @erikgrinaker)

@erikgrinaker
Copy link
Contributor Author

bors r=jbowens

@craig
Copy link
Contributor

craig bot commented May 24, 2022

Build succeeded:

@craig craig bot merged commit 7b094aa into cockroachdb:master May 24, 2022
@erikgrinaker erikgrinaker deleted the mvcc-iterator-visibility branch May 25, 2022 17:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants