Fix data loss when inserting duplicate values during a migration by ggilder · Pull Request #1633 · github/gh-ost

ggilder · 2026-02-28T06:22:15Z

Related issue: #1636

Description

This PR fixes a critical data loss bug in gh-ost's --panic-on-warnings mode that could silently delete rows when adding unique indexes to columns with duplicate values during binlog replay.

Problem

gh-ost could silently lose data when migrations add unique indexes to columns, even with --panic-on-warnings enabled. This occurred due to the use of REPLACE INTO for binlog event replay, which silently deletes conflicting rows instead of generating warnings.

Example Scenario

Migration: Add unique index to email column

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 applies to ghost table: REPLACE INTO ghost_table VALUES (3, 'bob@example.com')
Duplicate email='bob@example.com' conflicts with existing row id=1
Row with id=1 silently deleted → DATA LOSS

Root Cause

The REPLACE statement behavior:

Deletes conflicting rows on ANY unique index violation
Does NOT generate warnings or errors

Solution

Replace REPLACE with INSERT IGNORE + Warning Detection

Changed DML replay from:

REPLACE INTO ghost_table (id, email) VALUES (3, 'bob@example.com');

To:

INSERT IGNORE INTO ghost_table (id, email) VALUES (3, 'bob@example.com');

INSERT IGNORE behavior:

Skips insert on ANY unique index violation
Generates WARNING (detectable by PanicOnWarnings)
Does NOT delete existing rows

Added warning detection in ApplyDMLEventQueries() when PanicOnWarnings is enabled:

Executes SHOW WARNINGS after batch execution
Ignores duplicates on migration unique key (expected - row already copied in bulk phase)
Fails migration for duplicates on other unique indexes
Transaction rollback ensures no partial state

contributed code is using same conventions as original code
script/cibuild returns with no formatting errors, build errors or unit test errors.

(edited to remove references to Fatale/panic behavior, this will be tackled in a separate PR)

Copilot

Pull request overview

This PR addresses a critical correctness issue in gh-ost binlog replay when adding unique indexes by switching DML replay semantics to avoid REPLACE-driven row deletion, and adds warning-based failure behavior for PanicOnWarnings during replay (plus a change intended to make panic-aborts recoverable for library usage).

Changes:

Switch DML insert replay from REPLACE to INSERT IGNORE (SQL builder + tests).
Add SHOW WARNINGS detection in ApplyDMLEventQueries() to fail migrations on unexpected warnings when PanicOnWarnings is enabled.
Change panic-abort handling from os.Exit-style fatal logging to panic(err) and add regression tests for duplicate/warning scenarios.

Reviewed changes

Copilot reviewed 6 out of 6 changed files in this pull request and generated 6 comments.

Show a summary per file

File	Description
go/sql/builder.go	Updates generated DML insert statement from `REPLACE` to `INSERT IGNORE`.
go/sql/builder_test.go	Updates expectations for the new insert statement form.
go/logic/applier.go	Adds warning detection/rollback path for replay when `PanicOnWarnings` is enabled.
go/logic/applier_test.go	Adds integration test ensuring duplicate on non-migration unique index triggers failure without data loss.
go/logic/applier_delete_insert_test.go	Adds integration tests around UPDATE→DELETE+INSERT behavior with `INSERT IGNORE` + warnings.
go/logic/migrator.go	Changes panic-abort handler from fatal exit to `panic(err)`.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

go/logic/migrator.go

go/logic/applier_delete_insert_test.go

go/logic/applier.go

Copilot · 2026-03-02T19:59:31Z

go/logic/applier.go

