Skip to content

Fix handling of warnings on DML batches#1643

Open
ggilder wants to merge 7 commits intogithub:masterfrom
ggilder:fix-dml-batch-issue
Open

Fix handling of warnings on DML batches#1643
ggilder wants to merge 7 commits intogithub:masterfrom
ggilder:fix-dml-batch-issue

Conversation

@ggilder
Copy link
Contributor

@ggilder ggilder commented Mar 10, 2026

Related issue: #1636

Description

This is a follow-up to #1633 — this was actually an incomplete fix for the data loss issue described there, because any DML events that would cause data loss that happened in the middle of a batch could be masked by success of the final statement in the batch. This code change now runs SHOW WARNINGS after each statement in a batch so that we collect all warnings.

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

Copilot AI review requested due to automatic review settings March 10, 2026 17:33
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Improves --panic-on-warnings safety during binlog replay by ensuring warnings emitted by any statement inside a batched DML multi-statement are detected (not just the last statement), addressing the masking/data-loss scenario from #1636 / follow-up to #1633.

Changes:

  • Add executeBatchWithWarningChecking() to interleave SHOW WARNINGS after each DML in a batch when PanicOnWarnings is enabled.
  • Update ApplyDMLEventQueries() to use the new warning-checking execution path only for PanicOnWarnings (keep existing fast path otherwise).
  • Add a new unit test and a new local integration test case to validate “warning in the middle of a batch” behavior.

Reviewed changes

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

Show a summary per file
File Description
localtests/panic-on-warnings-batch-middle/extra_args Adds localtest arguments enabling --panic-on-warnings with a unique index alter.
localtests/panic-on-warnings-batch-middle/expect_failure Asserts failure output contains warning-detection text.
localtests/panic-on-warnings-batch-middle/create.sql Sets up event-driven DML to reproduce a batched replay scenario.
go/logic/applier_test.go Adds unit test asserting mid-batch warning causes rollback of entire batch.
go/logic/applier.go Implements per-statement warning detection for batched DML execution under PanicOnWarnings.

💡 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.

@ggilder
Copy link
Contributor Author

ggilder commented Mar 10, 2026

@meiji163 important correction to earlier data loss fix — lmk if you have questions

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants