distsql: copy prefix key in index skip table reader #39636
Conversation
|
This change is |
|
Also deleted some duplicate tests. |
There was a problem hiding this comment.
Thanks for cleaning up the tests alfonso!
Reviewable status:
complete! 0 of 0 LGTMs obtained (waiting on @asubiotto, @ridwanmsharif, and @rohany)
pkg/sql/distsqlrun/index_skip_table_reader.go, line 204 at r1 (raw file):
// If this is a forward scan, skip to the end of the current prefix's // keyspace on the next seek. t.spans[t.currentSpan].Key = prefixKey.PrefixEnd()
If we want to not modify the current key we should just make a copy here, rather than using prefixEnd. I'm not sure PrefixEnd guards against null values being in every other position in the index.
There was a problem hiding this comment.
Reviewable status:
pkg/sql/distsqlrun/index_skip_table_reader.go, line 204 at r1 (raw file):
Previously, rohany (Rohan Yadav) wrote…
If we want to not modify the current key we should just make a copy here, rather than using prefixEnd. I'm not sure PrefixEnd guards against null values being in every other position in the index.
Good point, done. We should probably have a test that checks this case. We should also maybe just increment currentSpan if the prefix key is maximal?
There was a problem hiding this comment.
The changes LGTM! Thanks for fixing this! I'll defer to @rohany for the ✅ cause I'm not too familiar with this area
There was a problem hiding this comment.
LGTM with a nit + question
Reviewable status:
pkg/sql/distsqlrun/index_skip_table_reader.go, line 204 at r1 (raw file):
Previously, asubiotto (Alfonso Subiotto Marqués) wrote…
Good point, done. We should probably have a test that checks this case. We should also maybe just increment
currentSpanif the prefix key is maximal?
I thought the interleaved tests should have tested this, or some of the null tests, but I guess not. I think a table with an index of 3 columns, where the last 2 columns only contain nulls should test this behavior. Can you open an issue about that?
Do you mean if the key is all 0xff? We could check that here, but it will get caught in the next case anyway, where we check that the span is valid.
pkg/sql/distsqlrun/index_skip_table_reader.go, line 203 at r2 (raw file):
if !t.reverse { // Modifying key in place would corrupt our current row, so make a copy. key = append([]byte(nil), key...)
[nit] would it be clearer to just use copy
There was a problem hiding this comment.
Reviewable status:
pkg/sql/distsqlrun/index_skip_table_reader.go, line 184 at r2 (raw file):
} key, err := t.fetcher.PartialKey(t.keyPrefixLen)
Should you move the copy up here? In the case where we are doing a reverse scan we are using the key from the fetcher which could be a problem.
This is necessary because the table reader goes on to modify the key returned from `PartialKey` in place, which would corrupt the current row to return since the key is unsafe. This commit also changes test descriptions to the more conventional TestName/AndSubtestName in the index skip table reader tests and shortens them to be able to run specific subtests more easily (less typing). Release note: None
There was a problem hiding this comment.
Reviewable status:
pkg/sql/distsqlrun/index_skip_table_reader.go, line 204 at r1 (raw file):
Previously, rohany (Rohan Yadav) wrote…
I thought the interleaved tests should have tested this, or some of the null tests, but I guess not. I think a table with an index of 3 columns, where the last 2 columns only contain nulls should test this behavior. Can you open an issue about that?
Do you mean if the key is all 0xff? We could check that here, but it will get caught in the next case anyway, where we check that the span is valid.
Yes (maximal key/all 0xff). Aren't there cases where a max key is still valid (e.g. if the EndKey is a longer max key, I'm thinking in terms of a variable prefix length which is what ridwan is working on)? That's what I was worried about, but I guess you're saying there's something else? Why would the rest of the index (after the prefix length) matter? I'll merge it without modifying this logic but we should revisit this. I added a TODO.
pkg/sql/distsqlrun/index_skip_table_reader.go, line 184 at r2 (raw file):
Previously, rohany (Rohan Yadav) wrote…
Should you move the copy up here? In the case where we are doing a reverse scan we are using the key from the fetcher which could be a problem.
The issue is that we're doing in-place modification of an unsafe key in the forward case, so it's only that that would corrupt the row. But I moved the copy up here and added a separate scope to disallow reuse of the key variable.
pkg/sql/distsqlrun/index_skip_table_reader.go, line 203 at r2 (raw file):
Previously, rohany (Rohan Yadav) wrote…
[nit] would it be clearer to just use
copy
True, in fact it's also more performant. I added a scratch variable though to avoid allocating on every lookup. I kept it as an append just to be sure we have enough capacity to copy the whole key (since it could have some extra bytes that we don't account for i.e. table/index ID).
There was a problem hiding this comment.
Reviewable status:
pkg/sql/distsqlrun/index_skip_table_reader.go, line 204 at r1 (raw file):
Previously, asubiotto (Alfonso Subiotto Marqués) wrote…
Yes (maximal key/all 0xff). Aren't there cases where a max key is still valid (e.g. if the
EndKeyis a longer max key, I'm thinking in terms of a variable prefix length which is what ridwan is working on)? That's what I was worried about, but I guess you're saying there's something else? Why would the rest of the index (after the prefix length) matter? I'll merge it without modifying this logic but we should revisit this. I added a TODO.
I think we guarantee here that maximal keys are invalid here (by doing t.indexLen - t.keyPrefixLen + 1), but I'm not sure. We can talk about this in person when I come back and take a look at the issues -- it is tricky.
pkg/sql/distsqlrun/index_skip_table_reader.go, line 184 at r2 (raw file):
Previously, asubiotto (Alfonso Subiotto Marqués) wrote…
The issue is that we're doing in-place modification of an unsafe key in the forward case, so it's only that that would corrupt the row. But I moved the copy up here and added a separate scope to disallow reuse of the
keyvariable.
that makes sense -- great.
|
Looks like a storage flake: #39651 bors r=rohany |
39636: distsql: use PrefixEnd in the index skip table reader r=rohany a=asubiotto This commit defers the construction of the next key to read to `PrefixEnd`, which does more or less the same thing as the current key construction code but also returns a new key for that prefix end. If we modify `key` in place and the needed output rows is greater than the prefix length, we overwrite the current row's values (since the key returned from `PartialKey` is unsafe. This commit also changes test descriptions to the more conventional TestName/AndSubtestName in the index skip table reader tests and shortens them to be able to run specific subtests more easily (less typing). Release note: None Co-authored-by: Alfonso Subiotto Marqués <alfonso@cockroachlabs.com>
asubiotto
changed the title
Build succeeded |
This is necessary because the table reader goes on to modify the key
returned from
PartialKeyin place, which would corrupt the current rowto return since the key is unsafe.
This commit also changes test descriptions to the more conventional
TestName/AndSubtestName in the index skip table reader tests and
shortens them to be able to run specific subtests more easily (less
typing).
Release note: None