feat(cdc): support alter cdc table schema #17334

StrikeW · 2024-06-19T03:18:01Z

I hereby agree to the terms of the RisingWave Labs, Inc. Contributor License Agreement.

What's changed and what's your intention?

Support ALTER ADD/DROP COLUMN for cdc table based on shared source

close #16287

Checklist

I have written necessary rustdoc comments
I have added necessary unit tests and integration tests
I have added test labels as necessary. See details.
I have added fuzzing tests or opened an issue to track them. (Optional, recommended for new SQL features Sqlsmith: Sql feature generation #7934).
My PR contains breaking changes. (If it deprecates some features, please create a tracking issue to remove them in the future).
All checks passed in ./risedev check (or alias, ./risedev c)
My PR changes performance-critical code. (Please run macro/micro-benchmarks and show the results.)

My PR contains critical fixes that are necessary to be merged into the latest release. (Please check out the details)

Documentation

My PR needs documentation updates. (Please use the Release note section below to summarize the impact on users)

Release note

If this PR includes changes that directly affect users or other significant modifications relevant to the community, kindly draft a release note to provide a concise summary of these changes. Please prioritize highlighting the impact these changes will have on users.

gitguardian · 2024-06-25T07:01:58Z

⚠️ GitGuardian has uncovered 4 secrets following the scan of your pull request.

Please consider investigating the findings and remediating the incidents. Failure to do so may lead to compromising the associated services or software components.

🔎 Detected hardcoded secrets in your pull request

GitGuardian id	GitGuardian status	Secret	Commit	Filename
9425213	Triggered	Generic Password	`44bebe4`	ci/scripts/e2e-source-test.sh	View secret
9425213	Triggered	Generic Password	`44bebe4`	ci/scripts/e2e-source-test.sh	View secret
9425213	Triggered	Generic Password	`4b55d07`	ci/scripts/e2e-source-test.sh	View secret
9425213	Triggered	Generic Password	`4b55d07`	ci/scripts/e2e-source-test.sh	View secret

🛠 Guidelines to remediate hardcoded secrets

Understand the implications of revoking this secret by investigating where it is used in your code.
Replace and store your secrets safely. Learn here the best practices.
Revoke and rotate these secrets.
If possible, rewrite git history. Rewriting git history is not a trivial act. You might completely break other contributing developers' workflow and you risk accidentally deleting legitimate data.

To avoid such incidents in the future consider

following these best practices for managing and storing secrets including API keys and other credentials
install secret detection on pre-commit to catch secret before it leaves your machine and ease remediation.

^{🦉 GitGuardian detects secrets in your source code to help developers and security teams secure the modern development process. You are seeing this because you or someone else with access to this repository has authorized GitGuardian to scan your pull request.}

e2e_test/source/cdc_inline/alter/cdc_table_alter.slt

BugenZhao · 2024-06-26T06:12:37Z

e2e_test/source/cdc_inline/alter/cdc_table_alter.slt

+  hostname = '${MYSQL_HOST:localhost}',
+  port = '${MYSQL_TCP_PORT:8306}',
+  username = 'root',
+  password = '123456',
+  database.name = 'testdb1',
+  server.id = '5185'


Can we use ${RISEDEV_MYSQL_WITH_OPTIONS_COMMON}?

Seems that similar comments have been made in #16986 (comment).

I tried it but it doesn't work when I run test locally with risedev slt. It seems we have to launch the mysql via risedev to let the RISEDEV_MYSQL_WITH_OPTIONS_COMMON work, but I use docker compose to launch mysql locally.

It seems we have to launch the mysql via risedev to let the RISEDEV_MYSQL_WITH_OPTIONS_COMMON work, but I use docker compose to launch mysql locally.

This is true. However, I would still recommend launching MySQL with RiseDev as it makes it easier for other developers to test locally without needing to follow any other conventions.

but I use docker compose to launch mysql locally.

You CAN still use your own setup, by using user-managed.

- use: mysql port: 3306 user-managed: true # - In user-managed mode, these configs are not validated by risedev. # They are passed as-is to risedev-env default user for MySQL operations. # - This is not used in RISEDEV_MYSQL_WITH_OPTIONS_COMMON. user: root password: "" database: "risedev"

Any reason why you don't use the mysql started by risedev? Have you tried it and met any problems?

I can stop and start one or more db services with clicks, no need to edit the yaml.

You still uses risedev to start RisingWave. So relying on risedev can also save the click.. Why not?

Anyway, I encourage you to just give it a try. I believe basically no effort is needed to change the workflow and the DX improvement is gained for free.

If it doesn't work well, please share some feedbacks about what is not good enough, so that we can keep improving the DX for all developers.

If the reasons above are still not convincing enough to you, please also tell me why, so that I can understand it better.

Finally, if you just don't want to try it, it's also OK, but please use the suggested style for the benefits of others. For it to work in your machine, I think it's just some additional env vars.

You don't need to manually set MYSQL_ env vars manually.
When you change the configs, the env vars are also updated automatically.

Thanks for the clarification. I got your idea.
I find that the mysql -e can read the env vars in the risedev-env file, but psql -c cannot. I still need to set those $PGxxx env vars in my shell to make the psql -c work locally. I checked that postgres related env vars has written into the risedev-env file. Do you have any idea? @xxchan
Here is the config I used:

default: steps: - use: meta-node - use: compute-node - use: frontend - use: postgres port: 8432 user-managed: true application: "connector" user: myuser password: "123456" database: "mydb" - use: mysql port: 8306 user-managed: true user: root password: "123456" database: "mydb"

Sorry for missing this comment.

Just to double confirm: Did you run with risedev slt? And what's the errors you found?

Perhaps you can try this first:

system ok echo $PGPORT ---- placeholder

Then running risedev slt will return errors "result mismatch", and shows what's the output of the echo command, i.e., the value of PGPORT.

Sorry for missing this comment.

Wait, why this reply is "2 days ago", but I really just received it just now. 😂

Sorry for missing this comment.

Wait, why this reply is "2 days ago", but I really just received it just now. 😂

Maybe due to I forget to submit the pending comment. 😄

src/frontend/src/handler/alter_table_column.rs

BugenZhao · 2024-06-26T06:26:08Z

src/frontend/src/handler/alter_table_column.rs

+            // Add the new column to the table definition if it is not created by `create table (*)` syntax.
+            if !columns.is_empty() {


This doesn't look good to me. 😕

I remember that we had a discussion long ago (frontend: refactor source schema resolution #9828) that we should purify the definition of a table/source before persisting it into the catalog. In this case, for example, * is completely a syntax-sugar and should be first expanded to column definitions. cc @st1page

Considering empty columns as created with * is also a fragile assumption. Users are allowed to create a table without columns like CREATE TABLE t;.

sorry for forgetting this point during my previous review of the #16986.
I think we should expand on CREATE TABLE (*). This is because operations such as CREATE SINK INTO TABLE, ALTER TABLE RENAME, and others rely on reparsing the SQL statement, and the parsed columns should remain consistent with the previous plan.

I think cdc table was the first to meet this problem, because in other scenarios with a schema registry, we simply prevented users from doing things like add columns.
https://github.com/risingwavelabs/risingwave/blob/main/src/frontend/src/handler/alter_table_column.rs#L117
https://github.com/risingwavelabs/risingwave/blob/main/src/frontend/src/handler/alter_source_column.rs#L73

And the only way to change their schema was to refresh the schema registry using alter table, which implied the assumption that their which implies the assumption that their schema needs to always be aligned upstream.

Before finding a elegent way to support that. We can tentatively assume that the cdc table of create table(*) is similar to the table with connector of the schema registry. Let's temporarily prevent users from using add columns on such a table?

I think for this PR we can prevent users from adding/drop columns on a table with wildcard.
And here is the future design #17472

I think for this PR we can prevent users from adding/drop columns on a table with wildcard.

ok, we can ban ALTER for tables created by create table(*) in this pr.

ok, we can ban ALTER for tables created by create table(*) in this pr.

Does this also mean that we no longer need let mut columns_altered = original_catalog.columns.clone();?

e2e_test/source/cdc_inline/alter/cdc_table_alter.slt

cyliu0 · 2024-07-02T00:44:39Z

/test

risingwave-ci · 2024-07-02T00:58:35Z

Failed: 3
Passed:0
FailedExecutions:
https://buildkite.com/risingwave-test/ch-benchmark-mysql-cdc-shared-source/builds/84
https://buildkite.com/risingwave-test/ch-benchmark-mysql-cdc-shared-source/builds/85
https://buildkite.com/risingwave-test/tpcc-mysql-cdc-shared-source/builds/28
PassedExecutions:

Please ignore this, the pipelines got bugs.

BugenZhao

The code impl generally LGTM

BugenZhao · 2024-07-04T04:34:49Z

src/meta/src/rpc/ddl_controller.rs

@@ -1971,7 +2000,6 @@ impl DdlController {
        } = actor_graph_builder
            .generate_graph(&self.env, stream_job, expr_context)
            .await?;
-        assert!(dispatchers.is_empty());


Can we still keep this assertion depending on the table job type?

src/meta/src/manager/catalog/mod.rs

src/frontend/src/handler/create_table.rs

BugenZhao

Generally LGTM!

e2e_test/source/cdc_inline/alter/cdc_table_alter.slt

alter cdc table

6f86b92

StrikeW changed the title ~~feat(cdc): support alter cdc table schema~~ feat(cdc): support alter cdc table schema (WIP) Jun 19, 2024

github-actions bot added type/feature ci/run-e2e-single-node-tests labels Jun 19, 2024

StrikeW added 5 commits June 20, 2024 19:24

finish alter add

feec30e

finish alter drop

6ba5f0c

minor

9faad96

add e2e test

ff59a0d

format

bb5b9ad

StrikeW added ci/run-build ci/run-build-other ci/run-e2e-source-tests ci/run-check ci/pr/run-selected labels Jun 25, 2024

minor

7e8e936

minor

44bebe4

StrikeW force-pushed the siyuan/cdc-alter-table branch from 6ce9713 to 44bebe4 Compare June 25, 2024 07:20

fix

4b55d07

StrikeW force-pushed the siyuan/cdc-alter-table branch from 9f0fee7 to 4b55d07 Compare June 25, 2024 08:15

StrikeW added 2 commits June 25, 2024 19:40

fix

ce31e20

fix final

c7dab61

StrikeW removed ci/run-build ci/run-build-other ci/run-e2e-source-tests ci/run-check ci/run-e2e-single-node-tests ci/pr/run-selected labels Jun 25, 2024

StrikeW changed the title ~~feat(cdc): support alter cdc table schema (WIP)~~ feat(cdc): support alter cdc table schema Jun 25, 2024

StrikeW marked this pull request as ready for review June 25, 2024 15:10

StrikeW requested review from shanicky, yezizp2012 and BugenZhao June 26, 2024 02:02

BugenZhao requested a review from xxchan June 26, 2024 05:30

BugenZhao reviewed Jun 26, 2024

View reviewed changes

BugenZhao requested a review from st1page June 26, 2024 06:29

xxchan reviewed Jun 26, 2024

View reviewed changes

e2e_test/source/cdc_inline/alter/cdc_table_alter.slt Outdated Show resolved Hide resolved

cyliu0 added the mysql-cdc label Jul 1, 2024

StrikeW added 4 commits July 3, 2024 22:14

fix comments

2c93b06

minor

fb08929

Merge remote-tracking branch 'origin/main' into siyuan/cdc-alter-table

c69cf71

minor

56ad830

StrikeW requested review from BugenZhao and xxchan July 4, 2024 00:30

BugenZhao reviewed Jul 4, 2024

View reviewed changes

StrikeW added 2 commits July 5, 2024 00:03

refactor

e2d392c

fix

46d224b

StrikeW requested a review from BugenZhao July 5, 2024 03:50

BugenZhao approved these changes Jul 5, 2024

View reviewed changes

e2e_test/source/cdc_inline/alter/cdc_table_alter.slt Outdated Show resolved Hide resolved

StrikeW added 2 commits July 7, 2024 17:56

fix alter table during cdc backfill

4d325ac

minor

3e56a60

StrikeW enabled auto-merge July 7, 2024 10:02

StrikeW added this pull request to the merge queue Jul 7, 2024

Merged via the queue into main with commit 739d005 Jul 7, 2024
31 of 33 checks passed

StrikeW deleted the siyuan/cdc-alter-table branch July 7, 2024 11:01

KeXiangWang pushed a commit that referenced this pull request Jul 18, 2024

feat(cdc): support alter cdc table schema (#17334)

07a9a66

xxchan mentioned this pull request Aug 6, 2024

feat(cdc): support sql server cdc #17429

Merged

9 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(cdc): support alter cdc table schema #17334

feat(cdc): support alter cdc table schema #17334

StrikeW commented Jun 19, 2024 •

edited

Loading

gitguardian bot commented Jun 25, 2024 •

edited

Loading

BugenZhao Jun 26, 2024

StrikeW Jun 26, 2024

BugenZhao Jun 27, 2024

xxchan Jun 27, 2024 •

edited

Loading

xxchan Jun 27, 2024

xxchan Jun 27, 2024

StrikeW Jul 2, 2024

xxchan Jul 4, 2024

xxchan Jul 4, 2024

StrikeW Jul 5, 2024

BugenZhao Jun 26, 2024 •

edited

Loading

st1page Jun 26, 2024

st1page Jun 26, 2024

st1page Jun 26, 2024

st1page Jun 26, 2024

StrikeW Jun 27, 2024

BugenZhao Jun 28, 2024

cyliu0 commented Jul 2, 2024

risingwave-ci commented Jul 2, 2024 •

edited by cyliu0

Loading

BugenZhao left a comment

BugenZhao Jul 4, 2024

StrikeW Jul 5, 2024

BugenZhao left a comment

		// Add the new column to the table definition if it is not created by `create table (*)` syntax.
		if !columns.is_empty() {

feat(cdc): support alter cdc table schema #17334

feat(cdc): support alter cdc table schema #17334

Conversation

StrikeW commented Jun 19, 2024 • edited Loading

What's changed and what's your intention?

Checklist

Documentation

Release note

gitguardian bot commented Jun 25, 2024 • edited Loading

⚠️ GitGuardian has uncovered 4 secrets following the scan of your pull request.

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xxchan Jun 27, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BugenZhao Jun 26, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cyliu0 commented Jul 2, 2024

risingwave-ci commented Jul 2, 2024 • edited by cyliu0 Loading

BugenZhao left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BugenZhao left a comment

Choose a reason for hiding this comment

StrikeW commented Jun 19, 2024 •

edited

Loading

gitguardian bot commented Jun 25, 2024 •

edited

Loading

xxchan Jun 27, 2024 •

edited

Loading

BugenZhao Jun 26, 2024 •

edited

Loading

risingwave-ci commented Jul 2, 2024 •

edited by cyliu0

Loading