skeema / tengo

Go La Tengo: a MySQL automation library
Apache License 2.0
27 stars 19 forks source link

Changing 2 foreign indexes for the same column #2

Closed chrisjpalmer closed 6 years ago

chrisjpalmer commented 7 years ago

If this is the current state of a table in the DB:

screen shot 2017-08-04 at 10 14 24 am

And you want to amend your mistake by changing it to this: --The mistake is that you have used the same column twice in (what is meant to be) a different foreign index.

screen shot 2017-08-04 at 9 04 08 am

You can do so by changing the DDL statement and then performing skeema push. This renders the following error:

screen shot 2017-08-04 at 10 15 59 am

I guess this is because it is trying to add the findex before deleting the old one?

evanelias commented 7 years ago

The problem appears to be related to how MySQL namespaces foreign keys. Apparently it doesn't permit you to run a single multi-clause DDL statement that drops a foreign key and then creates a new foreign key with the same name. Instead, these two clauses must be split into separate ALTER TABLE statements.

For example, this is illegal:

ALTER TABLE foo DROP FOREIGN KEY `bar`, ADD CONSTRAINT `bar` FOREIGN KEY (`some_col`) REFERENCES `some_table` (`id`) ON DELETE CASCADE;

But this is OK:

ALTER TABLE foo DROP FOREIGN KEY `bar`;
ALTER TABLE foo ADD CONSTRAINT `bar` FOREIGN KEY (`some_col`) REFERENCES `some_table` (`id`) ON DELETE CASCADE;

I think this is the first situation in Skeema where DDL clauses can't be combined into one statement. I'll need to give more thought how to handle this. It doesn't come up in other situations like normal indexes, since indexes are namespaced per table. In contrast, foreign keys are namespaced per database, which probably explains why they are handled differently by MySQL in this way.

evanelias commented 7 years ago

I have a potential fix, but I haven't pushed it to GitHub yet, since it's a bit over-complicated. I might try a different approach and see if it feels more natural.

The overall idea is that any TableDiff that adds a foreign key should be executed last, each in a separate AlterTable, after all other changes in the same schema. This has a few advantages:

But the complexity comes in how to organize the diffs such that the add-foreign-key values are separated out. And some calling code in Skeema assumes each table has at most 1 TableDiff per operation, so that will need to be more robust.

Anyway, definitely solvable, just wanted to give an update, since this is taking a bit longer than I had originally hoped. But it's still in progress, I haven't forgotten about this issue or the foreign-keys branch ;)

chrisjpalmer commented 7 years ago

That sounds great. Looking forward to seeing what you come up with. I agree the edge cases are very difficult.

On 28 Aug 2017, at 2:54 pm, Evan Elias notifications@github.com wrote:

I have a potential fix, but I haven't pushed it to GitHub yet, since it's a bit over-complicated. I might try a different approach and see if it feels more natural.

The overall idea is that any TableDiff that adds a foreign key should be executed last, each in a separate AlterTable, after all other changes in the same schema. This has a few advantages:

Supports modifications to existing foreign keys, without hitting conflicts with the name being re-used, since the DROP FOREIGN KEY gets executed earlier in a separate statement from the re-add Makes it easier to use foreign_key_checks=1 just when adding foreign keys Supports adding foreign keys that refer to other brand new columns in another table. (Doesn't necessarily work when referring to new columns in other schemas though; probably not going to worry about that edge case for now) But the complexity comes in how to organize the diffs such that the add-foreign-key values are separated out. And some calling code in Skeema assumes each table has at most 1 TableDiff per operation, so that will need to be more robust.

Anyway, definitely solvable, just wanted to give an update, since this is taking a bit longer than I had originally hoped. But it's still in progress, I haven't forgotten about this issue or the foreign-keys branch ;)

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/skeema/tengo/issues/2#issuecomment-325259847, or mute the thread https://github.com/notifications/unsubscribe-auth/AcaUOzl3MyYB-lWePhSJbvSavq3dF8dSks5sckgPgaJpZM4OtIqz.

evanelias commented 6 years ago

fwiw, I ended up solving this in #9 via a different approach than the one I outlined last year. The new approach is simply that if any TableDiff contains 1 or more DropForeignKey and 1 or more AddForeignKey, the Adds get split out into their own separate TableDiff, meaning that they will generate a separate AlterTable.

This will still require some tweaking in github.com/skeema/skeema, as it has some assumptions around each table in a diff either having 0 or 1 tablediffs, and now this is a case where they have 2. Should be an easy fix though, I'll have it in place in a few hours.

chrisjpalmer commented 6 years ago

Sounds good Evan.