mysql: add support for invisible columns #31239

martykulma · 2025-01-29T21:06:47Z

To support invisible columns in MySQL, explicitly request columns in snapshot select statement. And while I'm here, I corrected the glob root path in mzcompose.py to get the tests to run.

Motivation

Adds support for invisible columns in mysql

Tips for reviewer

Checklist

This PR has adequate test coverage / QA involvement has been duly considered. (trigger-ci for additional test/nightly runs)
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).
If this PR includes major user-facing behavior changes, I have pinged the relevant PM to schedule a changelog post.

github-actions · 2025-01-29T21:07:02Z

All contributors have signed the CLA.
_{Posted by the CLA Assistant Lite bot.}

martykulma · 2025-01-29T21:08:45Z

I have read the Contributor License Agreement (CLA) and I hereby sign the CLA.

petrosagg · 2025-01-31T15:00:23Z

src/storage/src/source/mysql/snapshot.rs

+                                .iter()
+                                .map(|col_info| format!("`{}`", col_info.name))
+                        })
+                        .join(", ");


Hm, I think this might be doing a different thing than what we need. Here you iterate over all outputs and concatenate the name of the columns from each one of them. So if I understand correctly, if the same table is ingested twice in Materialize we will end up with a query like SELECT col1, col2, col1, col2 and transfer twice the amount of data. Am I reading this right?

excellent point - I hadn't considered that case.

Now that I'm looking back at it, this may also run into time-of-check/time-of-use issues. There's no guarantee that the table definition is the same at the time we issue the select. Which means we need to identify the columns in the transaction that we run the query.. I'll make that change.

There's no guarantee that the table definition is the same at the time we issue the select.

That's true, but also fine, because Materialize expects a certain schema that has already been committed in the catalog. So we should explicitly request the columns that we expect to be there, and if they aren't we'll get an error from the MySQL client and report it.

Even if we learned the new schema in the dataflow there isn't anything else we can do at that point. If the user has changed the upstream schema after having created the ingestion in Materialize then the only way out is to drop and recreate the tables in Materialize.

I think what we want to do here is collect the set of columns that each output needs (using something like a BTreeSet) and then make sure that we decode and project the columns in the right order downstream

🤔 When we build MySqlTableDesc.columns, the intention appears to be that all columns will be in the struct. For ignored columns, column_type is set to None, but the name is maintained. So I expect it's possible to use any entry from outputs for the column names in the table description. I'll check if we have test coverage for it, if not, I'll add something.

I've updated the PR to just use the first entry rather than a BTreeSet based on my note above. Let me know if that sits well with you, thanks!

Sounds good to me! Let's also add an assertion that all the outputs contain the same column names in the same order to ensure a panic if this assumption ever changes

petrosagg · 2025-01-31T15:00:39Z

test/mysql-cdc/mzcompose.py

    for filter in args.filter:
-        matching_files.extend(glob.glob(filter, root_dir="test/mysql-cdc"))
+        matching_files.extend(
+            glob.glob(filter, root_dir=MZ_ROOT / "test" / "mysql-cdc")


Thanks for this fix! I've been bitten by it!

antiguru · 2025-02-03T20:25:02Z

src/storage/src/source/mysql/snapshot.rs

+        .iter()
+        .map(|col| format!("`{}`", col.name))
+        .join(", ");
+    format!("SELECT {} FROM {}", columns, info.table_name)


(sorry for the drive-by review) Do we have to worry about SQL injection here?

mmm.. good point, I made an unfortunate assumption that the existing format! was good enough.

I'll update the snapshot command and collect_table_statistics to use prepared statements.

I did wonder about that during review and concluded that we don't have to be worried as to perform an injection you have to somehow create weirdly named columns, which means you already admin level have access to the database. My understanding is that we can't use prepared statements here since with those you can put placeholders for values but here we want to put placeholders for column names. If it's possible and easy to do we should maybe do it

I have done this kind of thing in PG by building dynamic statements using virtual sets for the identifiers, but I'm not as familiar with MySQL, and this is proving challenging to try to implement via prepared statements.

mysql_async client doesn't provide a quote_identifier, but I can write one based on the stored procedure implementation

petrosagg

Looks great!

petrosagg · 2025-02-04T19:18:59Z

src/mysql-util/src/lib.rs

+        let input = ["a", "naughty`sql", "`;naughty;sql;`"].iter().map(|raw_str| quote_identifier(&raw_str)).collect::<Vec<_>>();
+        assert_eq!(expected, input);
+    }
+}


