-
Notifications
You must be signed in to change notification settings - Fork 583
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
feat(cdc): support more metadata columns for MySQL, PG and MongoDB #17051
Conversation
database_name
and table_name
meta column for MySQL, PG and MongoDBdatabase_name
and table_name
meta column for MySQL, PG and MongoDB
database_name
and table_name
meta column for MySQL, PG and MongoDB@@ -215,6 +224,10 @@ message AdditionalColumn { | |||
AdditionalColumnHeader header_inner = 5; | |||
AdditionalColumnFilename filename = 6; | |||
AdditionalColumnHeaders headers = 7; | |||
AdditionalDatabaseName database_name = 8; |
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.
Since they're all empty messages, can we use the same message type instead? Or directly use google.protobuf.Empty
?
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.
IIRC, prost has some trouble with google well-known types.
& I use message structs for future extensibility on the column def, such as
message AdditionalColumnHeader {
string inner_field = 1;
data.DataType data_type = 2;
}
we won't want to handle the changes inside the catalog in the future.
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.
message AdditionalColumn {
oneof column_type {
AdditionalColumnKey key = 1;
AdditionalColumnTimestamp timestamp = 2;
AdditionalColumnPartition partition = 3;
AdditionalColumnOffset offset = 4;
AdditionalColumnHeader header_inner = 5;
AdditionalColumnFilename filename = 6;
AdditionalColumnHeaders headers = 7;
}
}
I think we need different message type for the oneof
.
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.
the impl LGTM, just to confirm cdc source does not need the additional column to maintain its states so no need for compatible code?
No need compatible code for cdc sources and tables defined in previous version. User need to recreate their source or table to get the metadata column. |
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.
Rest LGTM
I hereby agree to the terms of the RisingWave Labs, Inc. Contributor License Agreement.
What's changed and what's your intention?
Support more metadata columns for cdc table
database_name
,schema_name
,table_name
for PG and MySQLcollection_name
for MongoDBExample:
close #16654
Checklist
./risedev check
(or alias,./risedev c
)Documentation
Release note
Support more metadata columns for cdc table
database_name
,schema_name
,table_name
for PG and MySQLcollection_name
for MongoDB