opt: create key for UNIQUE WITHOUT INDEX constraints #60591

rytaft · 2021-02-15T18:27:51Z

opt: don't add unique constraints with an index to test catalog

This commit updates the test catalog so that it is consistent with the
real catalog and doesn't add unique constraints with an index to the
catalog table. This also prevents some tests from failing unnecessarily.

Release note: None

opt: create key for UNIQUE WITHOUT INDEX constraints

This commit teaches the optimizer that columns with a valid UNIQUE WITHOUT INDEX
constraint form a key, and the functional dependencies should reflect that. This will be
necessary to support locality optimized search.

Fixes #58944
Informs #55185

Release note (performance improvement): The optimizer now knows that
the unique columns in an implicitly partitioned unique index form a
key. This can be used to enable certain optimizations and may result
in better plans.

cockroach-teamcity · 2021-02-15T18:27:59Z

This change is

mgartner

[nit] The PR description doesn't include the first commit.

Reviewed 6 of 6 files at r1, 4 of 4 files at r2.
Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @RaduBerinde and @rytaft)

pkg/sql/opt/exec/execbuilder/testdata/unique, line 1153 at r2 (raw file):


# Same test as the previous, but now that the constraint has been validated, it
# can be treated as a key.

So the only difference is that this test includes the equality cols are key? How does that affect the lookup join? Does it allow it to be more efficient?

pkg/sql/opt/testutils/testcat/create_table.go, line 495 at r1 (raw file):

	if !withoutIndex {
		return
	}

You beat me to this. Thanks!

rytaft

TFTR! Updated and fixed a previous oversight. PTAL.

[nit] The PR description doesn't include the first commit.

Done.

Reviewable status: complete! 0 of 0 LGTMs obtained (and 1 stale) (waiting on @mgartner and @RaduBerinde)

pkg/sql/opt/exec/execbuilder/testdata/unique, line 1153 at r2 (raw file):

Previously, mgartner (Marcus Gartner) wrote…

So the only difference is that this test includes the equality cols are key? How does that affect the lookup join? Does it allow it to be more efficient?

