schemadiff: validate and apply foreign key indexes #12026

shlomi-noach · 2023-01-01T13:46:23Z

Description

This PR extends both #11944 and #12016 ~~(at this time both unmerged)~~.

In this PR, schemadiff runs the following:

for every foreign key constraint, table validation verifies there's an appropriate index on the local table columns
for every foreign key constraint, schema validation verifies there's an appropriate index on the remote table columns
Apply()ing a ADD CONSTRAINT ... FOREIGN KEY auto-adds an appropriate index on the local table, if one does not exist. This is compliant with MySQL behavior.
A CREATE TABLE is normalized to have an implicit index, if need be, for every foreign key definition. This is compliant with MySQL behavior.

Tests added to validate new behavior.

Related Issue(s)

Foreign keys tracking issue: Experimental foreign keys tracking issue #11975
schemadiff tracking issue: schemadiff: tracking issue #10203
schemadiff: normalize PRIMARY KEY definition #12016
schemadiff: foreign key validation (tables and columns) #11944

Checklist

"Backport to:" labels have been added if this change should be back-ported
Tests were added or are not required
Documentation was added or is not required

Deployment Notes

Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com>

…nKeyColumnCountError, MismatchingForeignKeyColumnTypeError Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com>

…nstraint columns Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com>

Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com>

…ition splits into 'id int' and 'primary key (id)' parts. Primary key is always the first key in the list of keys. Handle queries such as 'modify id int primary key' (validate there isn't already a primary key, or that the existing primary key is over 'id'. add column with primary key definition... Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com>

Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com>

vitess-bot · 2023-01-01T13:46:25Z

Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com>

…smatchError Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com>

Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com>

shlomi-noach · 2023-01-23T14:22:58Z

Ready for review

…eded Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com>

dbussink · 2023-01-24T08:56:22Z

go/vt/schemadiff/errors.go

+		sqlescape.EscapeID(e.Key),
+		sqlescape.EscapeID(e.Table),
+	)
+}


Hmm, I'm not sure why these are 3 different error types when they seem all pretty similar? Would it be simpler to model this as one?

The first two seem to be the case of there's an FK constraint to the table itself, but in that case I think it's ok if Table and ReferencedTable contain the same value?

Not sure about the 3rd one, but that seems to not have a constraint name and not sure when that happens? Don't we always have to generate a constraint name anyway if it's missing and would it be fine if that name was set then on the same error?

Good catch on the 1st error, MissingForeignKeyIndexError: I've just removed it. It can never fire because, compatible with MySQL's behavior, we always add an index if one is needed, either in a CREATE TABLE statement or in a ALTER TABLE ... ADD ... FOREIGN KEY statement.

2nd error, MissingForeignKeyReferencedIndexError, is when the parent table does not have an index covering the referenced columns. Here, MySQL does not add anything implicitly, because that's on a different table. The error is compatible with MySQL.

3rd error, IndexNeededByForeignKeyError is for when you attempt to ALTER TABLE ... DROP KEY and when that leaves a foreign key without an index covering its local columns. Compatible with MySQL, that's an error.

dbussink · 2023-01-24T08:59:27Z

go/vt/schemadiff/table.go

