sql: scan only primary column for FK checks #42572
Conversation
|
This change is |
There was a problem hiding this comment.
Reviewed 3 of 3 files at r1.
Reviewable status:complete! 1 of 0 LGTMs obtained (waiting on @jordanlewis, @lucy-zhang, and @RaduBerinde)
pkg/sql/logictest/testdata/logic_test/fk, line 2725 at r1 (raw file):
query III UPDATE fam_parent SET b = b+1 WHERE k = 1 RETURNING k, a, b;
The RETURNING clause is just to place extra constraints on this, right? Mind adding a comment?
There was a problem hiding this comment.
Reviewable status:
pkg/sql/logictest/testdata/logic_test/fk, line 2725 at r1 (raw file):
Previously, nvanbenschoten (Nathan VanBenschoten) wrote…
The RETURNING clause is just to place extra constraints on this, right? Mind adding a comment?
Hm. I don't think it's needed, I just wanted to make sure it actually modifies something. I removed RETURNING and used statement count 1 which I had forgotten about.
There was a problem hiding this comment.
Reviewable status:
pkg/sql/logictest/testdata/logic_test/fk, line 13 at r2 (raw file):
id INT PRIMARY KEY, email STRING UNIQUE, FAMILY (id, email)
Ditto for the other tests, but why do you need to set the families on these? It seems like it would be good for the column family randomizer to run on these.
pkg/sql/row/fk_spans.go, line 28 at r2 (raw file):
f.searchTable.TableDesc(), f.searchIdx, f.prefixLen, f.ids, values, f.searchPrefix) if f.searchIdx.ID == f.searchTable.PrimaryIndex.ID && len(f.searchTable.Families) > 1 {
why do you need to check the the number of families is > 1?
|
It might also be a good idea to keep a list of column family optimizations that can be applied to secondary indexes as well once #42073 goes through |
There was a problem hiding this comment.
Reviewable status:
pkg/sql/logictest/testdata/logic_test/fk, line 13 at r2 (raw file):
Previously, rohany (Rohan Yadav) wrote…
Ditto for the other tests, but why do you need to set the families on these? It seems like it would be good for the column family randomizer to run on these.
Some of these tests use traces to see the exact keys we're scanning, so if there are multiple families we get a different key. Frankly I think this kind of tests belong in opt/exec/execbuilder but that's for another PR.
pkg/sql/row/fk_spans.go, line 28 at r2 (raw file):
Previously, rohany (Rohan Yadav) wrote…
why do you need to check the the number of families is > 1?
Not sure.. I took a cue from existing code which would never be exercised if we had a single family, eg
cockroach/pkg/sql/execinfra/joinreader.go
Line 358 in 845bd76
At the very least is saves an allocation or two in the single-family case. Maybe there is also some backward compatibility concern for pre-column-family tables? (CC @jordanlewis)
There was a problem hiding this comment.
Reviewable status:
pkg/sql/row/fk_spans.go, line 28 at r2 (raw file):
Previously, RaduBerinde wrote…
Not sure.. I took a cue from existing code which would never be exercised if we had a single family, eg
cockroach/pkg/sql/execinfra/joinreader.go
Line 358 in 845bd76
At the very least is saves an allocation or two in the single-family case. Maybe there is also some backward compatibility concern for pre-column-family tables? (CC @jordanlewis)
I don't think we need to worry about pre-cf tables. It's just about avoiding unnecessary work.
There was a problem hiding this comment.
Reviewable status:
pkg/sql/logictest/testdata/logic_test/fk, line 13 at r2 (raw file):
Previously, RaduBerinde wrote…
Some of these tests use traces to see the exact keys we're scanning, so if there are multiple families we get a different key. Frankly I think this kind of tests belong in
opt/exec/execbuilderbut that's for another PR.
Sounds good.
pkg/sql/row/fk_spans.go, line 28 at r2 (raw file):
Previously, jordanlewis (Jordan Lewis) wrote…
I don't think we need to worry about pre-cf tables. It's just about avoiding unnecessary work.
it seems like if we are always going to look at family 0, we don't need this check.
- If the table only has 1 family, then it will be family 0, which we can look up here.
- If the table has multiple families, then we want to just look at family 0.
The above code uses that check to avoid splitting a point lookup into multiple families if the table doesn't have more than one family.
There was a problem hiding this comment.
Reviewable status:
pkg/sql/row/fk_spans.go, line 28 at r2 (raw file):
Previously, rohany (Rohan Yadav) wrote…
it seems like if we are always going to look at family 0, we don't need this check.
- If the table only has 1 family, then it will be family 0, which we can look up here.
- If the table has multiple families, then we want to just look at family 0.
The above code uses that check to avoid splitting a point lookup into multiple families if the table doesn't have more than one family.
Updated, PTAL. Reverted the test family stuff because the spans now look the same in all cases.
This change improves the legacy FK path to only scan the primary column family. The new path already does this (as long as the best plan is a lookup join). Adding a test for both paths. Fixes cockroachdb#30852. Release note: None
There was a problem hiding this comment.
LGTM, but I don't know this part of the code.
Reviewed 3 of 3 files at r3.
Reviewable status:
|
bors r+ |
42572: sql: scan only primary column for FK checks r=RaduBerinde a=RaduBerinde This change improves the legacy FK path to only scan the primary column family. The new path already does this (as long as the best plan is a lookup join). Adding a test for both paths. Fixes #30852. Release note: None 42596: sql: remove local ordinality r=jordanlewis a=jordanlewis Now that it's got a distributed version, we can delete the local implementation. Release note: None Co-authored-by: Radu Berinde <[email protected]> Co-authored-by: Jordan Lewis <[email protected]>
Build succeeded |
This change improves the legacy FK path to only scan the primary
column family. The new path already does this (as long as the best plan is a
lookup join). Adding a test for both paths.
Fixes #30852.
Release note: None