Skip to content

MySQL Check binlog metadata setting during schema validation#35910

Merged
patrickwwbutler merged 4 commits intoMaterializeInc:mainfrom
patrickwwbutler:patrick/mysql-enforce-binlog
Apr 9, 2026
Merged

MySQL Check binlog metadata setting during schema validation#35910
patrickwwbutler merged 4 commits intoMaterializeInc:mainfrom
patrickwwbutler:patrick/mysql-enforce-binlog

Conversation

@patrickwwbutler
Copy link
Copy Markdown
Contributor

Addresses a gap where certain schema changes were being considered valid even though their validity is contingent on the binlog_row_metadata = FULL; mysql system variable setting.

Fixes this broken test: https://buildkite.com/materialize/nightly/builds/15965/steps/canvas?jid=019d6a48-8cf3-44a0-933f-451cda3adff1&tab=output

@patrickwwbutler patrickwwbutler requested review from a team April 8, 2026 18:23
@patrickwwbutler patrickwwbutler requested a review from a team as a code owner April 8, 2026 18:23
@github-actions
Copy link
Copy Markdown
Contributor

github-actions bot commented Apr 8, 2026

Thanks for opening this PR! Here are a few tips to help make the review process smooth for everyone.

PR title guidelines

  • Use imperative mood: "Fix X" not "Fixed X" or "Fixes X"
  • Be specific: "Fix panic in catalog sync when controller restarts" not "Fix bug" or "Update catalog code"
  • Prefix with area if helpful: compute: , storage: , adapter: , sql:

Pre-merge checklist

  • The PR title is descriptive and will make sense in the git log.
  • This PR has adequate test coverage / QA involvement has been duly considered. (trigger-ci for additional test/nightly runs)
  • If this PR includes major user-facing behavior changes, I have pinged the relevant PM to schedule a changelog post.
  • This PR has an associated up-to-date design doc, is a design doc (template), or is sufficiently small to not require a design.
  • If this PR evolves an existing $T ⇔ Proto$T mapping (possibly in a backwards-incompatible way), then it is tagged with a T-proto label.
  • If this PR will require changes to cloud orchestration or tests, there is a companion cloud PR to account for those changes that is tagged with the release-blocker label (example).

Copy link
Copy Markdown
Contributor

@ggevay ggevay left a comment

Choose a reason for hiding this comment

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

Approving to unblock the fixing of main, but would be good if someone from @MaterializeInc/sources-sinks would also take a look.

(Could you please rebase on the latest main? #35909 fixed another test fail that CI ran into here.)

Copy link
Copy Markdown
Contributor

@martykulma martykulma left a comment

Choose a reason for hiding this comment

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

lgtm - I kicked off the MySQL nightly tests to validate the fix. The comments need updating for determine_compatibility but doesn't block.

Comment on lines 97 to 99
@@ -95,16 +99,26 @@ impl MySqlTableDesc {
// ignore extra columns from `other.columns`.
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This comment should be updated, and also the one for the method signature. Ok to do in a follow on PR to unblock.

@patrickwwbutler patrickwwbutler force-pushed the patrick/mysql-enforce-binlog branch from aebf705 to 48c2706 Compare April 9, 2026 14:09
@patrickwwbutler patrickwwbutler force-pushed the patrick/mysql-enforce-binlog branch from 6b7d777 to ecc98a3 Compare April 9, 2026 19:32
@patrickwwbutler
Copy link
Copy Markdown
Contributor Author

I'm fairly certain this PR has actually just exposed a race condition that already existed when handling MySQL restores, so I'm going to disable the commented out test for right now so that this PR can be merged to unblock main

@patrickwwbutler patrickwwbutler enabled auto-merge (squash) April 9, 2026 21:16
@patrickwwbutler patrickwwbutler merged commit 20268ad into MaterializeInc:main Apr 9, 2026
118 checks passed
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.

3 participants