Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

sql: fix partial index and foreign key bugs #55702

Merged
merged 2 commits into from
Oct 19, 2020

Conversation

mgartner
Copy link
Collaborator

opt: build partial index predicates only when all table columns are in scope

This commit fixes a panic induced by trying to build partial index
predicate expressions without all of a table's columns in-scope. Some
scans, like scans built for foreign key checks, do not include all of a
table's column in their scope. For such scans, optbuilder no longer
attempts to build a partial index predicate because the predicate may
reference a column that is not in scope.

As a result of this change, opt.TableMeta may not have a predicate
expression for all partial indexes. The memo.PartialIndexPredicate
function which retrieves the predicate expressions has been updated to
account for this case.

Fixes #55672

Release justification: This is a critical bug fix for a new feature,
partial indexes.

Release note (bug fix): An INSERT into a table with a foreign key
reference to a table with a partial index no longer causes an error.

sql: disqualify partial unique indexes as foreign key reference indexes

Release justification: This is a critical bug fix for a new feature,
partial indexes.

Release note (bug fix): Foreign keys can no longer reference columns
that are only indexed by a partial unique index. A partial unique index
does not guarantee uniqueness in the entire table, therefore the column
indexed is not guaranteed to be a unique key.

@mgartner mgartner requested a review from RaduBerinde October 19, 2020 17:53
@mgartner mgartner requested a review from a team as a code owner October 19, 2020 17:53
@cockroach-teamcity
Copy link
Member

This change is Reviewable

Copy link
Member

@RaduBerinde RaduBerinde left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:lgtm: Thanks for the quick investigation and fix!

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


pkg/sql/opt/optbuilder/select.go, line 531 at r2 (raw file):

		// index predicates cannot be built because they may reference columns
		// not in outScope.
		containsAllOrdinaryTableColumns := true

[nit] We could skip the loop if we have tab.ColmnCount() columns (which I believe is the common case)

@andy-kimball
Copy link
Contributor

If the customer that ran into this issue creates a full index on the column (in addition to existing partial index), would it "fix" their issue? Can we tell them to workaround the problem by doing this?

Copy link
Collaborator

@rytaft rytaft left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:lgtm:

Reviewed 6 of 6 files at r1, 3 of 3 files at r2.
Reviewable status: :shipit: complete! 2 of 0 LGTMs obtained (waiting on @mgartner)


pkg/sql/opt/memo/expr.go, line 909 at r1 (raw file):

// Note that TableMeta.PartialIndexPredicates is not initialized for mutation
// queries. This function will never return ok=true in the context of a mutation.
func PartialIndexPredicate(

[nit] Do you need this function anymore? Doesn't seem like it's adding much on top of TableMeta.PartialIndexPredicates...

@RaduBerinde
Copy link
Member

No, they already had one (the PK).

@mgartner mgartner force-pushed the 55672-fix-fk-partial-index branch from c7d2daf to 3ee3d65 Compare October 19, 2020 18:32
Copy link
Collaborator Author

@mgartner mgartner left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


pkg/sql/opt/memo/expr.go, line 909 at r1 (raw file):

Previously, rytaft (Rebecca Taft) wrote…

[nit] Do you need this function anymore? Doesn't seem like it's adding much on top of TableMeta.PartialIndexPredicates...

Good point. It's also used far less with the recent changes to scanIndexIter. I've removed it here, but will not remove it in the backport to minimize changes.


pkg/sql/opt/optbuilder/select.go, line 531 at r2 (raw file):

Previously, RaduBerinde wrote…

[nit] We could skip the loop if we have tab.ColmnCount() columns (which I believe is the common case)

Good idea. Done.

Copy link
Collaborator

@rytaft rytaft left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:lgtm:

Reviewed 3 of 6 files at r3, 3 of 3 files at r4.
Reviewable status: :shipit: complete! 1 of 0 LGTMs obtained (and 1 stale) (waiting on @mgartner)


pkg/sql/opt/optbuilder/select.go, line 531 at r4 (raw file):

		// index predicates cannot be built because they may reference columns
		// not in outScope. In the most common case, the outScope has the same
		// number of columns as the table and we can skip checking the each

[nit] the each -> that each

@mgartner mgartner force-pushed the 55672-fix-fk-partial-index branch 2 times, most recently from 76456cd to d5d81c1 Compare October 19, 2020 19:05
Copy link
Collaborator Author

@mgartner mgartner left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


pkg/sql/opt/optbuilder/select.go, line 531 at r4 (raw file):

Previously, rytaft (Rebecca Taft) wrote…

[nit] the each -> that each

Done.

@mgartner
Copy link
Collaborator Author

bors r+

@mgartner
Copy link
Collaborator Author

bors r-

@craig
Copy link
Contributor

craig bot commented Oct 19, 2020

Canceled.

…n scope

This commit fixes a panic induced by trying to build partial index
predicate expressions without all of a table's columns in-scope. Some
scans, like scans built for foreign key checks, do not include all of a
table's column in their scope. For such scans, optbuilder no longer
attempts to build a partial index predicate because the predicate may
reference a column that is not in scope.

As a result of this change, `opt.TableMeta` may not have a predicate
expression for all partial indexes. The `memo.PartialIndexPredicate`
function which retrieves the predicate expressions has been updated to
account for this case.

Fixes cockroachdb#55672

Release justification: This is a critical bug fix for a new feature,
partial indexes.

Release note (bug fix): An INSERT into a table with a foreign key
reference to a table with a partial index no longer causes an error.
Release justification: This is a critical bug fix for a new feature,
partial indexes.

Release note (bug fix): Foreign keys can no longer reference columns
that are only indexed by a partial unique index. A partial unique index
does not guarantee uniqueness in the entire table, therefore the column
indexed is not guaranteed to be a unique key.
@mgartner mgartner force-pushed the 55672-fix-fk-partial-index branch from d5d81c1 to 21fd3a6 Compare October 19, 2020 20:44
@mgartner
Copy link
Collaborator Author

bors r+

@craig
Copy link
Contributor

craig bot commented Oct 19, 2020

Build succeeded:

@craig craig bot merged commit e4d1eb1 into cockroachdb:master Oct 19, 2020
@mgartner mgartner deleted the 55672-fix-fk-partial-index branch October 19, 2020 23:08
mgartner added a commit to mgartner/cockroach that referenced this pull request Oct 20, 2020
This commit re-enables the PartialIndexMutator for sqlsmith tests. This
mutator was disabled in cockroachdb#55724 after bug fixes in cockroachdb#55702 revealed
invalid schemas being created in sqlsmith. By reordering the mutators,
randomly generated schemas are no longer invalid.

Informs cockroachdb#55718

Release note: None
mgartner added a commit to mgartner/cockroach that referenced this pull request Oct 20, 2020
This commit re-enables the PartialIndexMutator for sqlsmith tests. This
mutator was disabled in cockroachdb#55724 after bug fixes in cockroachdb#55702 revealed
invalid schemas being created in sqlsmith. By reordering the mutators,
randomly generated schemas are no longer invalid.

Informs cockroachdb#55718

Release note: None
craig bot pushed a commit that referenced this pull request Oct 20, 2020
55761: sqlsmith: enable partial index mutator r=yuzefovich a=mgartner

This commit re-enables the PartialIndexMutator for sqlsmith tests. This
mutator was disabled in #55724 after bug fixes in #55702 revealed
invalid schemas being created in sqlsmith. By reordering the mutators,
randomly generated schemas are no longer invalid.

Informs #55718

Release note: None

Co-authored-by: Marcus Gartner <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Partial index in combination with foreign keys is broken
5 participants