square / shift

shift is an application that helps you run schema migrations on MySQL databases
Apache License 2.0
735 stars 51 forks source link

Runner script dropping the existing triggers #74

Open Dhanasekar93 opened 4 years ago

Dhanasekar93 commented 4 years ago

The runner script has dropped the old triggers for the table.

Logs:

I0118 13:00:53.844432 06964 runner.go:775] mig_id=9: cleaning up.
I0118 13:00:53.845156 06964 migration.go:322] mig_id=9: cleaning up triggers.
I0118 13:00:53.845193 06964 migration.go:350] mig_id=9: Running query 'SELECT trigger_name FROM information_schema.triggers WHERE trigger_schema=? AND event_object_table=?' (args: [stage t1]).
I0118 13:00:53.849166 06964 migration.go:357] mig_id=9: Query response was 'map[trigger_name:[check_trigger]]'
I0118 13:00:53.849207 06964 migration.go:364] mig_id=9: Running query 'DROP TRIGGER IF EXISTS `stage`.`check_trigger`'.
I0118 13:00:53.850382 06964 migration.go:328] mig_id=9: cleaning up shadow table.

is that expected behavior ?

michaelfinch commented 4 years ago

Depending on the situation it can be expected. What type of migration were you running, and what state was it in?

Dhanasekar93 commented 4 years ago

I tried to add a column a column for that table.

ALTER TABLE t1 add column rand_1 (int) default 1;

MySQL version is 5.7.25 and the table has triggers in it. Modified the runner code like below to add a support for [--preserve-triggers].

From: [Line number in git ]

        commandOptions = append(commandOptions, "--alter", alterStatement, "--execute",
            "-h", currentMigration.Host, "-P", strconv.Itoa(currentMigration.Port),
            "--defaults-file", runner.MysqlDefaultsFile, "--progress", ptOscProgress, "--exit-at", "copy",
            "--save-state", currentMigration.StateFile)

Like below: [Line number in git ]

        commandOptions = append(commandOptions, "--alter", alterStatement, "--execute",
            "-h", currentMigration.Host, "-P", strconv.Itoa(currentMigration.Port),
            "--defaults-file", runner.MysqlDefaultsFile, "--progress", ptOscProgress, "--exit-at", "copy",
            "--save-state", currentMigration.StateFile, "--preserve-triggers")

This is happening after the column addition in the table and the actual trigger for this table is dropped.

The status is failing at CleanUp migration steps.

michaelfinch commented 4 years ago

I'm not sure I follow. When you file a migration, the first thing Shift does is do a dry-run of pt-osc. If there are any existing triggers on the table, that dry-run will fail. Can you confirm that the dry-run is happening?

Dhanasekar93 commented 4 years ago

Initially it failed in dry-run itself. I have added the --preserve-triggers option in prepare migration state too in PrepMigrationStatus

Did i modified the code wrongly ?

michaelfinch commented 4 years ago

Okay, so let me make sure I understand you correctly. You're trying to run a migration on a table that already has triggers on it. Since shift fails on the dry-run step if a table has triggers, you patched shift to add the --preserve-triggers flag. This allowed the dry-run of your migration to complete. You were able to run the migration on your table, but then at the end shift dropped the pt-osc triggers as well as the triggers that were originally on the table. Do I have that right?

Dhanasekar93 commented 4 years ago

Yes exactly the same

michaelfinch commented 4 years ago

To answer your original question, yes, that is the expected behavior, because shift was never intended to operate on tables that have preexisting triggers on them. If we want to make it safe to run on such tables we'd need to add the --preserve-triggers flag to pt-osc as you did, but we'd also need to make sure the migration.DropTrigger method doesn't drop the preexisting triggers.