kvcoord: track a write buffer transformation for all requests #146112
Conversation
…for scans This commit expands the `TestTxnBufferedWriteReadYourOwnWrites` test with Scan requests. As part of that, it repros the issue described in cockroachdb#146103. Informs: cockroachdb#146103 Release note: None
|
This change is |
miraradeva
force-pushed
the
mira-146103-scans
branch
from
460a72d to
a5d1090
Compare
There was a problem hiding this comment.
Nice find!
Should we backport this? SQL will never construct a batch like this, but it might still be good to fix it on 25.2.
Reviewed 1 of 1 files at r1, 3 of 3 files at r2, all commit messages.
Reviewable status:complete! 1 of 0 LGTMs obtained (waiting on @arulajmani and @stevendanna)
pkg/kv/kvclient/kvcoord/txn_interceptor_write_buffer.go line 675 at r2 (raw file):
baRemote.Requests = nil var ts transformations
nit: we can precisely allocate this slice now.
pkg/kv/kvclient/kvcoord/txn_test.go line 45 at r1 (raw file):
) func checkResults(t *testing.T, expected map[string][]byte, results ...kv.Result) {
nit: should results still be a variadic argument? Looks like it's always a single kv.Result now. Might be even worth inlining since it's only called in a single spot now.
Previously, the transaction write buffer kept a transformation for many requests: truly transformed requests (e.g. CPut), and other requests, like Get, which were not actually transformed but needed to be tracked in order to correctly be able to read your own writes. Scan requests, specifically, were not tracked as transformations, but should be, just like Gets (cockroachdb#146103). This commit tracks all requests as transformations regardless of whether they were actually transformed. For most requests, this is needed for correctness; for requests like QueryLocksRequest and LeaseInfoRequest, it is not necessary, but it is harmless and simplifies the code. Fixes: cockroachdb#146103 Release note: None
miraradeva
force-pushed
the
mira-146103-scans
branch
from
a5d1090 to
98907b6
Compare
|
TFTR!
Yes, I think we should backport it given that it's a legit bug.
Done.
Done. I added a third commit on top to rename the |
There was a problem hiding this comment.
Thanks!
Reviewed 2 of 2 files at r3, 1 of 1 files at r4, all commit messages.
Reviewable status:
pkg/kv/kvclient/kvcoord/txn_interceptor_write_buffer.go line 647 at r4 (raw file):
// Some examples of transformations include: // // 1. Blind writes (Put/Delete requests) are buffered locally. When they the
nit: s/they the/the/.
pkg/kv/kvclient/kvcoord/txn_interceptor_write_buffer.go line 1080 at r4 (raw file):
// toResp returns the response that should be added to the batch response as // a result of applying the requestRecord. func (t requestRecord) toResp(
nit: s/t/r/ or s/t/rr/.
miraradeva
force-pushed
the
mira-146103-scans
branch
from
98907b6 to
494c9be
Compare
miraradeva
force-pushed
the
mira-146103-scans
branch
from
494c9be to
7d60dba
Compare
| // stripped, if true, indicates that the request was stripped from the batch | ||
| // and never sent to the KV layer. | ||
| stripped bool | ||
| // index of the request in the original batch to which the transformation | ||
| // transformed, if true, indicates that the request was transformed into a | ||
| // different request to be sent to the KV layer. |
There was a problem hiding this comment.
Should we specify that this is only relevant if stripped is false?
There was a problem hiding this comment.
Yes, I guess, though it depends on how we plan to use this new transformed flag.
Should a blind Put, which is stripped, set transformed = true? Currently, I set transformed = true for all CPut, Put, Del. But if we only set it when stripped == false, then we can ensure that stripped == true implies transformed == false. Maybe that's a useful invariant.
There was a problem hiding this comment.
I made the change ^^, and added a comment about it. I don't think it makes sense for a stripped Put to be considered transformed. @stevendanna let me know if that makes sense to you.
There was a problem hiding this comment.
I like it. At the very least it is some trail that will remind us of our intention if we get confused later on.
Now that we keep tranck of all requests as transformations, the term "transformation" doesn't make sense because some of the requests aren't actually transformed. This commit renames all transformation-related terms and replaces "transformation" with "requestRecord", while also adding a new `transformed` boolean field in `requestRecord` to indicate if a request was actually transformed. Part of: cockroachdb#146103 Release note: None
miraradeva
force-pushed
the
mira-146103-scans
branch
from
7d60dba to
bb59d45
Compare
|
bors r=yuzefovich,stevendanna |
Previously, the transaction write buffer kept a transformation for many requests: truly transformed requests (e.g. CPut), and other requests, like Get, which were not actually transformed but needed to be tracked in order to correctly be able to read your own writes. Scan requests, specifically, were not tracked as transformations, but should be, just like Gets (#146103).
This commit tracks all requests as transformations regardless of whether they were actually transformed. For most requests, this is needed for correctness; for requests like QueryLocksRequest and LeaseInfoRequest, it is not necessary, but it is harmless and simplifies the code.
Fixes: #146103
Release note: None