fix: prevent data loss when MySQL tables have triggers (#285)#311
Draft
ljluestc wants to merge 1 commit intobrokercap:masterfrom
Draft
fix: prevent data loss when MySQL tables have triggers (#285)#311ljluestc wants to merge 1 commit intobrokercap:masterfrom
ljluestc wants to merge 1 commit intobrokercap:masterfrom
Conversation
When MySQL tables have triggers and binlog_format is STATEMENT or MIXED, DML operations are logged as QUERY_EVENT instead of row events. This caused two cascading bugs resulting in data loss: 1. Bristol/mysql/conn_dump.go: After a DML QUERY_EVENT callback, commitEventOk was reset to false, causing the subsequent COMMIT/XID event to be skipped entirely. Downstream plugins never received the commit signal. 2. plugin/clickhouse/src/clickhouse.go: Non-DDL QUERY_EVENTs (DML and transaction control like SAVEPOINT) triggered premature AutoCommit(), flushing pending row data and resetting the buffer mid-transaction. Fixes: - Add QUERY_EVENT case in conn_dump.go post-callback switch to preserve commitEventOk=true for DML queries (identified by event.TableName!="") - Add IsNonDDLQuery() helper to skip DML and transaction control statements without calling AutoCommit in the ClickHouse plugin - Add 7 new tests covering the fix
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
When MySQL tables have triggers and
binlog_formatisSTATEMENTorMIXED, DML operations (INSERT/UPDATE/DELETE) are logged asQUERY_EVENTinstead of row-based events (WRITE_ROWS_EVENT,UPDATE_ROWS_EVENT,DELETE_ROWS_EVENT). Bifrost did not handle this case, causing two cascading bugs that resulted in silent data loss.Bug 1 — Binlog parser drops the commit signal (
Bristol/mysql/conn_dump.go):After a DML
QUERY_EVENTis dispatched via callback, the post-callbackswitchfalls through to thedefaultcase and resetscommitEventOk = false. When the subsequentCOMMITorXID_EVENTarrives, it seescommitEventOk == falseand skips the commit entirely. Downstream plugins never receive the transaction.Bug 2 — ClickHouse plugin flushes data prematurely (
plugin/clickhouse/src/clickhouse.go):For any
QUERY_EVENTthat does reach the ClickHouse plugin (e.g. DML or transaction control statements likeSAVEPOINT), theQuery()method falls through toAutoCommit(). This flushes whatever partial row data is in the buffer and resets it mid-transaction, losing data.Fixes #285
Changes
Bristol/mysql/conn_dump.goAdd a
QUERY_EVENTcase in the post-callback switch. Ifevent.TableName != ""(i.e. the event carries DML data for a specific table), preservecommitEventOk = trueso the subsequent COMMIT/XID event is properly propagated. For control queries like BEGIN/COMMIT that have no table name, resetcommitEventOk = falseas before.plugin/clickhouse/src/sql.goAdd
IsNonDDLQuery()helper function that identifies:INSERT,UPDATE,DELETE,REPLACESAVEPOINT,RELEASE SAVEPOINT,ROLLBACK TOThese are matched case-insensitively via prefix after trimming whitespace.
plugin/clickhouse/src/clickhouse.goCall
IsNonDDLQuery()early inQuery(). If the query is non-DDL, return(nil, nil, nil)immediately — noAutoCommit()call, no buffer flush. Pending row data is preserved for the eventual COMMIT to flush correctly.Tests (7 new test functions)
TestIsNonDDLQuery— 20 cases covering DDL, DML, transaction control, edge cases, and case insensitivityTestTranferQuerySql_DML— verifiesTranferQuerySqlreturns empty strings for all DML/transaction-control inputsTestQuery_NonDDLStatements— verifies 7 non-DDL query types don't triggerAutoCommitor flush buffered dataTestQuery_TransactionControlSkipped— verifies BEGIN/COMMIT don't flush pending dataTestQuery_AutoCreateTableFalse— verifies all queries are no-ops whenAutoCreateTableis disabled