Sync upstream 2026-03-10#26
Merged
Merged
Conversation
…er (github#1623) The WriteChangelogState call at line 1365 was missing error handling, which could cause the migration to fail silently if the changelog table was not properly created. This resulted in cryptic "Table doesn't exist" errors later in the migration process. All other calls to WriteChangelogState in the codebase properly check for errors. This change makes error handling consistent and provides clearer error messages when changelog table operations fail. Co-authored-by: Claude Sonnet 4.5 <noreply@anthropic.com>
…hub#1628) * Fix binary column trailing zero stripping for non-key columns MySQL's binlog strips trailing 0x00 bytes from binary(N) columns. PR github#915 fixed this for unique key columns only, but the same issue affects all binary columns in INSERT/UPDATE operations. Remove the isUniqueKeyColumn condition so all binary(N) columns are padded to their declared length. Fixes a variation of github#909 where the affected column is not a primary key. * Simplify by removing isUniqueKeyColumn now that it's no longer used. * In convertArg, don't convert binary data to strings. In this case, the input is binary, and the column type is `binary`. So the output should be binary, not text. * fix a lint
* fix: remove double-transformation in trigger length validation ValidateGhostTriggerLengthBelowMaxLength was calling GetGhostTriggerName on an already-transformed name, adding the suffix twice. This caused valid trigger names (ghost name <= 64 chars) to be falsely rejected. The caller in inspect.go:627 already transforms the name via GetGhostTriggerName before passing it, so the validation function should check the length as-is. Unit tests updated to reflect the correct call pattern: transform first with GetGhostTriggerName, then validate the result. Added boundary tests for exactly 64 and 65 char names. * fix: return error from trigger creation during atomic cut-over During atomic cut-over, if CreateTriggersOnGhost failed, the error was logged but not returned. The migration continued and completed without triggers, silently losing them. The two-step cut-over (line 793) already correctly returns the error. This aligns the atomic cut-over to do the same. * fix: check trigger name uniqueness per schema, not per table validateGhostTriggersDontExist was filtering by event_object_table, only checking if the ghost trigger name existed on the original table. MySQL trigger names are unique per schema, so a trigger with the same name on any other table would block CREATE TRIGGER but pass validation. Remove the event_object_table filter to check trigger_name + trigger_schema only, matching MySQL's uniqueness constraint. * fix: use parameterized query in GetTriggers to prevent SQL injection GetTriggers used fmt.Sprintf with string interpolation for database and table names, causing SQL syntax errors with special characters and potential SQL injection. Switched to parameterized query with ? placeholders, matching the safe pattern already used in inspect.go:553-559. * test: add regression tests for trigger handling bugs Add two integration tests: - trigger-long-name-validation: verifies 60-char trigger names (64-char ghost name) are not falsely rejected by double-transform - trigger-ghost-name-conflict: verifies validation detects ghost trigger name conflicts on other tables in the same schema * style: gofmt context_test.go --------- Co-authored-by: Yakir Gibraltar <yakir.g@taboola.com> Co-authored-by: meiji163 <meiji163@github.com>
* Execute hook on every batch insert retry Co-authored-by: Bastian Bartmann <bastian.bartmann@shopify.com> * Expose the last error message to the onBatchCopyRetry hook Co-authored-by: Bastian Bartmann <bastian.bartmann@shopify.com> * Remove double retries CalculateNextIterationRangeEndValues needs to be recomputed on every retry in case of configuration (e.g. chunk-size) changes were made by onBatchCopyRetry hooks. * include dev.yml (temp for Shopify) * Update doc/hooks.md * Remove dev.yml * Fix retry issue where MigrationIterationRangeMinValues advances before insert completes - extract MigrationContext.SetNextIterationRangeValues outside of applyCopyRowsFunc, so that it doesn't run on retries - add an integration test for Migrator with retry hooks Co-authored-by: Bastian Bartmann <bastian.bartmann@shopify.com> * Add localtest that expects gh-ost to fail on exhausted retries * Rename method * fmt and lint * gofmt * Fix problems when altering a column from `binary` to `varbinary` (github#1628) * Fix binary column trailing zero stripping for non-key columns MySQL's binlog strips trailing 0x00 bytes from binary(N) columns. PR github#915 fixed this for unique key columns only, but the same issue affects all binary columns in INSERT/UPDATE operations. Remove the isUniqueKeyColumn condition so all binary(N) columns are padded to their declared length. Fixes a variation of github#909 where the affected column is not a primary key. * Simplify by removing isUniqueKeyColumn now that it's no longer used. * In convertArg, don't convert binary data to strings. In this case, the input is binary, and the column type is `binary`. So the output should be binary, not text. * fix a lint * Fix 4 trigger handling bugs (github#1626) * fix: remove double-transformation in trigger length validation ValidateGhostTriggerLengthBelowMaxLength was calling GetGhostTriggerName on an already-transformed name, adding the suffix twice. This caused valid trigger names (ghost name <= 64 chars) to be falsely rejected. The caller in inspect.go:627 already transforms the name via GetGhostTriggerName before passing it, so the validation function should check the length as-is. Unit tests updated to reflect the correct call pattern: transform first with GetGhostTriggerName, then validate the result. Added boundary tests for exactly 64 and 65 char names. * fix: return error from trigger creation during atomic cut-over During atomic cut-over, if CreateTriggersOnGhost failed, the error was logged but not returned. The migration continued and completed without triggers, silently losing them. The two-step cut-over (line 793) already correctly returns the error. This aligns the atomic cut-over to do the same. * fix: check trigger name uniqueness per schema, not per table validateGhostTriggersDontExist was filtering by event_object_table, only checking if the ghost trigger name existed on the original table. MySQL trigger names are unique per schema, so a trigger with the same name on any other table would block CREATE TRIGGER but pass validation. Remove the event_object_table filter to check trigger_name + trigger_schema only, matching MySQL's uniqueness constraint. * fix: use parameterized query in GetTriggers to prevent SQL injection GetTriggers used fmt.Sprintf with string interpolation for database and table names, causing SQL syntax errors with special characters and potential SQL injection. Switched to parameterized query with ? placeholders, matching the safe pattern already used in inspect.go:553-559. * test: add regression tests for trigger handling bugs Add two integration tests: - trigger-long-name-validation: verifies 60-char trigger names (64-char ghost name) are not falsely rejected by double-transform - trigger-ghost-name-conflict: verifies validation detects ghost trigger name conflicts on other tables in the same schema * style: gofmt context_test.go --------- Co-authored-by: Yakir Gibraltar <yakir.g@taboola.com> Co-authored-by: meiji163 <meiji163@github.com> * fix update of LastIterationRange values --------- Co-authored-by: Jan Grodowski <jan.grodowski@shopify.com>
…hub#1633) * Fix data loss when PanicOnWarnings encounters duplicate keys on non-migration indexes gh-ost could silently lose data when adding a unique index to a column during a migration, even with the PanicOnWarnings flag enabled. This occurred when: 1. A migration adds a unique index to a column (e.g., email) 2. Rows with duplicate values are inserted into the original table after the bulk copy phase completes (during postponed cutover) 3. These duplicate rows are applied via binlog replay to the ghost table **Expected behavior:** Migration fails with clear error **Actual behavior:** Original rows with duplicate values silently deleted, data lost Original table: id PRIMARY KEY, email (no unique constraint) Ghost table: id PRIMARY KEY, email UNIQUE (being added) Initial state (after bulk copy): - Ghost table: (id=1, email='bob@example.com') (id=2, email='alice@example.com') During postponed cutover: - INSERT (id=3, email='bob@example.com') into original table Binlog replay attempts: - INSERT (id=3, email='bob@example.com') into ghost table - Duplicate email='bob@example.com' (conflicts with id=1) - Row with id=1 silently deleted → DATA LOSS The DMLInsertQueryBuilder used `REPLACE` statements for binlog event replay: ```sql REPLACE INTO ghost_table (id, email) VALUES (3, 'bob@example.com'); REPLACE behavior: - If duplicate PRIMARY KEY: deletes old row, inserts new row (no warning/error) - If duplicate on OTHER unique index: deletes conflicting row, inserts new row - NO warnings or errors generated, so PanicOnWarnings cannot detect the issue The original design assumed REPLACE was needed to handle timing edge cases where binlog replay might process a row before bulk copy, but this caused silent data corruption when other unique indexes had duplicates. Changed DMLInsertQueryBuilder to use INSERT IGNORE instead of REPLACE: INSERT IGNORE INTO ghost_table (id, email) VALUES (3, 'bob@example.com'); INSERT IGNORE behavior: - If duplicate on ANY unique index: skip insert, generate WARNING - Does not delete existing rows Added warning detection to ApplyDMLEventQueries() when PanicOnWarnings is enabled: - Checks SHOW WARNINGS after batch execution - Ignores duplicates on migration unique key (expected - row already copied) - FAILS migration for duplicates on other unique indexes - Transaction rollback ensures no partial state Edge Case: DELETE+INSERT Conversion When an UPDATE modifies the migration unique key (e.g., PRIMARY KEY), gh-ost converts it to DELETE+INSERT within a single transaction: BEGIN; DELETE FROM ghost WHERE id=2; INSERT IGNORE INTO ghost VALUES (3, 'bob@example.com'); COMMIT; If the INSERT encounters a duplicate on a non-migration unique index: - With PanicOnWarnings: Warning detected, transaction rolled back, both DELETE and INSERT undone → no data loss ✓ - Without PanicOnWarnings: DELETE succeeds, INSERT silently skips → data loss. This further reinforces that PanicOnWarnings should default to on. * Address linter issue * Clarify test comment * Consolidate test files * Add execinquery linter comment --------- Co-authored-by: meiji163 <meiji163@github.com>
grodowski
force-pushed
the
sync-upstream-2026-03-10
branch
from
a4971fb to
af128d2
Compare
![binks-code-reviewer[bot]](https://avatars.githubusercontent.com/in/2788732?s=60&v=4){kind=small}
| // Duplicate warnings are formatted differently across mysql versions, hence the optional table name prefix | ||
| migrationUniqueKeyExpression := fmt.Sprintf("for key '(%s\\.)?%s'", this.migrationContext.GetGhostTableName(), this.migrationContext.UniqueKey.NameInGhostTable) | ||
| matched, _ := regexp.MatchString(migrationUniqueKeyExpression, message) | ||
| if strings.Contains(message, "Duplicate entry") && matched { |
There was a problem hiding this comment.
Potential nil-pointer panic in new PanicOnWarnings warning handling
The new PanicOnWarnings path unconditionally dereferences this.migrationContext.UniqueKey.NameInGhostTable when building a regex to ignore expected duplicate warnings. If migrationContext.UniqueKey is nil (misconfiguration, call order, partial initialization, tests/custom integrations), this will panic during binlog apply and can abort the migration mid-flight, leaving intermediate artifacts (ghost table/changelog) and requiring manual cleanup.
|
|
||
| stmt := fmt.Sprintf(` | ||
| replace /* gh-ost %s.%s */ | ||
| insert /* gh-ost %s.%s */ ignore |
There was a problem hiding this comment.
Behavior change: REPLACE → INSERT IGNORE for binlog INSERTs
The DML insert builder changed from REPLACE ... INTO to INSERT ... IGNORE INTO. This is a major semantic shift: REPLACE overwrites on PK/unique conflict (delete+insert) while INSERT IGNORE silently skips conflicting inserts (with warnings). The new warning detection + rollback only applies when PanicOnWarnings is enabled; when disabled, conflicts will be ignored and the ghost table may not reflect the latest source-of-truth state from the binlog stream.
|
🤖 Code Review · #projects-dev-ai for questions ✅ Complete - No issues 📋 History✅ 2 findings → ✅ No issues |
coding-chimp
approved these changes
Mar 11, 2026
…unable to delete .gopath permission denied)
grodowski
force-pushed
the
sync-upstream-2026-03-10
branch
from
f1e2281 to
9fb2676
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
7 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This PR syncs github/gh-ost@master to our fork.
Included changes:
binarytovarbinarygithub/gh-ost#1628