-
Notifications
You must be signed in to change notification settings - Fork 469
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
mysql: add support for invisible columns #31239
Merged
Merged
Changes from all commits
Commits
Show all changes
10 commits
Select commit
Hold shift + click to select a range
6907869
mysql: support invisible columns in snapshot
martykulma bb7369e
mysql-cdc: fix glob root path in mzcompose.py
martykulma b47d1d8
Account for multiple table descriptions per table
martykulma 7ade1a6
Improve identifier quoting
martykulma 2ddba16
formatting
martykulma c2cc53e
clippy
martykulma 31f732a
doc
martykulma 0af5488
a little less functional
martykulma fe6897e
platform-checks: Add check for mysql invisible columns
def- 8f97a57
ci: Increase parallelism, bump timeout
def- File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains 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
This file contains 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
This file contains 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
This file contains 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
This file contains 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
This file contains 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
This file contains 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
This file contains 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
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -15,7 +15,7 @@ | |
import threading | ||
from textwrap import dedent | ||
|
||
from materialize import buildkite | ||
from materialize import MZ_ROOT, buildkite | ||
from materialize.mysql_util import ( | ||
retrieve_invalid_ssl_context_for_mysql, | ||
retrieve_ssl_context_for_mysql, | ||
|
@@ -99,7 +99,9 @@ def workflow_cdc(c: Composition, parser: WorkflowArgumentParser) -> None: | |
|
||
matching_files = [] | ||
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") | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks for this fix! I've been bitten by it! |
||
) | ||
sharded_files: list[str] = sorted( | ||
buildkite.shard_list(matching_files, lambda file: file) | ||
) | ||
|
This file contains 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
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,34 @@ | ||
# Copyright Materialize, Inc. and contributors. All rights reserved. | ||
# | ||
# Use of this software is governed by the Business Source License | ||
# included in the LICENSE file at the root of this repository. | ||
# | ||
# As of the Change Date specified in that file, in accordance with | ||
# the Business Source License, use of this software will be governed | ||
# by the Apache License, Version 2.0. | ||
|
||
# | ||
# identifiers that need proper quoting | ||
# | ||
|
||
> CREATE SECRET mysqlpass AS '${arg.mysql-root-password}' | ||
> CREATE CONNECTION mysql_conn TO MYSQL ( | ||
HOST mysql, | ||
USER root, | ||
PASSWORD SECRET mysqlpass | ||
) | ||
|
||
$ mysql-connect name=mysql url=mysql://root@mysql password=${arg.mysql-root-password} | ||
|
||
$ mysql-execute name=mysql | ||
DROP DATABASE IF EXISTS public; | ||
CREATE DATABASE public; | ||
USE public; | ||
CREATE TABLE `t``1` (`f``1` INT) | ||
INSERT INTO `t``1` VALUES(1); | ||
|
||
> CREATE SOURCE mz_source FROM MYSQL CONNECTION mysql_conn; | ||
> CREATE TABLE t1 FROM SOURCE mz_source (REFERENCE public."t`1"); | ||
|
||
> SELECT * FROM t1; | ||
1 |
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(sorry for the drive-by review) Do we have to worry about SQL injection here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 implementationThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nice!