Skip to content

Support MySQL binlog transaction compression - #57

Open
coding-chimp wants to merge 1 commit into
sync-upstream-2026-11-22from
bb/binlog-compression
Open

coding-chimp wants to merge 1 commit into
sync-upstream-2026-11-22from
bb/binlog-compression

Conversation

@coding-chimp

Copy link
Copy Markdown

Summary

Add support for MySQL’s binlog_transaction_compression=ON.

Compressed transactions arrive as a TransactionPayloadEvent containing nested row and commit events. Previously, gh-ost only handled top-level events, silently skipping the enclosed DML changes and failing to advance its completed-transaction checkpoint.

Changes

  • Upgrade go-mysql from v1.15.0 to v1.16.0, which preserves decoding settings inside compressed payloads and fixes nested-event checkpoint metadata.
  • Process nested INSERT, UPDATE, DELETE, and XID events through gh-ost’s existing row and transaction-completion handling.
  • Use the outer payload’s coordinates in file-position mode and clone the executed GTID set when recording transaction completion.
  • Preserve upstream MariaDB GTID support.

Testing

  • Added regression tests for compressed DML, table filtering, decimal and timestamp decoding, file-position and GTID checkpoints, mixed compressed/uncompressed transactions, and error handling.
  • Added a MySQL 8.0.42 integration test that verifies the server actually writes a compressed payload and confirms gh-ost consumes it correctly in both coordinate modes.
  • Full go test ./... suite passes.

@coding-chimp coding-chimp self-assigned this Sep 22, 2026
@coding-chimp
coding-chimp added this pull request to stack #58 September 22, 2026 07:59
for _, nested := range event.Events {
switch nestedEvent := nested.Event.(type) {
case *replication.RowsEvent:
if err := gmr.handleRowsEvent(nested, nestedEvent, entriesChannel); err != nil {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

A local review agent helped me find this. Each nested DML here carries the outer payload position, so the first applied row falsely claims that the complete payload was applied, which is a likely issue for the applier / checkpointing in compressed + file mode.

handleRowsEvent does currentCoords := gmr.GetCurrentBinlogCoordinates() for every event.

I think this potential bug also affects GTID-streaming which is unrelated to this change, because all row events in a transaction carry the same GTID, so applying the first row can mark the entire transaction as applied while later rows remain queued in entriesChannel.

There's a tiny window when this can happen (either in file-compressed here or GTID modes before):

  • DML event process start for transaction T
  • Partial row event processed
  • Checkpoint
  • Gh-ost crash
  • A resume from checkpoint would treat T as applied 🔥

For an uncompressed GTID transaction:

  GTID event → reader advances to G
  row 1      → queued with G
  row 2      → queued with G
  row 3      → queued with G
  XID        → reader marks G as completely read

A compressed GTID transaction has the same unsafe outcome:

  top-level GTID event          → reader.currentCoordinates = G
  top-level TransactionPayload  → unpack nested events
  nested row 1                  → queued with G
  nested row 2                  → queued with G
  nested row 3                  → queued with G
  nested XID                    → reader.LastTrxCoords = G

Here's a test with DMLBatchSize=1 to reproduce these 4 cases: https://vault.shopify.io/snippify/snippets/e3ebc6d0f58823d0c661

Possible fixes:

  1. Enqueue a transaction-completion marker after the nested XID.
  2. Buffer the entire transaction and apply it as one queue item.
  3. Track outstanding DML per transaction and advance the coordinate when the count reaches zero.
  4. Keep a pending-transaction watermark that makes Checkpoint() wait independently of applier.CurrentCoordinates.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Correction: prior to this change, the race would be limited to multiple rows inside a RowsEvent.

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