From 7787aa91dc3e6dd3f9c30948f65e6bba4926195b Mon Sep 17 00:00:00 2001
From: Jan Grodowski <jan.grodowski@shopify.com>
Date: Tue, 18 Feb 2025 11:07:41 +0100
Subject: [PATCH 01/14] Execute hook on every batch insert retry

Co-authored-by: Bastian Bartmann <bastian.bartmann@shopify.com>
---
 go/logic/hooks.go    |  6 +++++
 go/logic/migrator.go | 54 +++++++++++++++++++++++++++-----------------
 2 files changed, 39 insertions(+), 21 deletions(-)

diff --git a/go/logic/hooks.go b/go/logic/hooks.go
index 2543f8e9a..68626c9f3 100644
--- a/go/logic/hooks.go
+++ b/go/logic/hooks.go
@@ -28,6 +28,7 @@ const (
 	onInteractiveCommand = "gh-ost-on-interactive-command"
 	onSuccess            = "gh-ost-on-success"
 	onFailure            = "gh-ost-on-failure"
+	onBatchCopyRetry     = "gh-ost-on-batch-copy-retry"
 	onStatus             = "gh-ost-on-status"
 	onStopReplication    = "gh-ost-on-stop-replication"
 	onStartReplication   = "gh-ost-on-start-replication"
@@ -77,6 +78,7 @@ func (this *HooksExecutor) applyEnvironmentVariables(extraVariables ...string) [
 // executeHook executes a command, and sets relevant environment variables
 // combined output & error are printed to the configured writer.
 func (this *HooksExecutor) executeHook(hook string, extraVariables ...string) error {
+	this.migrationContext.Log.Infof("executing hook: %+v", hook)
 	cmd := exec.Command(hook)
 	cmd.Env = this.applyEnvironmentVariables(extraVariables...)
 
@@ -123,6 +125,10 @@ func (this *HooksExecutor) onBeforeRowCopy() error {
 	return this.executeHooks(onBeforeRowCopy)
 }
 
+func (this *HooksExecutor) onBatchCopyRetry() error {
+	return this.executeHooks(onBatchCopyRetry)
+}
+
 func (this *HooksExecutor) onRowCopyComplete() error {
 	return this.executeHooks(onRowCopyComplete)
 }
diff --git a/go/logic/migrator.go b/go/logic/migrator.go
index 09de0977b..5b9c4dd24 100644
--- a/go/logic/migrator.go
+++ b/go/logic/migrator.go
@@ -130,6 +130,18 @@ func (this *Migrator) sleepWhileTrue(operation func() (bool, error)) error {
 	}
 }
 
+func (this *Migrator) retryBatchCopyWithHooks(operation func() error, notFatalHint ...bool) (err error) {
+	wrappedOperation := func() error {
+		if err := operation(); err != nil {
+			this.hooksExecutor.onBatchCopyRetry()
+			return err
+		}
+		return nil
+	}
+
+	return this.retryOperation(wrappedOperation, notFatalHint...)
+}
+
 // retryOperation attempts up to `count` attempts at running given function,
 // exiting as soon as it returns with non-error.
 func (this *Migrator) retryOperation(operation func() error, notFatalHint ...bool) (err error) {
@@ -1232,28 +1244,28 @@ func (this *Migrator) iterateChunks() error {
 			return nil
 		}
 		copyRowsFunc := func() error {
-			if atomic.LoadInt64(&this.rowCopyCompleteFlag) == 1 || atomic.LoadInt64(&hasNoFurtherRangeFlag) == 1 {
-				// Done.
-				// There's another such check down the line
-				return nil
-			}
-
-			// When hasFurtherRange is false, original table might be write locked and CalculateNextIterationRangeEndValues would hangs forever
-
-			hasFurtherRange := false
-			expectedRangeSize := int64(0)
-			if err := this.retryOperation(func() (e error) {
-				hasFurtherRange, expectedRangeSize, e = this.applier.CalculateNextIterationRangeEndValues()
-				return e
-			}); err != nil {
-				return terminateRowIteration(err)
-			}
-			if !hasFurtherRange {
-				atomic.StoreInt64(&hasNoFurtherRangeFlag, 1)
-				return terminateRowIteration(nil)
-			}
 			// Copy task:
 			applyCopyRowsFunc := func() error {
+				if atomic.LoadInt64(&this.rowCopyCompleteFlag) == 1 || atomic.LoadInt64(&hasNoFurtherRangeFlag) == 1 {
+					// Done.
+					// There's another such check down the line
+					return nil
+				}
+
+				// When hasFurtherRange is false, original table might be write locked and CalculateNextIterationRangeEndValues would hangs forever
+
+				hasFurtherRange := false
+				// TODO: figure out how to rewrite this double retry?
+				if err := this.retryOperation(func() (e error) {
+					hasFurtherRange, e = this.applier.CalculateNextIterationRangeEndValues()
+					return e
+				}); err != nil {
+					return terminateRowIteration(err)
+				}
+				if !hasFurtherRange {
+					atomic.StoreInt64(&hasNoFurtherRangeFlag, 1)
+					return terminateRowIteration(nil)
+				}
 				if atomic.LoadInt64(&this.rowCopyCompleteFlag) == 1 {
 					// No need for more writes.
 					// This is the de-facto place where we avoid writing in the event of completed cut-over.
@@ -1286,7 +1298,7 @@ func (this *Migrator) iterateChunks() error {
 				atomic.AddInt64(&this.migrationContext.Iteration, 1)
 				return nil
 			}
-			if err := this.retryOperation(applyCopyRowsFunc); err != nil {
+			if err := this.retryBatchCopyWithHooks(applyCopyRowsFunc); err != nil {
 				return terminateRowIteration(err)
 			}
 			return nil

From 2bc897f06b7d7a7970bacb757e0348f211362a4c Mon Sep 17 00:00:00 2001
From: Jan Grodowski <jan.grodowski@shopify.com>
Date: Tue, 18 Feb 2025 11:45:55 +0100
Subject: [PATCH 02/14] Expose the last error message to the onBatchCopyRetry
 hook

Co-authored-by: Bastian Bartmann <bastian.bartmann@shopify.com>
---
 go/logic/hooks.go    | 5 +++--
 go/logic/migrator.go | 2 +-
 2 files changed, 4 insertions(+), 3 deletions(-)

diff --git a/go/logic/hooks.go b/go/logic/hooks.go
index 68626c9f3..5ba988a53 100644
--- a/go/logic/hooks.go
+++ b/go/logic/hooks.go
@@ -125,8 +125,9 @@ func (this *HooksExecutor) onBeforeRowCopy() error {
 	return this.executeHooks(onBeforeRowCopy)
 }
 
-func (this *HooksExecutor) onBatchCopyRetry() error {
-	return this.executeHooks(onBatchCopyRetry)
+func (this *HooksExecutor) onBatchCopyRetry(errorMessage string) error {
+	v := fmt.Sprintf("GH_OST_LAST_BATCH_COPY_ERROR=%s", errorMessage)
+	return this.executeHooks(onBatchCopyRetry, v)
 }
 
 func (this *HooksExecutor) onRowCopyComplete() error {
diff --git a/go/logic/migrator.go b/go/logic/migrator.go
index 5b9c4dd24..540523454 100644
--- a/go/logic/migrator.go
+++ b/go/logic/migrator.go
@@ -133,7 +133,7 @@ func (this *Migrator) sleepWhileTrue(operation func() (bool, error)) error {
 func (this *Migrator) retryBatchCopyWithHooks(operation func() error, notFatalHint ...bool) (err error) {
 	wrappedOperation := func() error {
 		if err := operation(); err != nil {
-			this.hooksExecutor.onBatchCopyRetry()
+			this.hooksExecutor.onBatchCopyRetry(err.Error())
 			return err
 		}
 		return nil

From 3f8fad4a9ac8b281dbd51dab31749923e89a4230 Mon Sep 17 00:00:00 2001
From: Jan Grodowski <jan.grodowski@shopify.com>
Date: Fri, 21 Feb 2025 15:22:54 +0100
Subject: [PATCH 03/14] 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.
---
 go/logic/migrator.go | 11 +++--------
 1 file changed, 3 insertions(+), 8 deletions(-)

diff --git a/go/logic/migrator.go b/go/logic/migrator.go
index 540523454..72c0ad830 100644
--- a/go/logic/migrator.go
+++ b/go/logic/migrator.go
@@ -1253,14 +1253,9 @@ func (this *Migrator) iterateChunks() error {
 				}
 
 				// When hasFurtherRange is false, original table might be write locked and CalculateNextIterationRangeEndValues would hangs forever
-
-				hasFurtherRange := false
-				// TODO: figure out how to rewrite this double retry?
-				if err := this.retryOperation(func() (e error) {
-					hasFurtherRange, e = this.applier.CalculateNextIterationRangeEndValues()
-					return e
-				}); err != nil {
-					return terminateRowIteration(err)
+				hasFurtherRange, expectedRangeSize, err := this.applier.CalculateNextIterationRangeEndValues()
+				if err != nil {
+					return err // wrapping call will retry
 				}
 				if !hasFurtherRange {
 					atomic.StoreInt64(&hasNoFurtherRangeFlag, 1)

From 4e12b9add7ad89dbac6441152c42dfc567e501be Mon Sep 17 00:00:00 2001
From: Jan Grodowski <jan.grodowski@shopify.com>
Date: Fri, 21 Feb 2025 15:26:08 +0100
Subject: [PATCH 04/14] include dev.yml (temp for Shopify)

---
 dev.yml | 21 +++++++++++++++++++++
 1 file changed, 21 insertions(+)
 create mode 100644 dev.yml

diff --git a/dev.yml b/dev.yml
new file mode 100644
index 000000000..4c28ff349
--- /dev/null
+++ b/dev.yml
@@ -0,0 +1,21 @@
+name: gh-ost
+
+env:
+    TESTCONTAINERS_DOCKER_SOCKET_OVERRIDE: /var/run/docker.sock
+    TESTCONTAINERS_RYUK_DISABLED: "true"
+
+up:
+  - go:
+        version: "1.22.12"
+  - podman
+  - custom:
+        name: Go Dependencies
+        met?: go mod download
+        meet: echo 'go mod failed to download dependencies'; false
+
+commands:
+  test:
+    desc: Run all the tests.
+    run: |
+      export DOCKER_HOST=unix://$(podman machine inspect --format '{{.ConnectionInfo.PodmanSocket.Path}}')
+      script/test

From 2c42692ee545b105cbb6c5e5780fdf7b3489d000 Mon Sep 17 00:00:00 2001
From: Jan Grodowski <jan.grodowski@shopify.com>
Date: Tue, 11 Mar 2025 16:11:14 +0100
Subject: [PATCH 05/14] Update doc/hooks.md

---
 doc/hooks.md | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/doc/hooks.md b/doc/hooks.md
index c1fe59453..08a450358 100644
--- a/doc/hooks.md
+++ b/doc/hooks.md
@@ -49,6 +49,7 @@ The full list of supported hooks is best found in code: [hooks.go](https://githu
 - `gh-ost-on-before-cut-over`
 - `gh-ost-on-success`
 - `gh-ost-on-failure`
+- `gh-ost-on-batch-copy-retry`
 
 ### Context
 
@@ -81,6 +82,7 @@ The following variable are available on particular hooks:
 
 - `GH_OST_COMMAND` is only available in `gh-ost-on-interactive-command`
 - `GH_OST_STATUS` is only available in `gh-ost-on-status`
+- `GH_OST_LAST_BATCH_COPY_ERROR` is only available in `gh-ost-on-batch-copy-retry`
 
 ### Examples
 

From 9e0119e58dd452164404aa9281e4dc3a32626d2a Mon Sep 17 00:00:00 2001
From: Jan Grodowski <jan.grodowski@shopify.com>
Date: Wed, 26 Mar 2025 16:40:30 +0100
Subject: [PATCH 06/14] Remove dev.yml

---
 dev.yml | 21 ---------------------
 1 file changed, 21 deletions(-)
 delete mode 100644 dev.yml

diff --git a/dev.yml b/dev.yml
deleted file mode 100644
index 4c28ff349..000000000
--- a/dev.yml
+++ /dev/null
@@ -1,21 +0,0 @@
-name: gh-ost
-
-env:
-    TESTCONTAINERS_DOCKER_SOCKET_OVERRIDE: /var/run/docker.sock
-    TESTCONTAINERS_RYUK_DISABLED: "true"
-
-up:
-  - go:
-        version: "1.22.12"
-  - podman
-  - custom:
-        name: Go Dependencies
-        met?: go mod download
-        meet: echo 'go mod failed to download dependencies'; false
-
-commands:
-  test:
-    desc: Run all the tests.
-    run: |
-      export DOCKER_HOST=unix://$(podman machine inspect --format '{{.ConnectionInfo.PodmanSocket.Path}}')
-      script/test

From 96f1a2881b2009b3f64ef3225148b2f566cd35a9 Mon Sep 17 00:00:00 2001
From: Jan Grodowski <jan.grodowski@shopify.com>
Date: Thu, 10 Apr 2025 12:59:58 +0200
Subject: [PATCH 07/14] 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>
---
 go/base/context.go        |   7 +++
 go/logic/applier.go       |   4 --
 go/logic/applier_test.go  |   1 +
 go/logic/migrator.go      |   1 +
 go/logic/migrator_test.go | 125 ++++++++++++++++++++++++++++++++++++++
 5 files changed, 134 insertions(+), 4 deletions(-)

diff --git a/go/base/context.go b/go/base/context.go
index ac077076f..de65ceca5 100644
--- a/go/base/context.go
+++ b/go/base/context.go
@@ -584,6 +584,13 @@ func (this *MigrationContext) GetIteration() int64 {
 	return atomic.LoadInt64(&this.Iteration)
 }
 
+func (this *MigrationContext) SetNextIterationRangeValues() {
+	this.MigrationIterationRangeMinValues = this.MigrationIterationRangeMaxValues
+	if this.MigrationIterationRangeMinValues == nil {
+		this.MigrationIterationRangeMinValues = this.MigrationRangeMinValues
+	}
+}
+
 func (this *MigrationContext) MarkPointOfInterest() int64 {
 	this.pointOfInterestTimeMutex.Lock()
 	defer this.pointOfInterestTimeMutex.Unlock()
diff --git a/go/logic/applier.go b/go/logic/applier.go
index 0491aae8d..ba1fca2e8 100644
--- a/go/logic/applier.go
+++ b/go/logic/applier.go
@@ -664,10 +664,6 @@ func (this *Applier) ReadMigrationRangeValues() error {
 // no further chunk to work through, i.e. we're past the last chunk and are done with
 // iterating the range (and this done with copying row chunks)
 func (this *Applier) CalculateNextIterationRangeEndValues() (hasFurtherRange bool, expectedRowCount int64, err error) {
-	this.migrationContext.MigrationIterationRangeMinValues = this.migrationContext.MigrationIterationRangeMaxValues
-	if this.migrationContext.MigrationIterationRangeMinValues == nil {
-		this.migrationContext.MigrationIterationRangeMinValues = this.migrationContext.MigrationRangeMinValues
-	}
 	for i := 0; i < 2; i++ {
 		buildFunc := sql.BuildUniqueKeyRangeEndPreparedQueryViaOffset
 		if i == 1 {
diff --git a/go/logic/applier_test.go b/go/logic/applier_test.go
index d5fa06949..b1baf75e3 100644
--- a/go/logic/applier_test.go
+++ b/go/logic/applier_test.go
@@ -562,6 +562,7 @@ func (suite *ApplierTestSuite) TestPanicOnWarningsInApplyIterationInsertQuerySuc
 	err = applier.ReadMigrationRangeValues()
 	suite.Require().NoError(err)
 
+	migrationContext.SetNextIterationRangeValues()
 	hasFurtherRange, expectedRangeSize, err := applier.CalculateNextIterationRangeEndValues()
 	suite.Require().NoError(err)
 	suite.Require().True(hasFurtherRange)
diff --git a/go/logic/migrator.go b/go/logic/migrator.go
index 72c0ad830..f77d9d9ff 100644
--- a/go/logic/migrator.go
+++ b/go/logic/migrator.go
@@ -1244,6 +1244,7 @@ func (this *Migrator) iterateChunks() error {
 			return nil
 		}
 		copyRowsFunc := func() error {
+			this.migrationContext.SetNextIterationRangeValues()
 			// Copy task:
 			applyCopyRowsFunc := func() error {
 				if atomic.LoadInt64(&this.rowCopyCompleteFlag) == 1 || atomic.LoadInt64(&hasNoFurtherRangeFlag) == 1 {
diff --git a/go/logic/migrator_test.go b/go/logic/migrator_test.go
index 813909208..e0e1d34b7 100644
--- a/go/logic/migrator_test.go
+++ b/go/logic/migrator_test.go
@@ -6,9 +6,12 @@
 package logic
 
 import (
+	"bytes"
 	"context"
 	gosql "database/sql"
 	"errors"
+	"fmt"
+	"io"
 	"os"
 	"path/filepath"
 	"runtime"
@@ -316,6 +319,8 @@ func (suite *MigratorTestSuite) SetupTest() {
 
 	_, err := suite.db.ExecContext(ctx, "CREATE DATABASE test")
 	suite.Require().NoError(err)
+
+	os.Remove("/tmp/gh-ost.sock")
 }
 
 func (suite *MigratorTestSuite) TearDownTest() {
@@ -379,6 +384,126 @@ func (suite *MigratorTestSuite) TestFoo() {
 	suite.Require().Equal("_testing_del", tableName)
 }
 
+func (suite *MigratorTestSuite) TestRetryBatchCopyWithHooks() {
+	ctx := context.Background()
+
+	_, err := suite.db.ExecContext(ctx, "CREATE TABLE test.test_retry_batch (id INT PRIMARY KEY AUTO_INCREMENT, name TEXT)")
+	suite.Require().NoError(err)
+
+	const initStride = 1000
+	const totalBatches = 3
+	for i := 0; i < totalBatches; i++ {
+		dataSize := 50 * i
+		for j := 0; j < initStride; j++ {
+			_, err = suite.db.ExecContext(ctx, fmt.Sprintf("INSERT INTO test.test_retry_batch (name) VALUES ('%s')", strings.Repeat("a", dataSize)))
+			suite.Require().NoError(err)
+		}
+	}
+
+	_, err = suite.db.ExecContext(ctx, fmt.Sprintf("SET GLOBAL max_binlog_cache_size = %d", 1024*8))
+	suite.Require().NoError(err)
+	defer func() {
+		_, err = suite.db.ExecContext(ctx, fmt.Sprintf("SET GLOBAL max_binlog_cache_size = %d", 1024*1024*1024))
+		suite.Require().NoError(err)
+	}()
+
+	tmpDir, err := os.MkdirTemp("", "gh-ost-hooks")
+	suite.Require().NoError(err)
+	defer os.RemoveAll(tmpDir)
+
+	hookScript := filepath.Join(tmpDir, "gh-ost-on-batch-copy-retry")
+	hookContent := `#!/bin/bash
+# Mock hook that reduces chunk size on binlog cache error
+ERROR_MSG="$GH_OST_LAST_BATCH_COPY_ERROR"
+SOCKET_PATH="/tmp/gh-ost.sock"
+
+if ! [[ "$ERROR_MSG" =~ "max_binlog_cache_size" ]]; then
+    echo "Nothing to do for error: $ERROR_MSG"
+    exit 0
+fi
+
+CHUNK_SIZE=$(echo "chunk-size=?" | nc -U $SOCKET_PATH | tr -d '\n')
+
+MIN_CHUNK_SIZE=10
+NEW_CHUNK_SIZE=$(( CHUNK_SIZE * 8 / 10 ))
+if [ $NEW_CHUNK_SIZE -lt $MIN_CHUNK_SIZE ]; then
+    NEW_CHUNK_SIZE=$MIN_CHUNK_SIZE
+fi
+
+if [ $CHUNK_SIZE -eq $NEW_CHUNK_SIZE ]; then
+    echo "Chunk size unchanged: $CHUNK_SIZE"
+    exit 0
+fi
+
+echo "[gh-ost-on-batch-copy-retry]: Changing chunk size from $CHUNK_SIZE to $NEW_CHUNK_SIZE"
+echo "chunk-size=$NEW_CHUNK_SIZE" | nc -U $SOCKET_PATH
+echo "[gh-ost-on-batch-copy-retry]: Done, exiting..."
+`
+	err = os.WriteFile(hookScript, []byte(hookContent), 0755)
+	suite.Require().NoError(err)
+
+	origStdout := os.Stdout
+	origStderr := os.Stderr
+
+	rOut, wOut, _ := os.Pipe()
+	rErr, wErr, _ := os.Pipe()
+	os.Stdout = wOut
+	os.Stderr = wErr
+
+	connectionConfig, err := GetConnectionConfig(ctx, suite.mysqlContainer)
+	suite.Require().NoError(err)
+
+	migrationContext := base.NewMigrationContext()
+	migrationContext.AllowedRunningOnMaster = true
+	migrationContext.ApplierConnectionConfig = connectionConfig
+	migrationContext.InspectorConnectionConfig = connectionConfig
+	migrationContext.DatabaseName = "test"
+	migrationContext.SkipPortValidation = true
+	migrationContext.OriginalTableName = "test_retry_batch"
+	migrationContext.SetConnectionConfig("innodb")
+	migrationContext.AlterStatementOptions = "MODIFY name LONGTEXT, ENGINE=InnoDB"
+	migrationContext.ReplicaServerId = 99999
+	migrationContext.HeartbeatIntervalMilliseconds = 100
+	migrationContext.ThrottleHTTPIntervalMillis = 100
+	migrationContext.ThrottleHTTPTimeoutMillis = 1000
+	migrationContext.HooksPath = tmpDir
+	migrationContext.ChunkSize = 1000
+	migrationContext.SetDefaultNumRetries(10)
+	migrationContext.ServeSocketFile = "/tmp/gh-ost.sock"
+
+	migrator := NewMigrator(migrationContext, "0.0.0")
+
+	err = migrator.Migrate()
+	suite.Require().NoError(err)
+
+	wOut.Close()
+	wErr.Close()
+	os.Stdout = origStdout
+	os.Stderr = origStderr
+
+	var bufOut, bufErr bytes.Buffer
+	io.Copy(&bufOut, rOut)
+	io.Copy(&bufErr, rErr)
+
+	outStr := bufOut.String()
+	errStr := bufErr.String()
+
+	suite.Assert().Contains(outStr, "chunk-size: 1000")
+	suite.Assert().Contains(errStr, "[gh-ost-on-batch-copy-retry]: Changing chunk size from 1000 to 800")
+	suite.Assert().Contains(outStr, "chunk-size: 800")
+
+	suite.Assert().Contains(errStr, "[gh-ost-on-batch-copy-retry]: Changing chunk size from 800 to 640")
+	suite.Assert().Contains(outStr, "chunk-size: 640")
+
+	suite.Assert().Contains(errStr, "[gh-ost-on-batch-copy-retry]: Changing chunk size from 640 to 512")
+	suite.Assert().Contains(outStr, "chunk-size: 512")
+
+	var count int
+	err = suite.db.QueryRowContext(ctx, "SELECT COUNT(*) FROM test.test_retry_batch").Scan(&count)
+	suite.Require().NoError(err)
+	suite.Assert().Equal(3000, count)
+}
+
 func TestMigratorRetry(t *testing.T) {
 	oldRetrySleepFn := RetrySleepFn
 	defer func() { RetrySleepFn = oldRetrySleepFn }()

From a48cae148228e9799caf7b43e30c782e2eac5302 Mon Sep 17 00:00:00 2001
From: Jan Grodowski <jan.grodowski@shopify.com>
Date: Thu, 10 Apr 2025 13:07:58 +0200
Subject: [PATCH 08/14] Add localtest that expects gh-ost to fail on exhausted
 retries

---
 localtests/copy-retries-exhausted/after.sql      |  1 +
 localtests/copy-retries-exhausted/before.sql     |  1 +
 localtests/copy-retries-exhausted/create.sql     | 12 ++++++++++++
 localtests/copy-retries-exhausted/expect_failure |  1 +
 localtests/copy-retries-exhausted/extra_args     |  1 +
 localtests/test.sh                               | 11 +++++++++++
 6 files changed, 27 insertions(+)
 create mode 100644 localtests/copy-retries-exhausted/after.sql
 create mode 100644 localtests/copy-retries-exhausted/before.sql
 create mode 100644 localtests/copy-retries-exhausted/create.sql
 create mode 100644 localtests/copy-retries-exhausted/expect_failure
 create mode 100644 localtests/copy-retries-exhausted/extra_args

diff --git a/localtests/copy-retries-exhausted/after.sql b/localtests/copy-retries-exhausted/after.sql
new file mode 100644
index 000000000..c3d55e5b9
--- /dev/null
+++ b/localtests/copy-retries-exhausted/after.sql
@@ -0,0 +1 @@
+set global max_binlog_cache_size = 1073741824; -- 1GB
diff --git a/localtests/copy-retries-exhausted/before.sql b/localtests/copy-retries-exhausted/before.sql
new file mode 100644
index 000000000..a3570171a
--- /dev/null
+++ b/localtests/copy-retries-exhausted/before.sql
@@ -0,0 +1 @@
+set global max_binlog_cache_size = 1024;
diff --git a/localtests/copy-retries-exhausted/create.sql b/localtests/copy-retries-exhausted/create.sql
new file mode 100644
index 000000000..4e37938ec
--- /dev/null
+++ b/localtests/copy-retries-exhausted/create.sql
@@ -0,0 +1,12 @@
+drop table if exists gh_ost_test;
+create table gh_ost_test (
+  id int auto_increment,
+  name mediumtext not null,
+  primary key (id)
+) auto_increment=1;
+
+insert into gh_ost_test (name)
+select repeat('a', 1500)
+from information_schema.columns
+cross join information_schema.tables
+limit 1000;
diff --git a/localtests/copy-retries-exhausted/expect_failure b/localtests/copy-retries-exhausted/expect_failure
new file mode 100644
index 000000000..cd6a516ec
--- /dev/null
+++ b/localtests/copy-retries-exhausted/expect_failure
@@ -0,0 +1 @@
+Multi-statement transaction required more than 'max_binlog_cache_size' bytes of storage
diff --git a/localtests/copy-retries-exhausted/extra_args b/localtests/copy-retries-exhausted/extra_args
new file mode 100644
index 000000000..e4f8a0104
--- /dev/null
+++ b/localtests/copy-retries-exhausted/extra_args
@@ -0,0 +1 @@
+--alter "modify column name mediumtext" --default-retries=1 --chunk-size=1000
diff --git a/localtests/test.sh b/localtests/test.sh
index e467c113b..3c3d1baf5 100755
--- a/localtests/test.sh
+++ b/localtests/test.sh
@@ -142,6 +142,12 @@ test_single() {
   fi
 
   gh-ost-test-mysql-master --default-character-set=utf8mb4 test < $tests_path/$test_name/create.sql
+
+  if [ -f $tests_path/$test_name/before.sql ]; then
+    gh-ost-test-mysql-master --default-character-set=utf8mb4 test < $tests_path/$test_name/before.sql
+    gh-ost-test-mysql-replica --default-character-set=utf8mb4 test < $tests_path/$test_name/before.sql
+  fi
+
   test_create_result=$?
 
   if [ $test_create_result -ne 0 ] ; then
@@ -208,6 +214,11 @@ test_single() {
     gh-ost-test-mysql-replica --default-character-set=utf8mb4 test -e "set @@global.sql_mode='${original_sql_mode}'"
   fi
 
+  if [ -f $tests_path/$test_name/after.sql ]; then
+    gh-ost-test-mysql-master --default-character-set=utf8mb4 test < $tests_path/$test_name/after.sql
+    gh-ost-test-mysql-replica --default-character-set=utf8mb4 test < $tests_path/$test_name/after.sql
+  fi
+
   if [ -f $tests_path/$test_name/destroy.sql ] ; then
     gh-ost-test-mysql-master --default-character-set=utf8mb4 test < $tests_path/$test_name/destroy.sql
   fi

From 695a1aa6f5821f80e7ec6b35d1033fbed35e1e69 Mon Sep 17 00:00:00 2001
From: Jan Grodowski <jan.grodowski@shopify.com>
Date: Thu, 10 Apr 2025 13:37:55 +0200
Subject: [PATCH 09/14] Rename method

---
 go/base/context.go       | 2 +-
 go/logic/applier_test.go | 2 +-
 go/logic/migrator.go     | 2 +-
 3 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/go/base/context.go b/go/base/context.go
index de65ceca5..e72d628b0 100644
--- a/go/base/context.go
+++ b/go/base/context.go
@@ -584,7 +584,7 @@ func (this *MigrationContext) GetIteration() int64 {
 	return atomic.LoadInt64(&this.Iteration)
 }
 
-func (this *MigrationContext) SetNextIterationRangeValues() {
+func (this *MigrationContext) SetNextIterationRangeMinValues() {
 	this.MigrationIterationRangeMinValues = this.MigrationIterationRangeMaxValues
 	if this.MigrationIterationRangeMinValues == nil {
 		this.MigrationIterationRangeMinValues = this.MigrationRangeMinValues
diff --git a/go/logic/applier_test.go b/go/logic/applier_test.go
index b1baf75e3..d1d7ce329 100644
--- a/go/logic/applier_test.go
+++ b/go/logic/applier_test.go
@@ -562,7 +562,7 @@ func (suite *ApplierTestSuite) TestPanicOnWarningsInApplyIterationInsertQuerySuc
 	err = applier.ReadMigrationRangeValues()
 	suite.Require().NoError(err)
 
-	migrationContext.SetNextIterationRangeValues()
+	migrationContext.SetNextIterationRangeMinValues()
 	hasFurtherRange, expectedRangeSize, err := applier.CalculateNextIterationRangeEndValues()
 	suite.Require().NoError(err)
 	suite.Require().True(hasFurtherRange)
diff --git a/go/logic/migrator.go b/go/logic/migrator.go
index f77d9d9ff..30ab4ca5a 100644
--- a/go/logic/migrator.go
+++ b/go/logic/migrator.go
@@ -1244,7 +1244,7 @@ func (this *Migrator) iterateChunks() error {
 			return nil
 		}
 		copyRowsFunc := func() error {
-			this.migrationContext.SetNextIterationRangeValues()
+			this.migrationContext.SetNextIterationRangeMinValues()
 			// Copy task:
 			applyCopyRowsFunc := func() error {
 				if atomic.LoadInt64(&this.rowCopyCompleteFlag) == 1 || atomic.LoadInt64(&hasNoFurtherRangeFlag) == 1 {

From 512b07228dad504954dfe477cbd4501594ce4c4b Mon Sep 17 00:00:00 2001
From: Jan Grodowski <jan.grodowski@shopify.com>
Date: Wed, 6 Aug 2025 13:19:48 +0200
Subject: [PATCH 10/14] fmt and lint

---
 go/logic/migrator_test.go | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/go/logic/migrator_test.go b/go/logic/migrator_test.go
index c16aa21f8..46435f1d3 100644
--- a/go/logic/migrator_test.go
+++ b/go/logic/migrator_test.go
@@ -10,8 +10,8 @@ import (
 	"context"
 	gosql "database/sql"
 	"errors"
-	"io"
 	"fmt"
+	"io"
 	"os"
 	"path/filepath"
 	"strings"

From 24595e485f1bb579d90b5b4e17ed17dbb871d433 Mon Sep 17 00:00:00 2001
From: Jan Grodowski <jan.grodowski@shopify.com>
Date: Thu, 6 Nov 2025 18:47:04 +0100
Subject: [PATCH 11/14] gofmt

---
 go/logic/applier.go | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/go/logic/applier.go b/go/logic/applier.go
index 4c7579948..5a87c1560 100644
--- a/go/logic/applier.go
+++ b/go/logic/applier.go
@@ -824,7 +824,7 @@ func (this *Applier) CalculateNextIterationRangeEndValues() (hasFurtherRange boo
 		this.LastIterationRangeMaxValues = this.migrationContext.MigrationIterationRangeMaxValues.Clone()
 	}
 	this.LastIterationRangeMutex.Unlock()
-  
+
 	for i := 0; i < 2; i++ {
 		buildFunc := sql.BuildUniqueKeyRangeEndPreparedQueryViaOffset
 		if i == 1 {

From 34f4da3552eca41a559bc62f2899d843f518fea7 Mon Sep 17 00:00:00 2001
From: Jason Orendorff <jorendorff@github.com>
Date: Tue, 10 Feb 2026 11:45:15 -0600
Subject: [PATCH 12/14] 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
---
 go/logic/applier.go                       |  5 +--
 go/sql/builder.go                         | 24 +++++------
 go/sql/builder_test.go                    | 35 ++++++---------
 go/sql/types.go                           |  6 +--
 go/sql/types_test.go                      | 52 ++++++++++++++++++++++-
 localtests/binary-to-varbinary/create.sql | 38 +++++++++++++++++
 localtests/binary-to-varbinary/extra_args |  1 +
 7 files changed, 120 insertions(+), 41 deletions(-)
 create mode 100644 localtests/binary-to-varbinary/create.sql
 create mode 100644 localtests/binary-to-varbinary/extra_args

diff --git a/go/logic/applier.go b/go/logic/applier.go
index d6e7da175..3d486fc62 100644
--- a/go/logic/applier.go
+++ b/go/logic/applier.go
@@ -1466,10 +1466,9 @@ func (this *Applier) buildDMLEventQuery(dmlEvent *binlog.BinlogDMLEvent) []*dmlB
 				results = append(results, this.buildDMLEventQuery(dmlEvent)...)
 				return results
 			}
-			query, sharedArgs, uniqueKeyArgs, err := this.dmlUpdateQueryBuilder.BuildQuery(dmlEvent.NewColumnValues.AbstractValues(), dmlEvent.WhereColumnValues.AbstractValues())
+			query, updateArgs, err := this.dmlUpdateQueryBuilder.BuildQuery(dmlEvent.NewColumnValues.AbstractValues(), dmlEvent.WhereColumnValues.AbstractValues())
 			args := sqlutils.Args()
-			args = append(args, sharedArgs...)
-			args = append(args, uniqueKeyArgs...)
+			args = append(args, updateArgs...)
 			return []*dmlBuildResult{newDmlBuildResult(query, args, 0, err)}
 		}
 	}
diff --git a/go/sql/builder.go b/go/sql/builder.go
index 61dd9706f..0bb2d5e6a 100644
--- a/go/sql/builder.go
+++ b/go/sql/builder.go
@@ -169,11 +169,11 @@ func (b *CheckpointInsertQueryBuilder) BuildQuery(uniqueKeyArgs []interface{}) (
 	}
 	convertedArgs := make([]interface{}, 0, 2*b.uniqueKeyColumns.Len())
 	for i, column := range b.uniqueKeyColumns.Columns() {
-		minArg := column.convertArg(uniqueKeyArgs[i], true)
+		minArg := column.convertArg(uniqueKeyArgs[i])
 		convertedArgs = append(convertedArgs, minArg)
 	}
 	for i, column := range b.uniqueKeyColumns.Columns() {
-		minArg := column.convertArg(uniqueKeyArgs[i+b.uniqueKeyColumns.Len()], true)
+		minArg := column.convertArg(uniqueKeyArgs[i+b.uniqueKeyColumns.Len()])
 		convertedArgs = append(convertedArgs, minArg)
 	}
 	return b.preparedStatement, convertedArgs, nil
@@ -533,7 +533,7 @@ func (b *DMLDeleteQueryBuilder) BuildQuery(args []interface{}) (string, []interf
 	uniqueKeyArgs := make([]interface{}, 0, b.uniqueKeyColumns.Len())
 	for _, column := range b.uniqueKeyColumns.Columns() {
 		tableOrdinal := b.tableColumns.Ordinals[column.Name]
-		arg := column.convertArg(args[tableOrdinal], true)
+		arg := column.convertArg(args[tableOrdinal])
 		uniqueKeyArgs = append(uniqueKeyArgs, arg)
 	}
 	return b.preparedStatement, uniqueKeyArgs, nil
@@ -595,7 +595,7 @@ func (b *DMLInsertQueryBuilder) BuildQuery(args []interface{}) (string, []interf
 	sharedArgs := make([]interface{}, 0, b.sharedColumns.Len())
 	for _, column := range b.sharedColumns.Columns() {
 		tableOrdinal := b.tableColumns.Ordinals[column.Name]
-		arg := column.convertArg(args[tableOrdinal], false)
+		arg := column.convertArg(args[tableOrdinal])
 		sharedArgs = append(sharedArgs, arg)
 	}
 	return b.preparedStatement, sharedArgs, nil
@@ -661,20 +661,18 @@ func NewDMLUpdateQueryBuilder(databaseName, tableName string, tableColumns, shar
 
 // BuildQuery builds the arguments array for a DML event UPDATE query.
 // It returns the query string, the shared arguments array, and the unique key arguments array.
-func (b *DMLUpdateQueryBuilder) BuildQuery(valueArgs, whereArgs []interface{}) (string, []interface{}, []interface{}, error) {
-	sharedArgs := make([]interface{}, 0, b.sharedColumns.Len())
+func (b *DMLUpdateQueryBuilder) BuildQuery(valueArgs, whereArgs []interface{}) (string, []interface{}, error) {
+	args := make([]interface{}, 0, b.sharedColumns.Len()+b.uniqueKeyColumns.Len())
 	for _, column := range b.sharedColumns.Columns() {
 		tableOrdinal := b.tableColumns.Ordinals[column.Name]
-		arg := column.convertArg(valueArgs[tableOrdinal], false)
-		sharedArgs = append(sharedArgs, arg)
+		arg := column.convertArg(valueArgs[tableOrdinal])
+		args = append(args, arg)
 	}
-
-	uniqueKeyArgs := make([]interface{}, 0, b.uniqueKeyColumns.Len())
 	for _, column := range b.uniqueKeyColumns.Columns() {
 		tableOrdinal := b.tableColumns.Ordinals[column.Name]
-		arg := column.convertArg(whereArgs[tableOrdinal], true)
-		uniqueKeyArgs = append(uniqueKeyArgs, arg)
+		arg := column.convertArg(whereArgs[tableOrdinal])
+		args = append(args, arg)
 	}
 
-	return b.preparedStatement, sharedArgs, uniqueKeyArgs, nil
+	return b.preparedStatement, args, nil
 }
diff --git a/go/sql/builder_test.go b/go/sql/builder_test.go
index 840d85c96..15c21d5fa 100644
--- a/go/sql/builder_test.go
+++ b/go/sql/builder_test.go
@@ -647,7 +647,7 @@ func TestBuildDMLUpdateQuery(t *testing.T) {
 		uniqueKeyColumns := NewColumnList([]string{"position"})
 		builder, err := NewDMLUpdateQueryBuilder(databaseName, tableName, tableColumns, sharedColumns, sharedColumns, uniqueKeyColumns)
 		require.NoError(t, err)
-		query, sharedArgs, uniqueKeyArgs, err := builder.BuildQuery(valueArgs, whereArgs)
+		query, updateArgs, err := builder.BuildQuery(valueArgs, whereArgs)
 		require.NoError(t, err)
 		expected := `
 			update /* gh-ost mydb.tbl */
@@ -657,15 +657,14 @@ func TestBuildDMLUpdateQuery(t *testing.T) {
 					((position = ?))
 		`
 		require.Equal(t, normalizeQuery(expected), normalizeQuery(query))
-		require.Equal(t, []interface{}{3, "testname", 17, 23}, sharedArgs)
-		require.Equal(t, []interface{}{17}, uniqueKeyArgs)
+		require.Equal(t, []interface{}{3, "testname", 17, 23, 17}, updateArgs)
 	}
 	{
 		sharedColumns := NewColumnList([]string{"id", "name", "position", "age"})
 		uniqueKeyColumns := NewColumnList([]string{"position", "name"})
 		builder, err := NewDMLUpdateQueryBuilder(databaseName, tableName, tableColumns, sharedColumns, sharedColumns, uniqueKeyColumns)
 		require.NoError(t, err)
-		query, sharedArgs, uniqueKeyArgs, err := builder.BuildQuery(valueArgs, whereArgs)
+		query, updateArgs, err := builder.BuildQuery(valueArgs, whereArgs)
 		require.NoError(t, err)
 		expected := `
 			update /* gh-ost mydb.tbl */
@@ -675,15 +674,14 @@ func TestBuildDMLUpdateQuery(t *testing.T) {
 					((position = ?) and (name = ?))
 		`
 		require.Equal(t, normalizeQuery(expected), normalizeQuery(query))
-		require.Equal(t, []interface{}{3, "testname", 17, 23}, sharedArgs)
-		require.Equal(t, []interface{}{17, "testname"}, uniqueKeyArgs)
+		require.Equal(t, []interface{}{3, "testname", 17, 23, 17, "testname"}, updateArgs)
 	}
 	{
 		sharedColumns := NewColumnList([]string{"id", "name", "position", "age"})
 		uniqueKeyColumns := NewColumnList([]string{"age"})
 		builder, err := NewDMLUpdateQueryBuilder(databaseName, tableName, tableColumns, sharedColumns, sharedColumns, uniqueKeyColumns)
 		require.NoError(t, err)
-		query, sharedArgs, uniqueKeyArgs, err := builder.BuildQuery(valueArgs, whereArgs)
+		query, updateArgs, err := builder.BuildQuery(valueArgs, whereArgs)
 		require.NoError(t, err)
 		expected := `
 			update /* gh-ost mydb.tbl */
@@ -693,15 +691,14 @@ func TestBuildDMLUpdateQuery(t *testing.T) {
 					((age = ?))
 		`
 		require.Equal(t, normalizeQuery(expected), normalizeQuery(query))
-		require.Equal(t, []interface{}{3, "testname", 17, 23}, sharedArgs)
-		require.Equal(t, []interface{}{56}, uniqueKeyArgs)
+		require.Equal(t, []interface{}{3, "testname", 17, 23, 56}, updateArgs)
 	}
 	{
 		sharedColumns := NewColumnList([]string{"id", "name", "position", "age"})
 		uniqueKeyColumns := NewColumnList([]string{"age", "position", "id", "name"})
 		builder, err := NewDMLUpdateQueryBuilder(databaseName, tableName, tableColumns, sharedColumns, sharedColumns, uniqueKeyColumns)
 		require.NoError(t, err)
-		query, sharedArgs, uniqueKeyArgs, err := builder.BuildQuery(valueArgs, whereArgs)
+		query, updateArgs, err := builder.BuildQuery(valueArgs, whereArgs)
 		require.NoError(t, err)
 		expected := `
 			update /* gh-ost mydb.tbl */
@@ -711,8 +708,7 @@ func TestBuildDMLUpdateQuery(t *testing.T) {
 					((age = ?) and (position = ?) and (id = ?) and (name = ?))
 		`
 		require.Equal(t, normalizeQuery(expected), normalizeQuery(query))
-		require.Equal(t, []interface{}{3, "testname", 17, 23}, sharedArgs)
-		require.Equal(t, []interface{}{56, 17, 3, "testname"}, uniqueKeyArgs)
+		require.Equal(t, []interface{}{3, "testname", 17, 23, 56, 17, 3, "testname"}, updateArgs)
 	}
 	{
 		sharedColumns := NewColumnList([]string{"id", "name", "position", "age"})
@@ -732,7 +728,7 @@ func TestBuildDMLUpdateQuery(t *testing.T) {
 		uniqueKeyColumns := NewColumnList([]string{"id"})
 		builder, err := NewDMLUpdateQueryBuilder(databaseName, tableName, tableColumns, sharedColumns, mappedColumns, uniqueKeyColumns)
 		require.NoError(t, err)
-		query, sharedArgs, uniqueKeyArgs, err := builder.BuildQuery(valueArgs, whereArgs)
+		query, updateArgs, err := builder.BuildQuery(valueArgs, whereArgs)
 		require.NoError(t, err)
 		expected := `
 			update /* gh-ost mydb.tbl */
@@ -742,8 +738,7 @@ func TestBuildDMLUpdateQuery(t *testing.T) {
 					((id = ?))
 		`
 		require.Equal(t, normalizeQuery(expected), normalizeQuery(query))
-		require.Equal(t, []interface{}{3, "testname", 17, 23}, sharedArgs)
-		require.Equal(t, []interface{}{3}, uniqueKeyArgs)
+		require.Equal(t, []interface{}{3, "testname", 17, 23, 3}, updateArgs)
 	}
 }
 
@@ -759,7 +754,7 @@ func TestBuildDMLUpdateQuerySignedUnsigned(t *testing.T) {
 	require.NoError(t, err)
 	{
 		// test signed
-		query, sharedArgs, uniqueKeyArgs, err := builder.BuildQuery(valueArgs, whereArgs)
+		query, updateArgs, err := builder.BuildQuery(valueArgs, whereArgs)
 		require.NoError(t, err)
 		expected := `
 			update /* gh-ost mydb.tbl */
@@ -769,14 +764,13 @@ func TestBuildDMLUpdateQuerySignedUnsigned(t *testing.T) {
 					((position = ?))
 		`
 		require.Equal(t, normalizeQuery(expected), normalizeQuery(query))
-		require.Equal(t, []interface{}{3, "testname", int8(-17), int8(-2)}, sharedArgs)
-		require.Equal(t, []interface{}{int8(-3)}, uniqueKeyArgs)
+		require.Equal(t, []interface{}{3, "testname", int8(-17), int8(-2), int8(-3)}, updateArgs)
 	}
 	{
 		// test unsigned
 		sharedColumns.SetUnsigned("age")
 		uniqueKeyColumns.SetUnsigned("position")
-		query, sharedArgs, uniqueKeyArgs, err := builder.BuildQuery(valueArgs, whereArgs)
+		query, updateArgs, err := builder.BuildQuery(valueArgs, whereArgs)
 		require.NoError(t, err)
 		expected := `
 			update /* gh-ost mydb.tbl */
@@ -786,8 +780,7 @@ func TestBuildDMLUpdateQuerySignedUnsigned(t *testing.T) {
 					((position = ?))
 		`
 		require.Equal(t, normalizeQuery(expected), normalizeQuery(query))
-		require.Equal(t, []interface{}{3, "testname", int8(-17), uint8(254)}, sharedArgs)
-		require.Equal(t, []interface{}{uint8(253)}, uniqueKeyArgs)
+		require.Equal(t, []interface{}{3, "testname", int8(-17), uint8(254), uint8(253)}, updateArgs)
 	}
 }
 
diff --git a/go/sql/types.go b/go/sql/types.go
index a01fb8bff..9a50bc620 100644
--- a/go/sql/types.go
+++ b/go/sql/types.go
@@ -57,7 +57,7 @@ type Column struct {
 	MySQLType         string
 }
 
-func (this *Column) convertArg(arg interface{}, isUniqueKeyColumn bool) interface{} {
+func (this *Column) convertArg(arg interface{}) interface{} {
 	var arg2Bytes []byte
 	if s, ok := arg.(string); ok {
 		arg2Bytes = []byte(s)
@@ -77,14 +77,14 @@ func (this *Column) convertArg(arg interface{}, isUniqueKeyColumn bool) interfac
 			}
 		}
 
-		if this.Type == BinaryColumnType && isUniqueKeyColumn {
+		if this.Type == BinaryColumnType {
 			size := len(arg2Bytes)
 			if uint(size) < this.BinaryOctetLength {
 				buf := bytes.NewBuffer(arg2Bytes)
 				for i := uint(0); i < (this.BinaryOctetLength - uint(size)); i++ {
 					buf.Write([]byte{0})
 				}
-				arg = buf.String()
+				arg = buf.Bytes()
 			}
 		}
 
diff --git a/go/sql/types_test.go b/go/sql/types_test.go
index 7b808e64f..83c74073a 100644
--- a/go/sql/types_test.go
+++ b/go/sql/types_test.go
@@ -62,6 +62,56 @@ func TestConvertArgCharsetDecoding(t *testing.T) {
 	}
 
 	// Should decode []uint8
-	str := col.convertArg(latin1Bytes, false)
+	str := col.convertArg(latin1Bytes)
 	require.Equal(t, "Garçon !", str)
 }
+
+func TestConvertArgBinaryColumnPadding(t *testing.T) {
+	// Test that binary columns are padded with trailing zeros to their declared length.
+	// This is needed because MySQL's binlog strips trailing 0x00 bytes from binary values.
+	// See https://github.com/github/gh-ost/issues/909
+
+	// Simulates a binary(20) column where binlog delivered only 18 bytes
+	// (trailing zeros were stripped)
+	truncatedValue := []uint8{
+		0x01, 0x02, 0x03, 0x04, 0x05, 0x06, 0x07, 0x08,
+		0x09, 0x0a, 0x0b, 0x0c, 0x0d, 0x0e, 0x0f, 0x10,
+		0x11, 0x12, // 18 bytes, missing 2 trailing zeros
+	}
+
+	col := Column{
+		Name:              "bin_col",
+		Type:              BinaryColumnType,
+		BinaryOctetLength: 20,
+	}
+
+	result := col.convertArg(truncatedValue)
+	resultBytes := result.([]byte)
+
+	require.Equal(t, 20, len(resultBytes), "binary column should be padded to declared length")
+	// First 18 bytes should be unchanged
+	require.Equal(t, truncatedValue, resultBytes[:18])
+	// Last 2 bytes should be zeros
+	require.Equal(t, []byte{0x00, 0x00}, resultBytes[18:])
+}
+
+func TestConvertArgBinaryColumnNoPaddingWhenFull(t *testing.T) {
+	// When binary value is already at full length, no padding should occur
+	fullValue := []uint8{
+		0x01, 0x02, 0x03, 0x04, 0x05, 0x06, 0x07, 0x08,
+		0x09, 0x0a, 0x0b, 0x0c, 0x0d, 0x0e, 0x0f, 0x10,
+		0x11, 0x12, 0x13, 0x14, // 20 bytes
+	}
+
+	col := Column{
+		Name:              "bin_col",
+		Type:              BinaryColumnType,
+		BinaryOctetLength: 20,
+	}
+
+	result := col.convertArg(fullValue)
+	resultBytes := result.([]byte)
+
+	require.Equal(t, 20, len(resultBytes))
+	require.Equal(t, fullValue, resultBytes)
+}
diff --git a/localtests/binary-to-varbinary/create.sql b/localtests/binary-to-varbinary/create.sql
new file mode 100644
index 000000000..c2867f9ea
--- /dev/null
+++ b/localtests/binary-to-varbinary/create.sql
@@ -0,0 +1,38 @@
+-- Test for https://github.com/github/gh-ost/issues/909 variant:
+-- Binary columns with trailing zeros should preserve their values
+-- when migrating from binary(N) to varbinary(M), even for rows
+-- modified during migration via binlog events.
+
+drop table if exists gh_ost_test;
+create table gh_ost_test (
+  id int NOT NULL AUTO_INCREMENT,
+  info varchar(255) NOT NULL,
+  data binary(20) NOT NULL,
+  PRIMARY KEY (id)
+) auto_increment=1;
+
+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 rows where data has trailing zeros (will be stripped by binlog)
+  INSERT INTO gh_ost_test (info, data) VALUES ('insert-during-1', X'aabbccdd00000000000000000000000000000000');
+  INSERT INTO gh_ost_test (info, data) VALUES ('insert-during-2', X'11223344556677889900000000000000000000ee');
+
+  -- Update existing rows to values with trailing zeros
+  UPDATE gh_ost_test SET data = X'ffeeddcc00000000000000000000000000000000' WHERE info = 'update-target-1';
+  UPDATE gh_ost_test SET data = X'aabbccdd11111111111111111100000000000000' WHERE info = 'update-target-2';
+end ;;
+
+-- Pre-existing rows (copied via rowcopy, not binlog - these should work fine)
+INSERT INTO gh_ost_test (info, data) VALUES
+  ('pre-existing-1', X'01020304050607080910111213141516171819ff'),
+  ('pre-existing-2', X'0102030405060708091011121314151617181900'),
+  ('update-target-1', X'ffffffffffffffffffffffffffffffffffffffff'),
+  ('update-target-2', X'eeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeee');
diff --git a/localtests/binary-to-varbinary/extra_args b/localtests/binary-to-varbinary/extra_args
new file mode 100644
index 000000000..7ebdacf6c
--- /dev/null
+++ b/localtests/binary-to-varbinary/extra_args
@@ -0,0 +1 @@
+--alter="MODIFY data varbinary(32)"

From b243eae23437657f97ba3e2370b08f1a9c61c56e Mon Sep 17 00:00:00 2001
From: Yakir Gibraltar <yakirgb@gmail.com>
Date: Tue, 10 Feb 2026 20:01:11 +0200
Subject: [PATCH 13/14] 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>
---
 go/base/context.go                            |  7 ++-
 go/base/context_test.go                       | 43 ++++++++++++++++---
 go/logic/inspect.go                           |  3 +-
 go/logic/migrator.go                          |  2 +-
 go/mysql/utils.go                             |  8 ++--
 .../trigger-ghost-name-conflict/create.sql    | 35 +++++++++++++++
 .../trigger-ghost-name-conflict/destroy.sql   |  2 +
 .../expect_failure                            |  1 +
 .../trigger-ghost-name-conflict/extra_args    |  1 +
 .../trigger-long-name-validation/create.sql   | 38 ++++++++++++++++
 .../trigger-long-name-validation/extra_args   |  1 +
 11 files changed, 124 insertions(+), 17 deletions(-)
 create mode 100644 localtests/trigger-ghost-name-conflict/create.sql
 create mode 100644 localtests/trigger-ghost-name-conflict/destroy.sql
 create mode 100644 localtests/trigger-ghost-name-conflict/expect_failure
 create mode 100644 localtests/trigger-ghost-name-conflict/extra_args
 create mode 100644 localtests/trigger-long-name-validation/create.sql
 create mode 100644 localtests/trigger-long-name-validation/extra_args

diff --git a/go/base/context.go b/go/base/context.go
index df431a004..891e27fef 100644
--- a/go/base/context.go
+++ b/go/base/context.go
@@ -977,9 +977,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 460e0a57b..0a982b6c3 100644
--- a/go/logic/migrator.go
+++ b/go/logic/migrator.go
@@ -854,7 +854,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

From fae34318f75a1c2540821b6fd90cf573b02763d9 Mon Sep 17 00:00:00 2001
From: meiji163 <meiji163@github.com>
Date: Thu, 5 Mar 2026 11:12:57 -0800
Subject: [PATCH 14/14] fix update of LastIterationRange values

---
 go/logic/applier.go  | 7 -------
 go/logic/migrator.go | 9 +++++++++
 2 files changed, 9 insertions(+), 7 deletions(-)

diff --git a/go/logic/applier.go b/go/logic/applier.go
index 3d486fc62..4e9d74cd2 100644
--- a/go/logic/applier.go
+++ b/go/logic/applier.go
@@ -819,13 +819,6 @@ func (this *Applier) ReadMigrationRangeValues() error {
 // no further chunk to work through, i.e. we're past the last chunk and are done with
 // iterating the range (and thus done with copying row chunks)
 func (this *Applier) CalculateNextIterationRangeEndValues() (hasFurtherRange bool, err error) {
-	this.LastIterationRangeMutex.Lock()
-	if this.migrationContext.MigrationIterationRangeMinValues != nil && this.migrationContext.MigrationIterationRangeMaxValues != nil {
-		this.LastIterationRangeMinValues = this.migrationContext.MigrationIterationRangeMinValues.Clone()
-		this.LastIterationRangeMaxValues = this.migrationContext.MigrationIterationRangeMaxValues.Clone()
-	}
-	this.LastIterationRangeMutex.Unlock()
-
 	for i := 0; i < 2; i++ {
 		buildFunc := sql.BuildUniqueKeyRangeEndPreparedQueryViaOffset
 		if i == 1 {
diff --git a/go/logic/migrator.go b/go/logic/migrator.go
index 0a982b6c3..aa9a97c1c 100644
--- a/go/logic/migrator.go
+++ b/go/logic/migrator.go
@@ -1470,6 +1470,15 @@ func (this *Migrator) iterateChunks() error {
 			if err := this.retryBatchCopyWithHooks(applyCopyRowsFunc); err != nil {
 				return terminateRowIteration(err)
 			}
+
+			// record last successfully copied range
+			this.applier.LastIterationRangeMutex.Lock()
+			if this.migrationContext.MigrationIterationRangeMinValues != nil && this.migrationContext.MigrationIterationRangeMaxValues != nil {
+				this.applier.LastIterationRangeMinValues = this.migrationContext.MigrationIterationRangeMinValues.Clone()
+				this.applier.LastIterationRangeMaxValues = this.migrationContext.MigrationIterationRangeMaxValues.Clone()
+			}
+			this.applier.LastIterationRangeMutex.Unlock()
+
 			return nil
 		}
 		// Enqueue copy operation; to be executed by executeWriteFuncs()