storage: QueryTxn declares the wrong key #23944
Assignees
Labels
C-bug
Code not up to spec/doc, specs & docs deemed correct. Solution expected to change code/behavior.
Milestone
Comments
nvanbenschoten
added
the
C-bug
nvanbenschoten
added a commit
to nvanbenschoten/cockroach
that referenced
this issue
Mar 17, 2018
Fixes cockroachdb#23944. This change corrects QueryTxn so that it now correctly declares the key that it reads from. This is important because the previous incorrect declaration allowed for improper CommandQueue synchronization with other requests that touch the transaction record. There were two approaches we could take here: - Map the requests header.Key to the correct key - Append the txnID to header.Key This change goes with the former approach because it does not have any compatibility concerns and because it is consistent with what we do for PushTxn requests. Release note: None
nvanbenschoten
added a commit
to nvanbenschoten/cockroach
that referenced
this issue
Mar 17, 2018
Before this change, we only performed SpanSet assertions on read-write requests. This was a pretty huge oversight and is what allowed cockroachdb#23944 to slip in. This change fixes this by adding a `engine.ReadWriter` spanset variant and using it for read-only requests while in race mode. It is currently blocked on cockroachdb#23996 and the revival of cockroachdb#18579. Release note: None
nvanbenschoten
added a commit
to nvanbenschoten/cockroach
that referenced
this issue
Mar 19, 2018
Fixes cockroachdb#23944. This change corrects QueryTxn so that it now correctly declares the key that it reads from. This is important because the previous incorrect declaration allowed for improper CommandQueue synchronization with other requests that touch the transaction record. There were two approaches we could take here: - Map the requests header.Key to the correct key - Append the txnID to header.Key This change goes with the former approach because it does not have any compatibility concerns and because it is consistent with what we do for PushTxn requests. Release note: None
nvanbenschoten
added a commit
to nvanbenschoten/cockroach
that referenced
this issue
Mar 19, 2018
Before this change, we only performed SpanSet assertions on read-write requests. This was a pretty huge oversight and is what allowed cockroachdb#23944 to slip in. This change fixes this by adding a `engine.ReadWriter` spanset variant and using it for read-only requests while in race mode. It is currently blocked on cockroachdb#23996 and the revival of cockroachdb#18579. Release note: None
nvanbenschoten
added a commit
to nvanbenschoten/cockroach
that referenced
this issue
Mar 19, 2018
Before this change, we only performed SpanSet assertions on read-write requests. This was a pretty huge oversight and is what allowed cockroachdb#23944 to slip in. This change fixes this by adding a `engine.ReadWriter` spanset variant and using it for read-only requests while in race mode. It is currently blocked on cockroachdb#23996 and the revival of cockroachdb#18579. Release note: None
craig bot
added a commit
that referenced
this issue
Apr 5, 2018
24046: backport-2.0: storage: perform SpanSet assertions on read-only requests r=nvanbenschoten a=nvanbenschoten Backport 1/1 commits from #23997. /cc @cockroachdb/release --- Before this change, we only performed SpanSet assertions on read-write requests. This was a pretty huge oversight and is what allowed #23944 to slip in. This change fixes this by adding an `engine.ReadWriter` spanset variant and using it for read-only requests while in race mode. It is currently blocked on #23996 and the revival of #18579. This is actually good news, as it means that no other spanset violations have crept in while this assertion was missing. First commit is from #23996. Release note: None
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Currently,
QueryTxnrequests are directed to the corresponding transaction's anchor key. However, this key is not suffixed with the transaction ID. This means that whenQueryTxnusesDefaultDeclareKeysto declare its keys for theCommandQueue, it declares the anchor key itself instead of the TransactionKey (AnchorKey + TxnID). This is an issue because it may result in improperCommandQueuesynchronization with other requests that touch the transaction record.One thing that's confusing me is how this wasn't caught by the spanset assertions.
The text was updated successfully, but these errors were encountered: