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: unsafe key mangling for MVCCIterator #60499

Merged
merged 1 commit into from
Feb 18, 2021

Conversation

sumeerbhola
Copy link
Collaborator

@sumeerbhola sumeerbhola commented Feb 11, 2021

It can uncover bugs in code that misuses unsafe
keys. It uncovered the bug fixed in
https://github.com/cockroachdb/cockroach/pull/60460/files#diff-84108c53fd1e766ef8802b87b394981d3497d87c40d86e084f2ed77bba0ca71a

Release note: None

@cockroach-teamcity
Copy link
Member

This change is Reviewable

@sumeerbhola sumeerbhola force-pushed the unstable_raw_key branch 4 times, most recently from 8233987 to 797aeba Compare February 12, 2021 19:14
@sumeerbhola sumeerbhola requested a review from a team February 12, 2021 19:14
@sumeerbhola sumeerbhola requested a review from a team as a code owner February 12, 2021 19:14
@sumeerbhola sumeerbhola requested review from pbardea and removed request for a team February 12, 2021 19:14
@pbardea pbardea removed their request for review February 12, 2021 19:27
Copy link
Contributor

@knz knz left a comment

Choose a reason for hiding this comment

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

Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (waiting on @sumeerbhola)


pkg/storage/intent_interleaving_iter.go, line 902 at r3 (raw file):

	if rand.Intn(2) == 0 {
		for _, b := range [3][]byte{i.keyBuf, i.rawKeyBuf, i.rawMVCCKeyBuf} {
			// for _, b := range [3][]byte{i.keyBuf, i.rawKeyBuf, i.rawMVCCKeyBuf} {

don't forget to remove this before it merges

@sumeerbhola sumeerbhola changed the title storage: unsafe key mangling for MVCCIterator [DNM] storage: unsafe key mangling for MVCCIterator Feb 15, 2021
@sumeerbhola
Copy link
Collaborator Author

I forgot to mark this Do Not Merge. I've done that now. I had not added reviewers since this was an experiment for which I needed a full set of CI test runs -- is there is a simpler way to do that without creating a PR?

@knz
Copy link
Contributor

knz commented Feb 15, 2021

Creating a PR is fine but github gives you the option to mark it as "draft" now so perhaps you could use that.

@sumeerbhola sumeerbhola force-pushed the unstable_raw_key branch 3 times, most recently from 6387e8f to cbbaddd Compare February 18, 2021 18:23
@sumeerbhola sumeerbhola changed the title [DNM] storage: unsafe key mangling for MVCCIterator storage: unsafe key mangling for MVCCIterator Feb 18, 2021
Copy link
Collaborator Author

@sumeerbhola sumeerbhola left a comment

Choose a reason for hiding this comment

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

This is ready for a look. Reviewable is confused because of the history of this PR and shows too many files -- if one selects "combine commits for review", it will look sane.

Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (waiting on @itsbilal, @nvanbenschoten, @petermattis, and @sumeerbhola)

Copy link
Member

@nvanbenschoten nvanbenschoten left a comment

Choose a reason for hiding this comment

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

:lgtm: cool idea! Does this have a noticeable impact on racetest duration?

Reviewed 1 of 4 files at r3, 3 of 18 files at r4.
Reviewable status: :shipit: complete! 1 of 0 LGTMs obtained (waiting on @itsbilal, @petermattis, and @sumeerbhola)

Copy link
Collaborator Author

@sumeerbhola sumeerbhola left a comment

Choose a reason for hiding this comment

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

TFTR!

Does this have a noticeable impact on racetest duration?

The racetests run for this PR look fast enough, with the slowest taking 7s
https://teamcity.cockroachdb.com/viewLog.html?buildId=2691122&buildTypeId=Cockroach_UnitTests_Testrace&tab=testsInfo

Reviewable status: :shipit: complete! 1 of 0 LGTMs obtained (waiting on @itsbilal, @knz, and @petermattis)


pkg/storage/intent_interleaving_iter.go, line 902 at r3 (raw file):

Previously, knz (kena) wrote…

don't forget to remove this before it merges

Done.

@sumeerbhola
Copy link
Collaborator Author

bors r+

@craig
Copy link
Contributor

craig bot commented Feb 18, 2021

Build failed (retrying...):

@craig craig bot merged commit 40a35fe into cockroachdb:master Feb 18, 2021
@craig
Copy link
Contributor

craig bot commented Feb 18, 2021

Build succeeded:

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.

4 participants