+	for i, col := range columns {
+		// the index must cover same columns, in order, wih possibly more columns covered than requested.
+		indexCol := index.Columns[i]
+		if col.String() != indexCol.Column.String() {


Should this be case insensitive? So like using !strings.EqualFold instead of !=?

Good catch, fixed

dbussink · 2023-01-24T09:00:30Z

go/vt/schemadiff/table_test.go

+	require.NoError(t, err)
+	tableColumns := map[string]sqlparser.IdentifierCI{}
+	for _, col := range c.CreateTable.TableSpec.Columns {
+		tableColumns[col.Name.String()] = col.Name


Should this be using strings.ToLower() as well? And should we extend the tests to have some more case mismatch cases to verify the behavior when those exist?

Good catch, fixed, and added a test!

…n tests Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com>

…QL behavior, we implicitly add the index if missing Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com>

shlomi-noach · 2023-01-29T06:20:00Z

Looking for a 2nd review/approval

rohit-nayak-ps

lgtm

* more testing for getViewDependentTableNames() * getForeignKeyParentTableNames * schema normalization: sort tables by fk reference order * minor test addition * ForeignKeyDependencyUnresolvedError * InvalidReferencedColumnInForeignKeyConstraintError, MismatchingForeignKeyColumnCountError, MismatchingForeignKeyColumnTypeError * validate or autogenerate index on local table based on foreign key constraint columns * validate referenced table key * ColumnKeyOption consts become public * adding DuplicateKeyNameError * normalize PRIMARY KEY definition: a 'id int primary key' column definition splits into 'id int' and 'primary key (id)' parts. Primary key is always the first key in the list of keys. Handle queries such as 'modify id int primary key' (validate there isn't already a primary key, or that the existing primary key is over 'id'. add column with primary key definition... * further unit test adaptations * adapting tests: adding required local index over foreign key columns * implicitly add key over foreign key columns if no appropriate key exists * normalize CREATE TABLE statement to include missing foreign key indexes * rename error: ForeignKeyColumnCountMismatchError * rename MismatchingForeignKeyColumnTypeError to ForeignKeyColumnTypeMismatchError * changes per review * add a few tests that validate an implicit index isn't added if not needed * case insensitive column comparison in indexCoversColumnsInOrder and in tests * MissingForeignKeyIndexError is not needed bcause, compatible with MySQL behavior, we implicitly add the index if missing --------- Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com>

shlomi-noach added 17 commits December 12, 2022 09:28

more testing for getViewDependentTableNames()

5acb9f3

Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com>

getForeignKeyParentTableNames

9e6e31c

Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com>

schema normalization: sort tables by fk reference order

9224d01

Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com>

minor test addition

03139da

Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com>

ForeignKeyDependencyUnresolvedError

5079877

Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com>

InvalidReferencedColumnInForeignKeyConstraintError, MismatchingForeig…

29bac39

…nKeyColumnCountError, MismatchingForeignKeyColumnTypeError Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com>

validate or autogenerate index on local table based on foreign key co…

87e00ee

…nstraint columns Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com>

validate referenced table key

fb5b19d

Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com>

ColumnKeyOption consts become public

fc78e0b

Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com>

adding DuplicateKeyNameError

fc5d4b6

Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com>

Merge branch 'main' into schema-diff-normalize-primary-key

8631527

Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com>

further unit test adaptations

b0a8c6f

Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com>

Merge branch 'main' into schemadiff-fk-keys

d9cd695

Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com>

merge main, resolve conflict

0f8c986

Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com>

adapting tests: adding required local index over foreign key columns

6eded6c

Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com>

implicitly add key over foreign key columns if no appropriate key exists

c0a3df4

Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com>

normalize CREATE TABLE statement to include missing foreign key indexes

211f85d

Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com>

shlomi-noach added Type: Enhancement Logical improvement (somewhere between a bug and feature) Component: Query Serving labels Jan 1, 2023

shlomi-noach mentioned this pull request Jan 2, 2023

*WIP* schemadiff: Diff() of schemas ordered by valid dependencies #12028

Closed

3 tasks

shlomi-noach mentioned this pull request Jan 23, 2023

schemadiff: foreign key validation (tables and columns) #11944

Merged

3 tasks

shlomi-noach added 4 commits January 23, 2023 15:38

rename error: ForeignKeyColumnCountMismatchError

76a4834

Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com>

rename MismatchingForeignKeyColumnTypeError to ForeignKeyColumnTypeMi…

fd46324

…smatchError Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com>

changes per review

9daec2b

Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com>

merge main, resolve conflict

6a3548d

Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com>

shlomi-noach marked this pull request as ready for review January 23, 2023 14:20

shlomi-noach requested a review from deepthi as a code owner January 23, 2023 14:20

shlomi-noach changed the title *WIP* schemadiff: validate and apply foreign key indexes schemadiff: validate and apply foreign key indexes Jan 23, 2023

shlomi-noach requested review from dbussink and a team January 23, 2023 14:22

add a few tests that validate an implicit index isn't added if not ne…

3fe002c

…eded Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com>

dbussink reviewed Jan 24, 2023

View reviewed changes

shlomi-noach added 2 commits January 24, 2023 11:06

case insensitive column comparison in indexCoversColumnsInOrder and i…

d175211

…n tests Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com>

MissingForeignKeyIndexError is not needed bcause, compatible with MyS…

16e1ed3

…QL behavior, we implicitly add the index if missing Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com>

dbussink approved these changes Jan 24, 2023

View reviewed changes

rohit-nayak-ps approved these changes Jan 29, 2023

View reviewed changes

shlomi-noach merged commit ae8406d into vitessio:main Jan 29, 2023

shlomi-noach deleted the schemadiff-fk-keys branch January 29, 2023 12:40

shlomi-noach mentioned this pull request Jan 29, 2023

*WIP* schemadiff: Diff() of schemas ordered by valid dependencies #12184

Closed

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

schemadiff: validate and apply foreign key indexes #12026

schemadiff: validate and apply foreign key indexes #12026

shlomi-noach commented Jan 1, 2023 •

edited

Loading

vitess-bot bot commented Jan 1, 2023 •

edited by dbussink

Loading

shlomi-noach commented Jan 23, 2023

dbussink Jan 24, 2023

shlomi-noach Jan 24, 2023

dbussink Jan 24, 2023

shlomi-noach Jan 24, 2023

dbussink Jan 24, 2023

shlomi-noach Jan 24, 2023

shlomi-noach commented Jan 29, 2023

rohit-nayak-ps left a comment

schemadiff: validate and apply foreign key indexes #12026

schemadiff: validate and apply foreign key indexes #12026

Conversation

shlomi-noach commented Jan 1, 2023 • edited Loading

Description

Related Issue(s)

Checklist

Deployment Notes

vitess-bot bot commented Jan 1, 2023 • edited by dbussink Loading

Review Checklist

General

If a new flag is being introduced:

If a workflow is added or modified:

Bug fixes

Non-trivial changes

New/Existing features

Backward compatibility

shlomi-noach commented Jan 23, 2023

dbussink Jan 24, 2023

Choose a reason for hiding this comment

shlomi-noach Jan 24, 2023

Choose a reason for hiding this comment

dbussink Jan 24, 2023

Choose a reason for hiding this comment

shlomi-noach Jan 24, 2023

Choose a reason for hiding this comment

dbussink Jan 24, 2023

Choose a reason for hiding this comment

shlomi-noach Jan 24, 2023

Choose a reason for hiding this comment

shlomi-noach commented Jan 29, 2023

rohit-nayak-ps left a comment

Choose a reason for hiding this comment

shlomi-noach commented Jan 1, 2023 •

edited

Loading

vitess-bot bot commented Jan 1, 2023 •

edited by dbussink

Loading