sql: scan only primary column for FK checks by RaduBerinde · Pull Request #42572 · cockroachdb/cockroach

RaduBerinde · 2019-11-19T17:51:48Z

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

cockroach-teamcity · 2019-11-19T17:51:55Z

This change is

nvb

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?

RaduBerinde

Reviewable status: complete! 0 of 0 LGTMs obtained (and 1 stale) (waiting on @jordanlewis, @lucy-zhang, and @nvanbenschoten)

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.

rohany

Reviewable status: complete! 0 of 0 LGTMs obtained (and 1 stale) (waiting on @jordanlewis, @lucy-zhang, @nvanbenschoten, and @RaduBerinde)

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?

rohany · 2019-11-19T23:39:07Z

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

RaduBerinde

Reviewable status: complete! 0 of 0 LGTMs obtained (and 1 stale) (waiting on @jordanlewis, @lucy-zhang, @nvanbenschoten, and @rohany)

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

if len(jr.neededFamilies) > 0 &&

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)

jordanlewis

Reviewable status: complete! 0 of 0 LGTMs obtained (and 1 stale) (waiting on @lucy-zhang, @nvanbenschoten, and @rohany)

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

if len(jr.neededFamilies) > 0 &&

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.

rohany

Reviewable status: complete! 0 of 0 LGTMs obtained (and 1 stale) (waiting on @jordanlewis, @lucy-zhang, @nvanbenschoten, and @RaduBerinde)

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/execbuilder but 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.

RaduBerinde

Reviewable status: complete! 0 of 0 LGTMs obtained (and 1 stale) (waiting on @lucy-zhang, @nvanbenschoten, @RaduBerinde, and @rohany)

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

thoszhang

LGTM, but I don't know this part of the code.

Reviewed 3 of 3 files at r3.
Reviewable status: complete! 0 of 0 LGTMs obtained (and 1 stale) (waiting on @nvanbenschoten, @RaduBerinde, and @rohany)

rohany

LGTM

RaduBerinde · 2019-11-20T19:25:24Z

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 <radu@cockroachlabs.com> Co-authored-by: Jordan Lewis <jordanthelewis@gmail.com>

craig · 2019-11-20T19:58:51Z

Build succeeded

GitHub CI (Cockroach)

RaduBerinde requested review from jordanlewis and nvb November 19, 2019 17:51

RaduBerinde requested a review from thoszhang November 19, 2019 17:53

RaduBerinde force-pushed the fk-fam branch from 9cad16c to b41d142 Compare November 19, 2019 18:29

nvb approved these changes Nov 19, 2019

View reviewed changes

RaduBerinde force-pushed the fk-fam branch from b41d142 to 8e7bb4f Compare November 19, 2019 20:44

RaduBerinde commented Nov 19, 2019

View reviewed changes

RaduBerinde requested a review from rohany November 19, 2019 22:02

rohany reviewed Nov 19, 2019

View reviewed changes

RaduBerinde commented Nov 20, 2019

View reviewed changes

jordanlewis reviewed Nov 20, 2019

View reviewed changes

rohany reviewed Nov 20, 2019

View reviewed changes

RaduBerinde force-pushed the fk-fam branch from 8e7bb4f to f456b88 Compare November 20, 2019 15:52

RaduBerinde commented Nov 20, 2019

View reviewed changes

sql: scan only primary column for FK checks

f9b9485

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

RaduBerinde force-pushed the fk-fam branch from f456b88 to f9b9485 Compare November 20, 2019 16:06

thoszhang reviewed Nov 20, 2019

View reviewed changes

rohany approved these changes Nov 20, 2019

View reviewed changes

craig bot merged commit f9b9485 into cockroachdb:master Nov 20, 2019

RaduBerinde deleted the fk-fam branch November 20, 2019 21:08

nvb mentioned this pull request Aug 5, 2020

sql/kv: support FOR NO KEY UPDATE and FOR KEY SHARE #52420

Open

michae2 mentioned this pull request Dec 19, 2023

opt: lock all column families for new SELECT FOR UPDATE #116170

Merged

Conversation

RaduBerinde commented Nov 19, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cockroach-teamcity commented Nov 19, 2019

Uh oh!

nvb left a comment

Choose a reason for hiding this comment

Uh oh!

RaduBerinde left a comment

Choose a reason for hiding this comment

Uh oh!

rohany left a comment

Choose a reason for hiding this comment

Uh oh!

rohany commented Nov 19, 2019

Uh oh!

RaduBerinde left a comment

Choose a reason for hiding this comment

Uh oh!

jordanlewis left a comment

Choose a reason for hiding this comment

Uh oh!

rohany left a comment

Choose a reason for hiding this comment

Uh oh!

RaduBerinde left a comment

Choose a reason for hiding this comment

Uh oh!

thoszhang left a comment

Choose a reason for hiding this comment

Uh oh!

rohany left a comment

Choose a reason for hiding this comment

Uh oh!

RaduBerinde commented Nov 20, 2019

Uh oh!

craig bot commented Nov 20, 2019

Build succeeded

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

RaduBerinde commented Nov 19, 2019 •

edited

Loading