Looks like your editor doesn't add trailing new lines, this might generate spurious diffs. Not blocking for this PR but something to configure

petrosagg · 2025-02-04T19:28:20Z

src/storage/src/source/mysql/snapshot.rs

+    if outputs.len() > 1
+        && !outputs
+            .iter()
+            .skip(1)
+            .map(|other_info| &other_info.desc.columns)
+            .all(|other_columns| *other_columns == info.desc.columns)


small nit: in idiomatic Rust, at least in the MZ codebase, it usually better to avoid long chains of iterator transformations with either a short chain if possible or just a imperative for loop. This functional style becomes harder to read after a certain point.

So I would write this either as:

assert!(outputs.iter().all(|o| info.desc.columns == o.desc.columns), "Mismatch...");

or:

for output in outputs[1..] { assert!(info.desc.columns == output.desc.columns), "Mismatch..."); }

got it, thank you!

petrosagg · 2025-02-04T19:28:32Z

src/storage/src/source/mysql/snapshot.rs

+        .iter()
+        .map(|col| format!("`{}`", col.name))
+        .join(", ");
+    format!("SELECT {} FROM {}", columns, info.table_name)


Noticed and fixed for one test by Marty in MaterializeInc#31239

def-

Thanks for adding a test. I have added a platform checks check, nightly run with it: https://buildkite.com/materialize/nightly/builds/11054

def- · 2025-02-05T12:29:45Z

I find it slightly suspicious that the MySQL CDC resumption test timed out twice on this PR: https://buildkite.com/materialize/test/builds/98149#0194d5c5-ceb8-4010-9f7f-a41aa8ce2acf
It doesn't seem to get stuck, but just seems to run slower. I will bump the timeout for now, but is this expected?

martykulma · 2025-02-05T12:48:32Z

I find it slightly suspicious that the MySQL CDC resumption test timed out twice on this PR: https://buildkite.com/materialize/test/builds/98149#0194d5c5-ceb8-4010-9f7f-a41aa8ce2acf It doesn't seem to get stuck, but just seems to run slower. I will bump the timeout for now, but is this expected?

hmm.. this PR changes how we query for snapshot from SELECT * FROM ... to SELECT col1, col2, ... FROM ... - so it should have no effect on performance.

That verify-rows-deleted test has another issue open: https://github.com/MaterializeInc/database-issues/issues/8884. I haven't seen either failure in my runs, but I can attempt tracking it down. @petrosagg, is that way above my head at this point, or would you feel comfortable with me taking it on?

petrosagg · 2025-02-05T13:53:56Z

hm, https://github.com/MaterializeInc/database-issues/issues/8884 seems a bit in the weeds since it involves a failover scenario so I wouldn't dig into it at the moment.

I compared the set of dataflow restarts that happened in this run vs one from main that succeeds and this is the difference:

-INFO mz_storage::healthcheck: Broadcasting suspend-and-restart command because of Some((MySql, Stalled { error: "Input/output error: Input/output error: Connection refused (os error 111): Input/output error: Connection refused (os error 111): Connection refused (os error 111)", hint: None, should_halt: true })) after 5s delay
+INFO mz_storage::healthcheck: Broadcasting suspend-and-restart command because of Some((MySql, Stalled { error: "Input/output error: Input/output error: connection closed: Input/output error: connection closed: connection closed", hint: None, should_halt: true })) after 5s delay
+INFO mz_storage::healthcheck: Broadcasting suspend-and-restart command because of Some((MySql, Stalled { error: "Input/output error: Input/output error: connection closed: Input/output error: connection closed: connection closed", hint: None, should_halt: true })) after 5s delay
+INFO mz_storage::healthcheck: Broadcasting suspend-and-restart command because of Some((MySql, Stalled { error: "Input/output error: Input/output error: connection closed: Input/output error: connection closed: connection closed", hint: None, should_halt: true })) after 5s delay

(For Marty: these logs are in each buildkite run under the "Artifacts" tab and named "services.log")

So it looks like in this run the dataflow found the MySQL service down more times. The run on main takes 24 minutes which is already close to the 30 minute timeout. At the same time these three restarts should take less than 6 minutes to complete so it's not that they are a complete explanation.

