storage: fix a bug with reverse scans, multiple column families, and max keys#92643
Conversation
erikgrinaker
left a comment
There was a problem hiding this comment.
Oops, sorry about that. Thanks for the fix!
| if e.hasArg("wholeRows") { | ||
| opts.WholeRowsOfSize = 10 // arbitrary, must be greater than largest column family in tests | ||
| for _, c := range e.td.CmdArgs { | ||
| if c.Key == "wholeRows" { |
There was a problem hiding this comment.
nit: maybe you tried this already, but can't this use e.scanArg() and fall back to 10 when set to 0?
Either way, the test doc comment should be updated with the new argument.
|
This change is |
yuzefovich
left a comment
There was a problem hiding this comment.
Reviewable status:
complete! 0 of 0 LGTMs obtained (waiting on @erikgrinaker)
pkg/storage/mvcc_history_test.go line 1486 at r1 (raw file):
Previously, erikgrinaker (Erik Grinaker) wrote…
nit: maybe you tried this already, but can't this use
e.scanArg()and fall back to 10 when set to 0?Either way, the test doc comment should be updated with the new argument.
e.scanArg() requires that the value is present, but I thought it would be annoying to add the value everywhere.
Where is the doc comment you're referring to? (I couldn't find it.)
sumeerbhola
left a comment
There was a problem hiding this comment.
pebbleResults change.
Reviewed 1 of 3 files at r1, all commit messages.
Reviewable status:
erikgrinaker
left a comment
There was a problem hiding this comment.
Reviewed 3 of 3 files at r1, all commit messages.
Reviewable status:
pkg/storage/mvcc_history_test.go line 1486 at r1 (raw file):
Previously, yuzefovich (Yahor Yuzefovich) wrote…
e.scanArg()requires that the value is present, but I thought it would be annoying to add the value everywhere.Where is the doc comment you're referring to? (I couldn't find it.)
Looks like wholeRows isn't documented at all. It should be listed here:
…max keys This commit fixes a bug with how we're checking whether the last row has final column family when performing a reverse scan. Previously, we'd incorrectly treat the largest column ID as the "last" one, but with the reverse iteration actually the zeroth column family is the "last" one. As a result, we could return an incomplete row to SQL. However, there is no production impact to this bug because no user at the SQL layer currently satisfies all the conditions: - `WholeRowsOfSize` option must be used. Currently, it is only used by the streamer; - the reverse scan must be requested and `MaxSpanRequestKeys` must be set - neither is currently done by the streamer. Release note: None
82237c6 to
1f7e094
Compare
yuzefovich
left a comment
There was a problem hiding this comment.
TFTRs!
bors r+
Reviewable status:
pkg/storage/mvcc_history_test.go line 1486 at r1 (raw file):
Previously, erikgrinaker (Erik Grinaker) wrote…
Looks like
wholeRowsisn't documented at all. It should be listed here:
Added.
|
Build succeeded: |
This commit fixes a bug with how we're checking whether the last row has final column family when performing a reverse scan. Previously, we'd incorrectly treat the largest column ID as the "last" one, but with the reverse iteration actually the zeroth column family is the "last" one. As a result, we could return an incomplete row to SQL.
However, there is no production impact to this bug because no user at the SQL layer currently satisfies all the conditions:
WholeRowsOfSizeoption must be used. Currently, it is only used by the streamer;MaxSpanRequestKeysmust be set - neither is currently done by the streamer.Epic: None
Release note: None