fix: issue 909 by cenkore · Pull Request #915 · github/gh-ost

added 2 commits

January 13, 2021 16:29

@pomelo2016

@rashiq rashiq temporarily deployed to production/mysql_role=ghost_testing

January 19, 2021 17:02

Inactive

shlomi-noach

shlomi-noach

@pomelo2016

@pomelo2016

shlomi-noach

@rashiq

@rashiq rashiq temporarily deployed to production/mysql_role=ghost_testing

March 8, 2021 19:20

Inactive

jorendorff added a commit to jorendorff/gh-ost that referenced this pull request

Feb 9, 2026
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.

jorendorff added a commit to jorendorff/gh-ost that referenced this pull request

Feb 9, 2026
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.

meiji163 pushed a commit that referenced this pull request

Feb 10, 2026
* Fix binary column trailing zero stripping for non-key columns

MySQL's binlog strips trailing 0x00 bytes from binary(N) columns.
PR #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 #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

meiji163 pushed a commit that referenced this pull request

Feb 23, 2026
* Fix binary column trailing zero stripping for non-key columns

MySQL's binlog strips trailing 0x00 bytes from binary(N) columns.
PR #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 #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

meiji163 added a commit that referenced this pull request

Mar 5, 2026
* 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` (#1628)

* Fix binary column trailing zero stripping for non-key columns

MySQL's binlog strips trailing 0x00 bytes from binary(N) columns.
PR #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 #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 (#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>