Given the change is pretty simple I'm inclined to just merge this with the increased timeout without further investigation.

Noticed in https://buildkite.com/materialize/nightly/builds/11064#0194d8ae-2f77-487b-b6fc-ec78a95aaaef Follow-up to MaterializeInc#31239

martykulma and others added 2 commits January 30, 2025 07:12

mysql: support invisible columns in snapshot

6907869

mysql-cdc: fix glob root path in mzcompose.py

bb7369e

martykulma force-pushed the maz-mysql-invisible-cols branch from f1ec013 to bb7369e Compare January 30, 2025 12:16

martykulma marked this pull request as ready for review January 30, 2025 15:01

martykulma requested a review from a team as a code owner January 30, 2025 15:01

petrosagg reviewed Jan 31, 2025

View reviewed changes

Account for multiple table descriptions per table

b47d1d8

martykulma requested a review from petrosagg February 3, 2025 18:55

antiguru reviewed Feb 3, 2025

View reviewed changes

martykulma added 4 commits February 4, 2025 09:36

Improve identifier quoting

7ade1a6

formatting

2ddba16

clippy

c2cc53e

doc

31f732a

petrosagg approved these changes Feb 4, 2025

View reviewed changes

a little less functional

0af5488

martykulma requested a review from antiguru February 5, 2025 02:37

def- added a commit to def-/materialize that referenced this pull request Feb 5, 2025

tests: Fix glob root dir to respect MZ_ROOT

73f66c1

Noticed and fixed for one test by Marty in MaterializeInc#31239

def- mentioned this pull request Feb 5, 2025

tests: Fix glob root dir to respect MZ_ROOT #31289

Merged

5 tasks

def- requested a review from a team as a code owner February 5, 2025 09:43

def- approved these changes Feb 5, 2025

View reviewed changes

platform-checks: Add check for mysql invisible columns

fe6897e

def- force-pushed the maz-mysql-invisible-cols branch from e065a5f to fe6897e Compare February 5, 2025 10:08

ci: Increase parallelism, bump timeout

8f97a57

martykulma merged commit f960792 into main Feb 5, 2025
83 checks passed

martykulma deleted the maz-mysql-invisible-cols branch February 5, 2025 18:42

def- added a commit to def-/materialize that referenced this pull request Feb 6, 2025

mysql old syntax test: Invisible columns supported

dd09155

Noticed in https://buildkite.com/materialize/nightly/builds/11064#0194d8ae-2f77-487b-b6fc-ec78a95aaaef Follow-up to MaterializeInc#31239

def- added a commit to def-/materialize that referenced this pull request Feb 6, 2025

mysql old syntax test: Invisible columns supported

6047d24

Noticed in https://buildkite.com/materialize/nightly/builds/11064#0194d8ae-2f77-487b-b6fc-ec78a95aaaef Follow-up to MaterializeInc#31239

def- added a commit to def-/materialize that referenced this pull request Feb 6, 2025

mysql old syntax test: Invisible columns supported

8615d72

Noticed in https://buildkite.com/materialize/nightly/builds/11064#0194d8ae-2f77-487b-b6fc-ec78a95aaaef Follow-up to MaterializeInc#31239

def- mentioned this pull request Feb 6, 2025

mysql old syntax test: Invisible columns supported #31302

Merged

5 tasks

mysql: add support for invisible columns #31239

mysql: add support for invisible columns #31239

Uh oh!

Conversation

martykulma commented Jan 29, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Motivation

Tips for reviewer

Checklist

Uh oh!

github-actions bot commented Jan 29, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

martykulma commented Jan 29, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

martykulma Jan 31, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

martykulma Feb 3, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

martykulma Feb 4, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

petrosagg left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

def- left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

def- commented Feb 5, 2025

Uh oh!

martykulma commented Feb 5, 2025

Uh oh!

petrosagg commented Feb 5, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

martykulma commented Jan 29, 2025 •

edited

Loading

github-actions bot commented Jan 29, 2025 •

edited

Loading

martykulma Jan 31, 2025 •

edited

Loading

martykulma Feb 3, 2025 •

edited

Loading

martykulma Feb 4, 2025 •

edited

Loading

def- left a comment •

edited

Loading