I realized that the lookup join case was actually incorrect (after the update we can't guarantee that the constraints are unique... which is why we need the uniqueness checks in the first place), but the hash join cases are correct (right cols are key). This is because the right side is from the buffered row that was updated, and we know that only a single row was updated since the filter a=5 was applied to the table before the update.

But anyway, you're right that both "equality cols are key" and "right cols are key" will make the join more efficient -- added a comment.

pkg/sql/opt/testutils/testcat/create_table.go, line 495 at r1 (raw file):

Previously, mgartner (Marcus Gartner) wrote…

You beat me to this. Thanks!

👍

mgartner

Reviewed 10 of 10 files at r3.
Reviewable status: complete! 0 of 0 LGTMs obtained (and 1 stale) (waiting on @RaduBerinde and @rytaft)

pkg/sql/opt/exec/execbuilder/testdata/unique, line 1153 at r2 (raw file):

Previously, rytaft (Rebecca Taft) wrote…

I realized that the lookup join case was actually incorrect (after the update we can't guarantee that the constraints are unique... which is why we need the uniqueness checks in the first place), but the hash join cases are correct (right cols are key). This is because the right side is from the buffered row that was updated, and we know that only a single row was updated since the filter a=5 was applied to the table before the update.

But anyway, you're right that both "equality cols are key" and "right cols are key" will make the join more efficient -- added a comment.

Cool! Good catch too!

pkg/sql/opt/optbuilder/mutation_builder_unique.go, line 257 at r3 (raw file):

		tabMeta,
		h.uniqueAndPrimaryKeyOrdinals,
		&tree.IndexFlags{IgnoreUniqueWithoutIndexKeys: true},

[nit] add your explanation of why this is required ("after the update we can't guarantee that the constraints are unique... which is why we need the uniqueness checks in the first place")

rytaft

Reviewable status: complete! 0 of 0 LGTMs obtained (and 1 stale) (waiting on @mgartner and @RaduBerinde)

pkg/sql/opt/exec/execbuilder/testdata/unique, line 1153 at r2 (raw file):

Previously, mgartner (Marcus Gartner) wrote…

Cool! Good catch too!

Thanks!

pkg/sql/opt/optbuilder/mutation_builder_unique.go, line 257 at r3 (raw file):

Previously, mgartner (Marcus Gartner) wrote…

[nit] add your explanation of why this is required ("after the update we can't guarantee that the constraints are unique... which is why we need the uniqueness checks in the first place")

Done.

mgartner

Reviewed 1 of 1 files at r4.
Reviewable status: complete! 0 of 0 LGTMs obtained (and 1 stale) (waiting on @RaduBerinde)

RaduBerinde

Reviewable status: complete! 1 of 0 LGTMs obtained (and 1 stale) (waiting on @rytaft)

pkg/sql/opt/testutils/testcat/testdata/foreign_keys, line 15 at r1 (raw file):

 ├── p int not null
 ├── crdb_internal_mvcc_timestamp decimal [hidden] [system]
 ├── INDEX primary

[nit] Strange that there's nothing left indicating p is a key, are we missing a "unique" mention for the index?

rytaft

TFTR!

Reviewable status: complete! 0 of 0 LGTMs obtained (and 2 stale) (waiting on @mgartner)

pkg/sql/opt/testutils/testcat/testdata/foreign_keys, line 15 at r1 (raw file):

Previously, RaduBerinde wrote…

[nit] Strange that there's nothing left indicating p is a key, are we missing a "unique" mention for the index?

Done.

mgartner

Reviewed 22 of 22 files at r5, 11 of 11 files at r6.
Reviewable status: complete! 0 of 0 LGTMs obtained (and 2 stale) (waiting on @rytaft)

pkg/sql/opt/cat/utils.go, line 194 at r5 (raw file):

	if idx.IsUnique() {
		unique = "UNIQUE "
	}

We could have a special case for the primary index like:

 ├── PRIMARY INDEX primary
 │    └── x int not null

But I guess that'll alwasy be redundant because "primary" will always be the name of the index..

rytaft

Reviewable status: complete! 0 of 0 LGTMs obtained (and 2 stale) (waiting on @mgartner)

pkg/sql/opt/cat/utils.go, line 194 at r5 (raw file):

Previously, mgartner (Marcus Gartner) wrote…

We could have a special case for the primary index like:
 ├── PRIMARY INDEX primary
 │    └── x int not null
But I guess that'll alwasy be redundant because "primary" will always be the name of the index..

Done.

mgartner

Reviewed 22 of 22 files at r7, 11 of 11 files at r8.
Reviewable status: complete! 0 of 0 LGTMs obtained (and 2 stale) (waiting on @rytaft)

pkg/sql/opt/cat/utils.go, line 194 at r5 (raw file):

Previously, rytaft (Rebecca Taft) wrote…

Done.

Haha well I didn't have conviction that this was a great suggestion, but as long as you're for it then 👍

pkg/sql/opt/cat/utils.go, line 192 at r7 (raw file):

	}
	if idx.IsUnique() {
		if idx.Ordinal() == PrimaryIndex {

[nit] I think we could check only the ordinal for the PRIMARY case, because there's no such thing as a non-unique primary index, correct?

This commit updates the test catalog so that it is consistent with the real catalog and doesn't add unique constraints with an index to the catalog table. This also prevents some tests from failing unnecessarily. Release note: None

This commit teaches the optimizer that columns with a valid UNIQUE WITHOUT INDEX constraint form a key, and the functional dependencies should reflect that. This will be necessary to support locality optimized search. Fixes cockroachdb#58944 Informs cockroachdb#55185 Release note (performance improvement): The optimizer now knows that the unique columns in an implicitly partitioned unique index form a key. This can be used to enable certain optimizations and may result in better plans.

rytaft

bors r+

Reviewable status: complete! 0 of 0 LGTMs obtained (and 2 stale) (waiting on @mgartner)

pkg/sql/opt/cat/utils.go, line 194 at r5 (raw file):

Previously, mgartner (Marcus Gartner) wrote…

Haha well I didn't have conviction that this was a great suggestion, but as long as you're for it then 👍

Yea, I think it's a slight improvement, since it gives more information and more closely matches the real output of SHOW CREATE TABLE

pkg/sql/opt/cat/utils.go, line 192 at r7 (raw file):

Previously, mgartner (Marcus Gartner) wrote…

[nit] I think we could check only the ordinal for the PRIMARY case, because there's no such thing as a non-unique primary index, correct?

Done.

craig · 2021-02-17T13:11:21Z

Build failed:

GitHub CI (Cockroach)

rytaft · 2021-02-17T14:40:57Z

bors r+

craig · 2021-02-17T15:38:55Z

Build succeeded:

GitHub CI (Cockroach)

rytaft requested review from mgartner, RaduBerinde and a team February 15, 2021 18:27

rytaft requested a review from a team as a code owner February 15, 2021 18:27

mgartner approved these changes Feb 15, 2021

View reviewed changes

rytaft force-pushed the unique-keys branch from e3dd20a to f5330e7 Compare February 15, 2021 22:26

rytaft commented Feb 15, 2021

View reviewed changes

mgartner approved these changes Feb 15, 2021

View reviewed changes

rytaft force-pushed the unique-keys branch from f5330e7 to 5980e56 Compare February 16, 2021 01:25

rytaft commented Feb 16, 2021

View reviewed changes

mgartner approved these changes Feb 16, 2021

View reviewed changes

RaduBerinde approved these changes Feb 16, 2021

View reviewed changes

rytaft force-pushed the unique-keys branch from 5980e56 to 59228cc Compare February 17, 2021 00:10

rytaft commented Feb 17, 2021

View reviewed changes

rytaft force-pushed the unique-keys branch from 59228cc to 6c14505 Compare February 17, 2021 01:10

mgartner approved these changes Feb 17, 2021

View reviewed changes

rytaft force-pushed the unique-keys branch from 6c14505 to dd91572 Compare February 17, 2021 01:41

rytaft commented Feb 17, 2021

View reviewed changes

mgartner approved these changes Feb 17, 2021

View reviewed changes

rytaft added 2 commits February 17, 2021 05:16

rytaft force-pushed the unique-keys branch from dd91572 to 99c3af7 Compare February 17, 2021 12:25

rytaft commented Feb 17, 2021

View reviewed changes

craig bot merged commit 06486c9 into cockroachdb:master Feb 17, 2021

irfansharif mentioned this pull request Mar 15, 2021

roachtest: tpccbench/nodes=3/cpu=16 failed #61973

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

opt: create key for UNIQUE WITHOUT INDEX constraints #60591

opt: create key for UNIQUE WITHOUT INDEX constraints #60591

rytaft commented Feb 15, 2021 •

edited

Loading

cockroach-teamcity commented Feb 15, 2021

mgartner left a comment

rytaft left a comment

mgartner left a comment

rytaft left a comment

mgartner left a comment

RaduBerinde left a comment

rytaft left a comment

mgartner left a comment

rytaft left a comment

mgartner left a comment

rytaft left a comment

craig bot commented Feb 17, 2021

rytaft commented Feb 17, 2021

craig bot commented Feb 17, 2021

opt: create key for UNIQUE WITHOUT INDEX constraints #60591

opt: create key for UNIQUE WITHOUT INDEX constraints #60591

Conversation

rytaft commented Feb 15, 2021 • edited Loading

cockroach-teamcity commented Feb 15, 2021

mgartner left a comment

Choose a reason for hiding this comment

rytaft left a comment

Choose a reason for hiding this comment

mgartner left a comment

Choose a reason for hiding this comment

rytaft left a comment

Choose a reason for hiding this comment

mgartner left a comment

Choose a reason for hiding this comment

RaduBerinde left a comment

Choose a reason for hiding this comment

rytaft left a comment

Choose a reason for hiding this comment

mgartner left a comment

Choose a reason for hiding this comment

rytaft left a comment

Choose a reason for hiding this comment

mgartner left a comment

Choose a reason for hiding this comment

rytaft left a comment

Choose a reason for hiding this comment

craig bot commented Feb 17, 2021

rytaft commented Feb 17, 2021

craig bot commented Feb 17, 2021

rytaft commented Feb 15, 2021 •

edited

Loading