diff --git a/go/base/context.go b/go/base/context.go index c6ccb800c..1e96cbcc1 100644 --- a/go/base/context.go +++ b/go/base/context.go @@ -970,9 +970,8 @@ func (this *MigrationContext) GetGhostTriggerName(triggerName string) string { return triggerName + this.TriggerSuffix } -// validateGhostTriggerLength check if the ghost trigger name length is not more than 64 characters +// ValidateGhostTriggerLengthBelowMaxLength checks if the given trigger name (already transformed +// by GetGhostTriggerName) does not exceed the maximum allowed length. func (this *MigrationContext) ValidateGhostTriggerLengthBelowMaxLength(triggerName string) bool { - ghostTriggerName := this.GetGhostTriggerName(triggerName) - - return utf8.RuneCountInString(ghostTriggerName) <= mysql.MaxTableNameLength + return utf8.RuneCountInString(triggerName) <= mysql.MaxTableNameLength } diff --git a/go/base/context_test.go b/go/base/context_test.go index f87bc9f13..f8bce6f27 100644 --- a/go/base/context_test.go +++ b/go/base/context_test.go @@ -86,38 +86,69 @@ func TestGetTriggerNames(t *testing.T) { } func TestValidateGhostTriggerLengthBelowMaxLength(t *testing.T) { + // Tests simulate the real call pattern: GetGhostTriggerName first, then validate the result. { + // Short trigger name with suffix appended: well under 64 chars context := NewMigrationContext() context.TriggerSuffix = "_gho" - require.True(t, context.ValidateGhostTriggerLengthBelowMaxLength("my_trigger")) + ghostName := context.GetGhostTriggerName("my_trigger") // "my_trigger_gho" = 14 chars + require.True(t, context.ValidateGhostTriggerLengthBelowMaxLength(ghostName)) } { + // 64-char original + "_ghost" suffix = 70 chars → exceeds limit context := NewMigrationContext() context.TriggerSuffix = "_ghost" - require.False(t, context.ValidateGhostTriggerLengthBelowMaxLength(strings.Repeat("my_trigger_ghost", 4))) // 64 characters + "_ghost" + ghostName := context.GetGhostTriggerName(strings.Repeat("my_trigger_ghost", 4)) // 64 + 6 = 70 + require.False(t, context.ValidateGhostTriggerLengthBelowMaxLength(ghostName)) } { + // 48-char original + "_ghost" suffix = 54 chars → valid context := NewMigrationContext() context.TriggerSuffix = "_ghost" - require.True(t, context.ValidateGhostTriggerLengthBelowMaxLength(strings.Repeat("my_trigger_ghost", 3))) // 48 characters + "_ghost" + ghostName := context.GetGhostTriggerName(strings.Repeat("my_trigger_ghost", 3)) // 48 + 6 = 54 + require.True(t, context.ValidateGhostTriggerLengthBelowMaxLength(ghostName)) } { + // RemoveTriggerSuffix: 64-char name ending in "_ghost" → suffix removed → 58 chars → valid context := NewMigrationContext() context.TriggerSuffix = "_ghost" context.RemoveTriggerSuffix = true - require.True(t, context.ValidateGhostTriggerLengthBelowMaxLength(strings.Repeat("my_trigger_ghost", 4))) // 64 characters + "_ghost" removed + ghostName := context.GetGhostTriggerName(strings.Repeat("my_trigger_ghost", 4)) // suffix removed → 58 + require.True(t, context.ValidateGhostTriggerLengthBelowMaxLength(ghostName)) } { + // RemoveTriggerSuffix: name doesn't end in suffix → suffix appended → 65 + 6 = 71 chars → exceeds context := NewMigrationContext() context.TriggerSuffix = "_ghost" context.RemoveTriggerSuffix = true - require.False(t, context.ValidateGhostTriggerLengthBelowMaxLength(strings.Repeat("my_trigger_ghost", 4)+"X")) // 65 characters + "_ghost" not removed + ghostName := context.GetGhostTriggerName(strings.Repeat("my_trigger_ghost", 4) + "X") // no match, appended → 71 + require.False(t, context.ValidateGhostTriggerLengthBelowMaxLength(ghostName)) } { + // RemoveTriggerSuffix: 70-char name ending in "_ghost" → suffix removed → 64 chars → exactly at limit → valid context := NewMigrationContext() context.TriggerSuffix = "_ghost" context.RemoveTriggerSuffix = true - require.True(t, context.ValidateGhostTriggerLengthBelowMaxLength(strings.Repeat("my_trigger_ghost", 4)+"_ghost")) // 70 characters + last "_ghost" removed + ghostName := context.GetGhostTriggerName(strings.Repeat("my_trigger_ghost", 4) + "_ghost") // suffix removed → 64 + require.True(t, context.ValidateGhostTriggerLengthBelowMaxLength(ghostName)) + } + { + // Edge case: exactly 64 chars after transformation → valid (boundary test) + context := NewMigrationContext() + context.TriggerSuffix = "_ght" + originalName := strings.Repeat("x", 60) // 60 chars + ghostName := context.GetGhostTriggerName(originalName) // 60 + 4 = 64 + require.Equal(t, 64, len(ghostName)) + require.True(t, context.ValidateGhostTriggerLengthBelowMaxLength(ghostName)) + } + { + // Edge case: 65 chars after transformation → exceeds (boundary test) + context := NewMigrationContext() + context.TriggerSuffix = "_ght" + originalName := strings.Repeat("x", 61) // 61 chars + ghostName := context.GetGhostTriggerName(originalName) // 61 + 4 = 65 + require.Equal(t, 65, len(ghostName)) + require.False(t, context.ValidateGhostTriggerLengthBelowMaxLength(ghostName)) } } diff --git a/go/logic/inspect.go b/go/logic/inspect.go index 044360153..97895890d 100644 --- a/go/logic/inspect.go +++ b/go/logic/inspect.go @@ -596,7 +596,7 @@ func (this *Inspector) validateGhostTriggersDontExist() error { var foundTriggers []string for _, trigger := range this.migrationContext.Triggers { triggerName := this.migrationContext.GetGhostTriggerName(trigger.Name) - query := "select 1 from information_schema.triggers where trigger_name = ? and trigger_schema = ? and event_object_table = ?" + query := "select 1 from information_schema.triggers where trigger_name = ? and trigger_schema = ?" err := sqlutils.QueryRowsMap(this.db, query, func(rowMap sqlutils.RowMap) error { triggerExists := rowMap.GetInt("1") if triggerExists == 1 { @@ -606,7 +606,6 @@ func (this *Inspector) validateGhostTriggersDontExist() error { }, triggerName, this.migrationContext.DatabaseName, - this.migrationContext.OriginalTableName, ) if err != nil { return err diff --git a/go/logic/migrator.go b/go/logic/migrator.go index 714b42f1d..6c915ea62 100644 --- a/go/logic/migrator.go +++ b/go/logic/migrator.go @@ -842,7 +842,7 @@ func (this *Migrator) atomicCutOver() (err error) { // If we need to create triggers we need to do it here (only create part) if this.migrationContext.IncludeTriggers && len(this.migrationContext.Triggers) > 0 { if err := this.applier.CreateTriggersOnGhost(); err != nil { - this.migrationContext.Log.Errore(err) + return this.migrationContext.Log.Errore(err) } } diff --git a/go/mysql/utils.go b/go/mysql/utils.go index 7d57afbf1..9619e1e9f 100644 --- a/go/mysql/utils.go +++ b/go/mysql/utils.go @@ -248,9 +248,9 @@ func Kill(db *gosql.DB, connectionID string) error { // GetTriggers reads trigger list from given table func GetTriggers(db *gosql.DB, databaseName, tableName string) (triggers []Trigger, err error) { - query := fmt.Sprintf(`select trigger_name as name, event_manipulation as event, action_statement as statement, action_timing as timing - from information_schema.triggers - where trigger_schema = '%s' and event_object_table = '%s'`, databaseName, tableName) + query := `select trigger_name as name, event_manipulation as event, action_statement as statement, action_timing as timing + from information_schema.triggers + where trigger_schema = ? and event_object_table = ?` err = sqlutils.QueryRowsMap(db, query, func(rowMap sqlutils.RowMap) error { triggers = append(triggers, Trigger{ @@ -260,7 +260,7 @@ func GetTriggers(db *gosql.DB, databaseName, tableName string) (triggers []Trigg Timing: rowMap.GetString("timing"), }) return nil - }) + }, databaseName, tableName) if err != nil { return nil, err } diff --git a/localtests/trigger-ghost-name-conflict/create.sql b/localtests/trigger-ghost-name-conflict/create.sql new file mode 100644 index 000000000..32980daa0 --- /dev/null +++ b/localtests/trigger-ghost-name-conflict/create.sql @@ -0,0 +1,35 @@ +-- Bug #3 regression test: validateGhostTriggersDontExist must check whole schema +-- MySQL trigger names are unique per SCHEMA, not per table. +-- The validation must detect a trigger with the ghost name on ANY table in the schema. + +drop trigger if exists gh_ost_test_ai_ght; +drop trigger if exists gh_ost_test_ai; +drop table if exists gh_ost_test_other; +drop table if exists gh_ost_test; + +create table gh_ost_test ( + id int auto_increment, + i int not null, + primary key(id) +) auto_increment=1; + +-- This trigger has the _ght suffix (simulating a previous migration left it). +-- Ghost name = "gh_ost_test_ai" (suffix removed). +create trigger gh_ost_test_ai_ght + after insert on gh_ost_test for each row + set @dummy = 1; + +-- Create ANOTHER table with a trigger named "gh_ost_test_ai" (the ghost name). +-- Validation must detect this conflict even though the trigger is on a different table. +create table gh_ost_test_other ( + id int auto_increment, + primary key(id) +); + +create trigger gh_ost_test_ai + after insert on gh_ost_test_other for each row + set @dummy = 1; + +insert into gh_ost_test values (null, 11); +insert into gh_ost_test values (null, 13); +insert into gh_ost_test values (null, 17); diff --git a/localtests/trigger-ghost-name-conflict/destroy.sql b/localtests/trigger-ghost-name-conflict/destroy.sql new file mode 100644 index 000000000..5b5e1e4f7 --- /dev/null +++ b/localtests/trigger-ghost-name-conflict/destroy.sql @@ -0,0 +1,2 @@ +drop trigger if exists gh_ost_test_ai; +drop table if exists gh_ost_test_other; diff --git a/localtests/trigger-ghost-name-conflict/expect_failure b/localtests/trigger-ghost-name-conflict/expect_failure new file mode 100644 index 000000000..1a4c997f3 --- /dev/null +++ b/localtests/trigger-ghost-name-conflict/expect_failure @@ -0,0 +1 @@ +Found gh-ost triggers \ No newline at end of file diff --git a/localtests/trigger-ghost-name-conflict/extra_args b/localtests/trigger-ghost-name-conflict/extra_args new file mode 100644 index 000000000..128ce0a66 --- /dev/null +++ b/localtests/trigger-ghost-name-conflict/extra_args @@ -0,0 +1 @@ +--include-triggers --trigger-suffix=_ght --remove-trigger-suffix-if-exists \ No newline at end of file diff --git a/localtests/trigger-long-name-validation/create.sql b/localtests/trigger-long-name-validation/create.sql new file mode 100644 index 000000000..526713b74 --- /dev/null +++ b/localtests/trigger-long-name-validation/create.sql @@ -0,0 +1,38 @@ +-- Bug #1: Double-transformation in trigger length validation +-- A trigger with a 60-char name should be valid: ghost name = 60 + 4 (_ght) = 64 chars (max allowed). +-- But validateGhostTriggersLength() applies GetGhostTriggerName() twice, +-- computing 60 + 4 + 4 = 68, which falsely exceeds the 64-char limit. + +drop table if exists gh_ost_test; + +create table gh_ost_test ( + id int auto_increment, + i int not null, + primary key(id) +) auto_increment=1; + +-- Trigger name is exactly 60 characters (padded to 60). +-- Ghost name with _ght suffix = 64 chars = exactly at the MySQL limit. +-- 60 chars: trigger_long_name_padding_aaaaaaaaaaaaaaaaaaaaa_60chars_xxxx +create trigger trigger_long_name_padding_aaaaaaaaaaaaaaaaaaaaa_60chars_xxxx + after insert on gh_ost_test for each row + set @dummy = 1; + +insert into gh_ost_test values (null, 11); +insert into gh_ost_test values (null, 13); +insert into gh_ost_test values (null, 17); + +drop event if exists gh_ost_test; +delimiter ;; +create event gh_ost_test + on schedule every 1 second + starts current_timestamp + ends current_timestamp + interval 60 second + on completion not preserve + enable + do +begin + insert into gh_ost_test values (null, 23); + update gh_ost_test set i=i+1 where id=1; +end ;; +delimiter ; diff --git a/localtests/trigger-long-name-validation/extra_args b/localtests/trigger-long-name-validation/extra_args new file mode 100644 index 000000000..128ce0a66 --- /dev/null +++ b/localtests/trigger-long-name-validation/extra_args @@ -0,0 +1 @@ +--include-triggers --trigger-suffix=_ght --remove-trigger-suffix-if-exists \ No newline at end of file