Various MySQL Bug Fixes

[patrickwwbutler]

Fixes https://github.com/MaterializeInc/database-issues/issues/11312 by changing the decoding logic - previously with binlog_row_metadata = MINIMAL, excluded columns were not being skipped in the binlog, resulting in the wrong column, because we were filtering out excluded columns from the desc columns before zipping. Also changes logic for replication when binlog_row_metadata = FULL by building a vec of pairs by locating the matching index in the binlog and grabbing the value there.

Fixes https://github.com/MaterializeInc/database-issues/issues/11313 by filtering out excluded columns when checking compatibility.

Fixes https://github.com/MaterializeInc/database-issues/issues/11315 by checking for version before both of the binlog_row_metadata checks.

[patrickwwbutler]

@def- I clauded up a test for the version check - but unfortunately it seems hard to actually run as we don't have images for lower mysql versions:

$ docker compose up --detach --wait materialized mysql
[+] Running 0/1
 ⠦ mysql Pulling                                                                                                                                                                                                                                                              0.6s 
no matching manifest for linux/arm64/v8 in the manifest list entries

Do we have a good way to test this?

[def-]

With this test addition:

diff --git a/test/mysql-cdc/binlog-backward-compat.td b/test/mysql-cdc/binlog-backward-compat.td
index e4113285a8..765c66f0bd 100644
--- a/test/mysql-cdc/binlog-backward-compat.td
+++ b/test/mysql-cdc/binlog-backward-compat.td
@@ -102,6 +102,33 @@ DELETE FROM bar WHERE a = 4;
 > DROP SOURCE mysql_src2 CASCADE;


+$ mysql-execute name=mysql
+SET GLOBAL binlog_row_metadata = FULL;
+USE public;
+CREATE TABLE baz (a INT, b INT, c INT);
+INSERT INTO baz VALUES (1, 2, 3), (4, 5, 6);
+
+> CREATE SOURCE mysql_src3 FROM MYSQL CONNECTION mysql_conn;
+
+> CREATE TABLE baz FROM SOURCE mysql_src3 (REFERENCE public.baz) WITH (EXCLUDE COLUMNS (b));
+
+> SELECT * FROM baz;
+1 3
+4 6
+
+$ mysql-execute name=mysql
+SET GLOBAL binlog_row_metadata = MINIMAL;
+ALTER TABLE baz ADD COLUMN d INT DEFAULT NULL;
+INSERT INTO baz (a, b, c) VALUES (7, 8, 9);
+
+> SELECT * FROM baz;
+1 3
+4 6
+7 9
+
+> DROP SOURCE mysql_src3 CASCADE;
+
+
 # Restore to a clean state so other tests are unaffected.
 $ mysql-execute name=mysql
 SET GLOBAL binlog_row_metadata = FULL;

Do we expect to get "incompatible schema change" when MySQL switches from FULL to MINIMAL and you run any DDL? Because it fails like that for me:

binlog-backward-compat.td:136:1: error: executing query failed: db error: ERROR: Source error: source must be dropped and recreated due to failure: incompatible schema change: column c in table baz has been altered
     |
  15 | > CREATE SECRET mysq ... [rest of line truncated for security]
  20 |     PASSWORD SECRET  ... [rest of line truncated for security]
  23 | $ mysql-connect name ... [rest of line truncated for security]
 135 |
 136 | > SELECT * FROM baz;
     | ^
^^^ +++
+++ !!! Error Report
1 errors were encountered during execution
files involved: binlog-backward-compat.td

[martykulma]

Thanks @patrickwwbutler! Left some comments, one of them is a nit, but the other one we need to nail down.

@maheshwarip - just a heads up as this may be unexpected - MySQL 5.7 can't be supported with source versioning as it doesn't have support for full row metadata.

[martykulma]

To avoid problems when reordering, it seems that we would need to ensure that binlog_row_metadata did not revert from FULL to MINIMAL. Otherwise, we might see an alter table reorder columns, which would pass this check when FULL, and then a switch to MINIMAL would cause parsing issues.

We could pin the behavior at source creation time by capturing the setting like we do with initial_gtid_set and store it in MySqlSourceExportDetails - if it happens to change during runtime, it won't change behavior (the user would have to recreate).

[patrickwwbutler]

Hmm. Technically CTFS syntax requires binlog_row_metadata = FULL, and those ALTER TABLE statements should require CTFS syntax, so yeah, technically any reversion from FULL to MINIMAL with the new syntax should break the source. We just need a good way to know what the source type is from here, is that visible from this section of the codebase?

[martykulma]

it seems odd that pure.rs looks at the system variable and this uses VERSION() - can they use the same means of getting the version?

[patrickwwbutler]

Oh they are identical - VERSION() is just syntactic sugar for the system variable, but we can't use the mz_mysql_util::query_sys_var function here because of some trait requirements on conn that I decided were more trouble than it's worth to fix here when there's such a simple workaround.

I can update it so that they're the same if that improves readability, I just opted to use mz_mysql_util::query_sys_var when possible since it has some convenient wrapping

[martykulma]

this is also a problem - any error that becomes a definite error can't change the string it renders to, so this needs to be "extra column description"