pick new viable underlying index for foreign keys when current one is dropped #4847

jycor · 2022-11-22T20:47:18Z

When dropping underlying secondary indexes used by foreign key, another existing viable one should be used in its place, unless there isn't one.

TODO:

we should show duplicate indexes to primary key because MySQL does

zachmu

Good start, needs a little work

zachmu · 2022-11-22T23:00:33Z

go/libraries/doltcore/sqle/tables.go

+		return nil, nil, err
+	}
+	for _, fk := range fkc.AllKeys() {
+		newIdx, ok, err := findIndexWithPrefix(t.sch, oldIdx.ColumnNames())


This isn't quite right: the old index being used might have had more column names than necessary for it to be used with this foreign key.

Consider parent (a, b, c), child (d, e, f). Parent table has an index on (a,b,c) and (a,b). Child declares a foreign key constraint on d references parent (a). We'll use the index (a,b,c) for it. When we drop index (a,b,c), we should fall back to (a,b). But this code doesn't do that, because it tries to use the old index columns, as opposed to the constraint definition.

Add a test.

zachmu · 2022-11-22T23:02:18Z

go/libraries/doltcore/sqle/tables.go

+			newFk.ReferencedTableIndex = newIdx.Name()
+		} else {
+			// if a replacement index wasn't found; it matched on primary key, so use empty string
+			newFk.ReferencedTableIndex = ""


Also not good enough -- you can't make this assumption. If there is no index available to use after dropping this one, you have to raise an error.

I think we can make this assumption, because we do in other places like here and here.

This is because GMS also checks if a valid prefix index exists, and errors when there isn't (I updated the tests to confirm this behavior). In the case of switching to a PK as the underlying index, GMS actually considers PKs as indexes and will correctly not error. However, on dolt side, we don't treat PK as an index, so findIndexWithPrefix wont be able to find it. A side effect of this is that if there is a viable PK and multiple viable secondary indexes, the PK will only be chosen if we drop all other viable secondary indexes first.

GMS Code for reference:

drop index

find prefix

As long as there are tests then great

zachmu · 2022-11-23T17:40:53Z

go/libraries/doltcore/sqle/sqlddl_test.go

@@ -1186,6 +1186,90 @@ INSERT INTO child_non_unq VALUES ('1', 1), ('2', NULL), ('3', 3), ('4', 3), ('5'
 	}
 }

+func TestDropIndex(t *testing.T) {


Probably better expressed as a Sql script test in ddl_queries.go

I don't think there are any queries that show what underlying secondary index a foreign key is using, which is why I wrote the tests here.

I missed the explicit checks into foreign keys here, my fault.

There are some decent functional tests in GMS for this, so I think you're covered there.

But you don't have coverage for a bunch of scenarios you should be testing. Specifically, there aren't multiple indexes to choose from when you drop the existing one here.

A unit test for findIndexWithPrefix could be more effective for that, your call

zachmu

See comments

zachmu · 2022-11-23T19:46:15Z

go/libraries/doltcore/sqle/sqlddl_test.go

+	// setup
+	root, err = ExecuteSql(dEnv, root, "create table parent (i int);")
+	require.NoError(t, err)
+	root, err = ExecuteSql(dEnv, root, "alter table parent add index (i);")


Name these indexes explicitly so that the names you use below don't break if we change the auto naming scheme

zachmu · 2022-11-23T19:46:35Z

go/libraries/doltcore/sqle/tables.go

+			newFk.ReferencedTableIndex = newIdx.Name()
+		} else {
+			// if a replacement index wasn't found; it matched on primary key, so use empty string
+			newFk.ReferencedTableIndex = ""


As long as there are tests then great

zachmu · 2022-11-23T20:19:14Z

go/libraries/doltcore/sqle/sqlddl_test.go

@@ -1186,6 +1186,90 @@ INSERT INTO child_non_unq VALUES ('1', 1), ('2', NULL), ('3', 3), ('4', 3), ('5'
 	}
 }

