kv,storage,sql: purge snapshot isolation#32860
Conversation
|
This change is |
8da2fd0 to
d94f47c
Compare
nvb
left a comment
There was a problem hiding this comment.
Mostly 
RetryOnPush during EndTxn evaluation).
It was a pretty easy review because all you're doing is deleting code. Definitely get others to weigh in as well, as I'm sure we all have a few special-cases we remember adding to deal with SNAPSHOT isolation that may not have been picked up here.
Out of curiosity, did you look at every single use of enginepb.SERIALIZABLE
Reviewed 33 of 33 files at r1.
Reviewable status:complete! 1 of 0 LGTMs obtained
pkg/roachpb/data.go, line 732 at r1 (raw file):
baseKey Key, userPriority UserPriority, isolation enginepb.IsolationType,
Should we remove this argument?
pkg/roachpb/data.proto, line 384 at r1 (raw file):
// prevent the LostDeleteRange anomaly. This flag is relevant only // for SNAPSHOT transactions. bool retry_on_push = 13;
reserved
pkg/storage/batcheval/cmd_end_transaction.go, line 390 at r1 (raw file):
// timestamp. if isTxnPushed { if txn.Isolation == enginepb.SERIALIZABLE {
Do we want to assume that all transactions are SERIALIZABLE now? Is the plan to remove conditions like this?
bdarnell
left a comment
There was a problem hiding this comment.
🎉
Reviewable status:
pkg/kv/txn_correctness_test.go, line 1034 at r1 (raw file):
} // TestTxnDBWriteSkewAnomaly verifies that SI suffers from the write
This comment needs an update.
pkg/storage/batcheval/cmd_end_transaction.go, line 390 at r1 (raw file):
Previously, nvanbenschoten (Nathan VanBenschoten) wrote…
Do we want to assume that all transactions are
SERIALIZABLEnow? Is the plan to remove conditions like this?
Yes, I think so.
andreimatei
left a comment
There was a problem hiding this comment.
🎉 LGTM
Reviewable status:
pkg/roachpb/data.proto, line 384 at r1 (raw file):
Previously, nvanbenschoten (Nathan VanBenschoten) wrote…
reserved
so glad to see this one gone
pkg/roachpb/errors.proto, line 203 at r1 (raw file):
// A concurrent writer finished first, causing restart. RETRY_WRITE_TOO_OLD = 1; reserved 2;
I think we usually put these at the top ?
pkg/sql/conn_executor.go, line 1681 at r1 (raw file):
} } if modes.Isolation != tree.UnspecifiedIsolation && modes.Isolation != tree.SerializableIsolation {
can't tree.SerializableIsolation go away? (and maybe the whole enum)
pkg/storage/engine/enginepb/mvcc3.proto, line 23 at r1 (raw file):
// TODO(tschottdorf): Should not live in enginepb (but can't live in roachpb // either). enum IsolationType {
can't this go away?
Snapshot isolation was disabled in v2.1, so it is safe to remove all code paths that handle snapshot isolation in v2.2. Note that the change to the MVCCMetadata checksum in pkg/storage/below_raft_protos_test.go is safe, because the actual encoding of the protobuf is not affected by this change. What is affected is NewPopulatedMVCCMetadata, which no longer returns a TxnMeta with a non-zero IsolationType, since the IsolationType field has been removed. This is a necessary precursor to cockroachdb#32487. Release note: None
benesch
left a comment
There was a problem hiding this comment.
Reviewable status:
pkg/kv/txn_correctness_test.go, line 1034 at r1 (raw file):
Previously, bdarnell (Ben Darnell) wrote…
This comment needs an update.
Done.
pkg/roachpb/data.go, line 732 at r1 (raw file):
Previously, nvanbenschoten (Nathan VanBenschoten) wrote…
Should we remove this argument?
Done.
pkg/roachpb/data.proto, line 384 at r1 (raw file):
Previously, andreimatei (Andrei Matei) wrote…
so glad to see this one gone
Yikes, good catch, Nathan. Done.
pkg/roachpb/errors.proto, line 203 at r1 (raw file):
Previously, andreimatei (Andrei Matei) wrote…
I think we usually put these at the top ?
We do it both ways, it appears. It seems leaving them inline is slightly more common than putting them at the top, so I'm going to leave as is, though I'll note that I didn't do the most thorough check.
pkg/sql/conn_executor.go, line 1681 at r1 (raw file):
Previously, andreimatei (Andrei Matei) wrote…
can't
tree.SerializableIsolationgo away? (and maybe the whole enum)
In theory, but it changes the roundtripping of SET TRANSACTION ISOLATION LEVEL statements, so I'd like to leave it to another PR where it can be properly bikeshedded.
pkg/storage/batcheval/cmd_end_transaction.go, line 390 at r1 (raw file):
Previously, bdarnell (Ben Darnell) wrote…
Yes, I think so.
Done, here and elsewhere.
pkg/storage/engine/enginepb/mvcc3.proto, line 23 at r1 (raw file):
Previously, andreimatei (Andrei Matei) wrote…
can't this go away?
Yep, done.
nvb
left a comment
There was a problem hiding this comment.
Reviewed 49 of 49 files at r2.
Reviewable status:
pkg/roachpb/data.go, line 732 at r1 (raw file):
Previously, benesch (Nikhil Benesch) wrote…
Done.
🙏
|
bors r=andreimatei,nvanbenschoten,bdarnell |
32860: kv,storage,sql: purge snapshot isolation r=andreimatei,nvanbenschoten,bdarnell a=benesch This touches a lot so I cast a wide net for reviewers. Feel free to remove yourself! --- Snapshot isolation was disabled in v2.1, so it is safe to remove all code paths that handle snapshot isolation in v2.2. This is a necessary precursor to #32487. Release note: None Co-authored-by: Nikhil Benesch <nikhil.benesch@gmail.com>
Build succeeded |
andreimatei
left a comment
There was a problem hiding this comment.
Reviewable status:
pkg/sql/conn_executor.go, line 1681 at r1 (raw file):
Previously, benesch (Nikhil Benesch) wrote…
In theory, but it changes the roundtripping of
SET TRANSACTION ISOLATION LEVELstatements, so I'd like to leave it to another PR where it can be properly bikeshedded.
Do other isolation levels round trip? If not, I say burn it.
This commit adds an isolation level field to TxnMeta. The field is currently unused, but will be soon. Note that we re-use the reserved protobuf field number that previous transaction isolation field used back before it was removed by cockroachdb#32860. This is safe because the enum's values are the same. Release note: None
This touches a lot so I cast a wide net for reviewers. Feel free to remove yourself!
Snapshot isolation was disabled in v2.1, so it is safe to remove all
code paths that handle snapshot isolation in v2.2.
This is a necessary precursor to #32487.
Release note: None