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: properly enforce uniqueness of key columns for FK references #43793

Merged
merged 1 commit into from
Jan 14, 2020

Conversation

thoszhang
Copy link
Contributor

@thoszhang thoszhang commented Jan 7, 2020

We introduced a bug in 19.2 that would allow, e.g., a unique index on (a, b, c) to be used as an index that is supposed to enforce uniqueness for a foreign
key constraint pointing only to (a, b). This PR reintroduces a check that the
indexed columns match the FK columns exactly.

Fixes #42680.

Release note (bug fix): Fixed a bug introduced in 19.2 that would allow foreign
keys to use a unique index on the referenced columns that indexed more columns
than were included in the columns used in the FK constraint, which allows
potentially violating uniqueness in the referenced columns themselves.

@thoszhang thoszhang requested a review from otan January 7, 2020 20:09
@cockroach-teamcity
Copy link
Member

This change is Reviewable

Copy link
Contributor

@otan otan left a comment

Choose a reason for hiding this comment

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

Minor nit, otherwise LGTM!

Not important for this PR, but checking -- could we also allow these kinds of indices if they were in different order, e.g. can a foreign key referencing (a, b) match on unique index (b, a)?

Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (waiting on @lucy-zhang and @otan)


pkg/sql/sqlbase/table.go, line 426 at r1 (raw file):

	// Search for a unique index on the referenced table that matches our foreign
	// key columns.
	if len(referencedTable.PrimaryIndex.ColumnIDs) == len(referencedColIDs) &&

nit: I think these changes read better if we had an Equal function defined on the ColumnIDs data type.

@otan
Copy link
Contributor

otan commented Jan 9, 2020


pkg/sql/sqlbase/table.go, line 426 at r1 (raw file):

Previously, otan (Oliver Tan) wrote…

nit: I think these changes read better if we had an Equal function defined on the ColumnIDs data type.

(or "Exactly")

@thoszhang thoszhang force-pushed the fix-fk-unique-index-matching branch 2 times, most recently from 4906f5d to 609a554 Compare January 14, 2020 17:20
Copy link
Contributor Author

@thoszhang thoszhang 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


pkg/sql/sqlbase/table.go, line 426 at r1 (raw file):

Previously, otan (Oliver Tan) wrote…

(or "Exactly")

Done.

Copy link
Contributor Author

@thoszhang thoszhang left a comment

Choose a reason for hiding this comment

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

We don't support that yet, but I think there are indefinite plans to support it eventually.

Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained

We introduced a bug in 19.2 that would allow, e.g., a unique index on `(a, b,
c)` to be used as an index that is supposed to enforce uniqueness for a foreign
key constraint pointing only to `(a, b)`. This PR reintroduces a check that the
indexed columns match the FK columns exactly.

Release note (bug fix): Fixed a bug introduced in 19.2 that would allow foreign
keys to use a unique index on the referenced columns that indexed more columns
than were included in the columns used in the FK constraint, which allows
potentially violating uniqueness in the referenced columns themselves.
@thoszhang
Copy link
Contributor Author

TFTR!

bors r+

craig bot pushed a commit that referenced this pull request Jan 14, 2020
43311: colserde: add simple serialization for DECIMALs r=yuzefovich a=yuzefovich

This commit adds simple serialization of DECIMALs using the provided
MarshalText method.

Touches: #37044.

Release note (sql change): Previously, DECIMALs could not be sent over
the network when the computation was performed by the vectorized engine.
This has been fixed, and the vectorized engine now fully supports
DECIMAL type.

43793: sql: properly enforce uniqueness of key columns for FK references r=lucy-zhang a=lucy-zhang

We introduced a bug in 19.2 that would allow, e.g., a unique index on `(a, b,
c)` to be used as an index that is supposed to enforce uniqueness for a foreign
key constraint pointing only to `(a, b)`. This PR reintroduces a check that the
indexed columns match the FK columns exactly.

Fixes #42680.

Release note (bug fix): Fixed a bug introduced in 19.2 that would allow foreign
keys to use a unique index on the referenced columns that indexed more columns
than were included in the columns used in the FK constraint, which allows
potentially violating uniqueness in the referenced columns themselves.

43956: build: post issues from local roachtests r=andreimatei a=tbg

This wasn't happening due to some missing env vars. I noticed this since
there are many failures of acceptance/version-upgrade not reflected via
created issues (I get email notifications from TC).

Release note: None

Co-authored-by: Yahor Yuzefovich <yahor@cockroachlabs.com>
Co-authored-by: Lucy Zhang <lucy-zhang@users.noreply.github.com>
Co-authored-by: Tobias Schottdorf <tobias.schottdorf@gmail.com>
@thoszhang
Copy link
Contributor Author

The TC failure seems to be this test flake: #43826

@craig
Copy link
Contributor

craig bot commented Jan 14, 2020

Build succeeded

@craig craig bot merged commit 6430e47 into cockroachdb:master Jan 14, 2020
@thoszhang thoszhang deleted the fix-fk-unique-index-matching branch January 14, 2020 18:07
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.

sql: FK constraints can erroneously use unique indexes on a strict superset of their columns
4 participants