Skip to content

kv: compare MVCC GC threshold against Refresh{Range}Request.RefreshFrom#75624

Merged
craig[bot] merged 1 commit intocockroachdb:masterfrom
nvb:nvanbenschoten/refreshAfterGC
Jan 29, 2022
Merged

kv: compare MVCC GC threshold against Refresh{Range}Request.RefreshFrom#75624
craig[bot] merged 1 commit intocockroachdb:masterfrom
nvb:nvanbenschoten/refreshAfterGC

Conversation

@nvb
Copy link
Copy Markdown
Contributor

@nvb nvb commented Jan 27, 2022

Noticed by Sumeer in #74628.

A Refresh request needs to observe all MVCC versions between its
exclusive RefreshFrom time and its inclusive RefreshTo time. If it were
to permit MVCC GC between these times then it could miss conflicts that
should cause the refresh to fail. This could in turn lead to violations
of serializability. For example:

txn1 reads value k1@10
txn2 deletes (tombstones) k1@15
mvcc gc @ 20 clears versions k1@10 and k1@15
txn1 refreshes @ 25, sees no value between (10, 25], refresh successful

In the example, the refresh erroneously succeeds because the request is
permitted to evaluate after part of the MVCC history it needs to read
has been GCed. By considering the RefreshFrom time to be the earliest
active timestamp of the request, we avoid this hazard. Instead of being
allowed to evaluate, the refresh request in the example would have hit
a BatchTimestampBeforeGCError.

@nvb nvb requested review from a team as code owners January 27, 2022 18:32
@cockroach-teamcity
Copy link
Copy Markdown
Member

This change is Reviewable

Copy link
Copy Markdown
Collaborator

@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.

:lgtm:

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

Noticed by Sumeer in cockroachdb#74628.

A Refresh request needs to observe all MVCC versions between its
exclusive RefreshFrom time and its inclusive RefreshTo time. If it were
to permit MVCC GC between these times then it could miss conflicts that
should cause the refresh to fail. This could in turn lead to violations
of serializability. For example:

```
txn1 reads value k1@10
txn2 deletes (tombstones) k1@15
mvcc gc @ 20 clears versions k1@10 and k1@15
txn1 refreshes @ 25, sees no value between (10, 25], refresh successful
```

In the example, the refresh erroneously succeeds because the request is
permitted to evaluate after part of the MVCC history it needs to read
has been GCed. By considering the RefreshFrom time to be the earliest
active timestamp of the request, we avoid this hazard. Instead of being
allowed to evaluate, the refresh request in the example would have hit
a BatchTimestampBeforeGCError.
@nvb nvb force-pushed the nvanbenschoten/refreshAfterGC branch from 1073ff1 to 4d041e2 Compare January 29, 2022 16:16
@nvb
Copy link
Copy Markdown
Contributor Author

nvb commented Jan 29, 2022

bors r+

@craig
Copy link
Copy Markdown
Contributor

craig bot commented Jan 29, 2022

Build succeeded:

@craig craig bot merged commit 1e96583 into cockroachdb:master Jan 29, 2022
@nvb nvb deleted the nvanbenschoten/refreshAfterGC branch January 31, 2022 18:33
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