+			var sqlWarnings []string
+			for rows.Next() {
+				var level, message string
+				var code int
+				if err := rows.Scan(&level, &code, &message); err != nil {


rows.Err() is checked before iterating, but errors that occur during iteration will only surface after the loop. Add a rows.Err() check after the for rows.Next() loop so iteration errors don't get ignored in PanicOnWarnings mode.

Also copied from existing code — may be worth fixing in both places

go/logic/applier.go

meiji163 · 2026-03-03T19:51:12Z

Thanks for the PR @ggilder. This was a known issue when @grodowski introduced --panic-on-warnings. Unfortunately we can't use INSERT IGNORE instead of REPLACE INTO because the binlog applier must overwrite rows that were copied to the ghost table. The binlog applier is responsible for inserting the latest version of the row, while INSERT IGNORE would keep the old one

ggilder · 2026-03-04T19:13:55Z

@meiji163 I'm not sure I understand the problem with INSERT IGNORE, can you elaborate with an example scenario where INSERT IGNORE would be incorrect? My reading of the code is that, with both bulk copy and binlog replay using INSERT IGNORE, we would have correct handling of inserts and updates that occur during the migration:

Scenario 1: Binlog event first, then bulk copy
T1: INSERT id=1, 'Alice' → binlog event
T2: Binlog replay: INSERT IGNORE id=1, 'Alice' → inserts
T3: Bulk copy: INSERT IGNORE (SELECT id=1, 'Alice' ...) → skips
Result: 'Alice'

Scenario 2: Bulk copy first, then binlog event
T1: INSERT id=1, 'Alice' → binlog event captured
T2: Bulk copy: INSERT IGNORE (SELECT id=1, 'Alice' ...) → inserts
T3: Binlog replay: INSERT IGNORE id=1, 'Alice' → skips
Result: 'Alice'

Scenario 3: With updates
T1: INSERT id=1, 'Alice' → binlog Q1
T2: UPDATE id=1 to 'Bob' → binlog Q2
T3: Bulk copy sees current state: INSERT IGNORE id=1, 'Bob' → inserts
T4: Binlog replay Q1: INSERT IGNORE id=1, 'Alice' → skips (id=1 exists)
T5: Binlog replay Q2: UPDATE id=1 to 'Bob' → no-op (already 'Bob')
Result: 'Bob'

…igration 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.

ggilder · 2026-03-04T19:21:46Z

I'm removing the code changes to replace Fatale with panic since they weren't quite correct and are not related to the data loss issue. Will tackle that in a separate PR.

Copilot

Pull request overview

Copilot reviewed 5 out of 5 changed files in this pull request and generated 4 comments.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

You can also share your feedback on Copilot code review. Take the survey.

go/logic/applier_delete_insert_test.go

Copilot · 2026-03-04T19:28:59Z

go/logic/applier.go

+		// Check for warnings when PanicOnWarnings is enabled
+		if this.migrationContext.PanicOnWarnings {
+			rows, err := tx.Query("SHOW WARNINGS")
+			if err != nil {


SHOW WARNINGS only reports warnings for the most recently executed statement. Since the DML batch is executed as a single multi-statement, warnings generated by earlier statements in the batch can be missed, which undermines PanicOnWarnings and can reintroduce silent data loss. Consider disabling multi-statement batching when PanicOnWarnings is enabled (execute each statement and check warnings after each), or otherwise capture warning counts per statement.

Copilot · 2026-03-04T19:29:00Z

go/logic/applier.go

+					this.migrationContext.Log.Warningf("Failed to read SHOW WARNINGS row")
+					continue


On rows.Scan failure, this logs and continues, which can cause real warnings to be ignored and the transaction to commit even though PanicOnWarnings is enabled. It’s safer to treat scan errors as fatal here (rollback/return error) so warning handling can’t silently degrade.

Suggested change

this.migrationContext.Log.Warningf("Failed to read SHOW WARNINGS row")

continue

return rollback(err)

Copied from existing code; will consider fixing in both places

go/logic/applier.go

ggilder · 2026-03-04T20:29:23Z

@meiji163 I've pushed up some commits to address Copilot feedback. I still need to make changes to address the issues around SHOW WARNINGS with a batch of inserts, but first I'd like to understand your comment about INSERT IGNORE — please let me know what you think of #1633 (comment)

meiji163 · 2026-03-05T18:16:38Z

@ggilder Scenario such as this

T0: INSERT INTO table id=1, name='Alice'
T1: Row copier: INSERT IGNORE INTO _table_gho id=1, name='Alice'
T2: UPDATE table SET name='Bob' WHERE id=1
T3: Binlog applier: INSERT IGNORE INTO _table_gho id=1, name='Bob'

Therefore the update is ignored by binlog applier

ggilder · 2026-03-05T18:33:45Z

@meiji163 I don't think T3 in your example would be an INSERT IGNORE though. The Applier would see an UpdateDML event (source), which would be handled by NewDMLUpdateQueryBuilder, generating an UPDATE statement (source).

meiji163 · 2026-03-05T18:39:17Z

@ggilder Aha I forgot that, then it does seem OK. I will do a few internal tests

ggilder marked this pull request as ready for review March 2, 2026 19:54

ggilder requested review from meiji163, rashiq and timvaillancourt as code owners March 2, 2026 19:54

Copilot AI review requested due to automatic review settings March 2, 2026 19:54

Copilot started reviewing on behalf of ggilder March 2, 2026 19:54 View session

Copilot AI reviewed Mar 2, 2026

View reviewed changes

grodowski mentioned this pull request Mar 3, 2026

PanicOnWarnings option to detect SQL warnings and fail the copy process #1500

Merged

2 tasks

ggilder force-pushed the fix-panic-on-warnings branch from 9780c84 to 57b77b3 Compare March 4, 2026 19:22

ggilder requested a review from Copilot March 4, 2026 19:24

Copilot started reviewing on behalf of ggilder March 4, 2026 19:24 View session

Copilot AI reviewed Mar 4, 2026

View reviewed changes

ggilder added 4 commits March 4, 2026 11:40

Address linter issue

4248710

Clarify test comment

d1ccd44

Consolidate test files

278ec29

Add execinquery linter comment

c13be70

Merge branch 'master' into fix-panic-on-warnings

6b5c547

		this.migrationContext.Log.Warningf("Failed to read SHOW WARNINGS row")
		continue

	this.migrationContext.Log.Warningf("Failed to read SHOW WARNINGS row")
	continue
	return rollback(err)

Conversation

ggilder commented Feb 28, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Problem

Example Scenario

Root Cause

Solution

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI Mar 2, 2026

Choose a reason for hiding this comment

Uh oh!

ggilder Mar 4, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

meiji163 commented Mar 3, 2026

Uh oh!

ggilder commented Mar 4, 2026

Uh oh!

ggilder commented Mar 4, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Copilot AI Mar 4, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 4, 2026

Choose a reason for hiding this comment

Uh oh!

ggilder Mar 4, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ggilder commented Mar 4, 2026

Uh oh!

meiji163 commented Mar 5, 2026

Uh oh!

ggilder commented Mar 5, 2026

Uh oh!

meiji163 commented Mar 5, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

ggilder commented Feb 28, 2026 •

edited

Loading

ggilder commented Mar 4, 2026 •

edited

Loading