Skip to content
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

fix(cdc): fix wrong default column matching #20348

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

KeXiangWang
Copy link
Contributor

@KeXiangWang KeXiangWang commented Jan 31, 2025

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

What's changed and what's your intention?

Fix the bug introduced in #19354. Resolve #20346
The bug will cause two problems:

  1. Differences in the orders of rw schema and pg schema can lead to wrong default value assigned.
  2. Due to zip_eq_fast, if the numbers of columns in rw schema and pg schema are different, panic happens.

Checklist

  • I have written necessary rustdoc comments.
  • I have added necessary unit tests and integration tests.
  • I have added test labels as necessary.
  • I have added fuzzing tests or opened an issue to track them.
  • My PR contains breaking changes.
  • My PR changes performance-critical code, so I will run (micro) benchmarks and present the results.
  • My PR contains critical fixes that are necessary to be merged into the latest release.

Documentation

  • My PR needs documentation updates.
Release note

Copy link
Member

@BugenZhao BugenZhao left a comment

Choose a reason for hiding this comment

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

Rest LGTM

Comment on lines 1199 to 1202
if let Some(external_col) = external_columns.get(col.name()) {
col.column_desc.generated_or_default_column =
external_col.column_desc.generated_or_default_column.clone();
}
Copy link
Member

Choose a reason for hiding this comment

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

Is it possible that we don't find a column in external_columns? Should we return an error (just like in #20203)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I cannot find a case for now. But a potential case is #19112.
An example haven't been supported but to be supported:

CREATE TABLE test (
    id int,
    name varchar,
    my_id int as id + 1, // generated column
    PRIMARY KEY (id)
) FROM pg_source TABLE 'public.test';

I can make it throw an error for now.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated

Copy link
Contributor Author

@KeXiangWang KeXiangWang Jan 31, 2025

Choose a reason for hiding this comment

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

The same logic can be found here, so it's also OK not to throw an error.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

postgres_cdc: fails miserably when there are less columns in RW table than in PG source table
2 participants