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

Runner script dropping the existing triggers #74

Open
Dhanasekar93 opened this issue Jan 18, 2020 · 7 comments
Open

Runner script dropping the existing triggers #74

Dhanasekar93 opened this issue Jan 18, 2020 · 7 comments

Comments

@Dhanasekar93
Copy link

Dhanasekar93 commented Jan 18, 2020

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
Copy link
Collaborator

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

@Dhanasekar93
Copy link
Author

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
Copy link
Collaborator

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
Copy link
Author

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
Copy link
Collaborator

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
Copy link
Author

Dhanasekar93 commented Feb 7, 2020 via email

@michaelfinch
Copy link
Collaborator

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.

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

No branches or pull requests

2 participants