+func TestDropIndex(t *testing.T) {


I missed the explicit checks into foreign keys here, my fault.

There are some decent functional tests in GMS for this, so I think you're covered there.

But you don't have coverage for a bunch of scenarios you should be testing. Specifically, there aren't multiple indexes to choose from when you drop the existing one here.

A unit test for findIndexWithPrefix could be more effective for that, your call

zachmu · 2022-11-24T00:07:50Z

go/cmd/dolt/commands/indexcmds/ls.go

@@ -113,6 +113,16 @@ func (cmd LsCmd) Exec(ctx context.Context, commandStr string, args []string, dEn
 		}
 	}

+	// TODO: delete this


zachmu · 2022-11-24T00:30:54Z

go/libraries/doltcore/sqle/sqlddl_test.go

+	require.NoError(t, err)
+
+	// dropping secondary index ij, should choose ijk
+	root, err = ExecuteSql(dEnv, root, "alter table parent drop index ij;")


Is this deterministic? Odd that it doesn't choose the primary key if suitable

Add a note that this isn't by design

zachmu · 2022-11-24T00:31:57Z

go/libraries/doltcore/sqle/sqlddl_test.go

+	require.NoError(t, err)
+
+	// setup
+	root, err = ExecuteSql(dEnv, root, "create table parent (i int);")


You have two separate test cases in this method. Name them and run them in t.Run() blocks

zachmu · 2022-11-24T00:32:26Z

go/libraries/doltcore/sqle/tables.go

@@ -1851,6 +1851,7 @@ func (t *AlterableDoltTable) CreateIndex(ctx *sql.Context, idx sql.IndexDef) err
 	if err != nil {
 		return err
 	}
+	// TODO: why do we need to do this?


zachmu · 2022-11-24T00:32:27Z

go/libraries/doltcore/sqle/tables.go

@@ -1851,6 +1851,7 @@ func (t *AlterableDoltTable) CreateIndex(ctx *sql.Context, idx sql.IndexDef) err
 	if err != nil {
 		return err
 	}
+	// TODO: why do we need to do this?


zachmu

LGTM, just a couple small comments and some code that probably should be removed before checkin

James Cor added 8 commits November 21, 2022 14:04

potential fix for fk references

1e22d3c

Merge branch 'main' into james/fks

a8579b7

fixed

2603051

see if anything breaks

7a9e574

fix tests

51c3414

this is needed

b04692a

adding tests

98da645

removing comment

db87d74

zachmu reviewed Nov 23, 2022

View reviewed changes

James Cor added 3 commits November 23, 2022 10:21

Merge branch 'main' into james/fks

1d4d62f

Merge branch 'main' into james/fks

6c64f7a

feedback

1af3624

zachmu reviewed Nov 23, 2022

View reviewed changes

James Cor added 2 commits November 23, 2022 16:04

fix test, and dropping pk

df9e400

fix better

768a47b

zachmu reviewed Nov 24, 2022

View reviewed changes

zachmu approved these changes Nov 24, 2022

View reviewed changes

James Cor added 3 commits November 27, 2022 22:03

Merge branch 'main' into james/fks

1a844c8

feedback

da65a5d

putting into run blocks

17bf30d

jycor merged commit fe2eb95 into main Nov 28, 2022

tbantle22 deleted the james/fks branch March 9, 2023 00:58

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

pick new viable underlying index for foreign keys when current one is dropped #4847

pick new viable underlying index for foreign keys when current one is dropped #4847

jycor commented Nov 22, 2022 •

edited

zachmu left a comment

zachmu Nov 22, 2022

zachmu Nov 22, 2022

jycor Nov 23, 2022 •

edited

zachmu Nov 23, 2022

zachmu Nov 23, 2022

jycor Nov 23, 2022

zachmu Nov 23, 2022

zachmu left a comment

zachmu Nov 23, 2022

zachmu Nov 23, 2022

zachmu Nov 23, 2022

zachmu Nov 24, 2022

zachmu Nov 24, 2022

zachmu Nov 24, 2022

zachmu Nov 24, 2022

zachmu Nov 24, 2022

zachmu Nov 24, 2022

zachmu left a comment

pick new viable underlying index for foreign keys when current one is dropped #4847

pick new viable underlying index for foreign keys when current one is dropped #4847

Conversation

jycor commented Nov 22, 2022 • edited

zachmu left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jycor Nov 23, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zachmu left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zachmu left a comment

Choose a reason for hiding this comment

jycor commented Nov 22, 2022 •

edited

jycor Nov 23, 2022 •

edited