runtime RBR minimal event check#651
Conversation
05d8e54 to
ec7cf76
Compare
e40c029 to
64f8bf2
Compare
There was a problem hiding this comment.
Pull request overview
This PR implements runtime checks for minimal Row-Based Replication (RBR) images (issue #561), detecting binlog_row_image=MINIMAL events during move/migration operations that use a buffered applier (which requires full row images). The PR also refactors DDL change notifications from a channel-based system to a CancelFunc callback pattern.
Changes:
- Added
isMinimalRowImagehelper and a runtime check inprocessRowsEventthat returns an error when a minimal RBR event is received while a buffered applier is in use. - Replaced the
OnDDL chan stringchannel +SetDDLNotificationChannelAPI with aCancelFunccallback inClientConfig, simplifying the notification path and removing the need for a background goroutine in each runner. - Replaced panics with graceful
fatalError()returns that invoke the caller'sCancelFunc.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
pkg/repl/utils.go |
Made encodeSchemaTable unexported; removed DecodeSchemaTable; changed extractTablesFromDDLStmts return type to []schemaTable |
pkg/repl/utils_test.go |
Updated tests to use new schemaTable type; removed tests for removed functions |
pkg/repl/client.go |
Added isMinimalRowImage, fatalError; replaced DDL channel with CancelFunc/DDLFilterSchema; replaced panics with graceful shutdown |
pkg/repl/client_test.go |
Updated tests for new API; added TestIsMinimalRowImage and TestProcessRowsEventMinimalRBRWithApplier |
pkg/migration/runner.go |
Replaced ddlNotification channel + tableChangeNotification goroutine with fatalError callback |
pkg/migration/runner_test.go |
Added TestBufferedMigrationFailsGracefullyWithMinimalRBR |
pkg/move/runner.go |
Replaced ddlNotification channel + tableChangeNotification goroutine with fatalError callback and DDLFilterSchema |
pkg/move/runner_test.go |
Added TestMoveFailsGracefullyWithMinimalRBR |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 8 out of 8 changed files in this pull request and generated 2 comments.
Comments suppressed due to low confidence (1)
pkg/repl/client.go:625
- After
processDDLNotificationcallsc.fatalError(), thereadStreamgoroutine continues running (there is noreturnstatement after the loop that callsprocessDDLNotificationon lines 623–625). This is inconsistent with the rows-event path wherefatalError()is always followed byreturn(lines 605–606). As a result, after a DDL-triggered cancellation, the goroutine keeps consuming and processing binlog events until the client's internal context is eventually cancelled byClose(). While this is not a correctness issue (sincecallerCancelFuncis idempotent), it wastes resources and introduces an asymmetry that may complicate future maintenance.
// For example, it won't parse [CREATE|DROP] TRIGGER statements *or*
// ALTER USER x IDENTIFIED WITH x RETAIN CURRENT PASSWORD
// This behavior is copied from canal:
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
A Pull Request should be associated with an Issue.
Fixes #561
This refactors the DDL notification channel to be a generic cancel callback. We use the same callback if we detect that there was an RBR minimal event.