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

DBZ-1163 Enable postgres snapshot for tables without primary key #824

Closed
wants to merge 1 commit into from

Conversation

preethi29
Copy link
Contributor

No description provided.

@preethi29
Copy link
Contributor Author

any comments or updates on this PR? @jpechane

Copy link
Contributor

@jpechane jpechane left a comment

Choose a reason for hiding this comment

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

@preethi29 Just a single comment regarding code formatting
@gunnarmorling LGTM, any objections from your side?

@@ -392,8 +392,7 @@ protected void generateReadRecord(TableId tableId, Object[] rowData) {
Object key = tableSchema.keyFromColumnData(rowData);
Struct value = tableSchema.valueFromColumnData(rowData);

//note this is different than implementation in Streams producer. See DBZ-1163
if (key == null || value == null) {
if ( value == null) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Superfluous space after (

Copy link
Contributor Author

Choose a reason for hiding this comment

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

this is fixed

@gunnarmorling
Copy link
Member

Thanks a lot, @preethi29!

It'd be good to have a test for this in RecordsStreamProducerIT, too. Another question is what to use as message key in this case:

  1. simply null
  2. one (the first) unique constraint for that table

IIRC, we do 2. for MySQL. Perhaps you could double check on this and see whether the same could be done for Postgres? @jpechane, do you know from the top of your head how it is for SQL Server? It'd be nice to have a consistent behavior for all the (relational) connectors eventually.

@gunnarmorling
Copy link
Member

Ok, so we indeed use the first unique column as message key for MySQL. Let's wait to hear back from @jpechane about SQL Server. I think we should do the same for PG.

@jpechane
Copy link
Contributor

jpechane commented Apr 3, 2019

@gunnarmorling Both SQL Server on Oracle drops non-key messages. MySQL requires either key or value to be non null to send the message.

@preethi29
Copy link
Contributor Author

@jpechane @gunnarmorling What scenario should we test on RecordsStreamProducerIT

About the key, I am little lost with searching the code for this behaviour in MYSQL. Can you point me to the right code?

@preethi29 preethi29 force-pushed the DBZ-1163 branch 2 times, most recently from c7ae5d3 to e8f20dc Compare April 6, 2019 07:40
@preethi29
Copy link
Contributor Author

The current behaviour is to send key as null in postgres. One other thing to note is, key is null when the table has replica identity configured and no primary key.

@gunnarmorling
Copy link
Member

Thanks a lot, @preethi29! I've rebased and applied this fix. I've also filed DBZ-1225 as a follow-up, so to make this behavior in case of null keys consistent across all the relational Debezium connectors. If you'd like to help with that, e.g. for the Postgres connector, please let us know :)

Finally, I didn't have your last name, so I've just added you as "Preethi S" to the list of all contributors in COPYRIGHT.txt. If you give me the full name, I can add that instead. Thanks again!

@preethi29
Copy link
Contributor Author

Thanks @gunnarmorling. Sure I can help with DBZ-1225 for postgres connector.
My full name is 'Preethi Sadagopan'.

@gunnarmorling
Copy link
Member

Sure I can help with DBZ-1225 for postgres connector.

Thanks, that sounds great! I'd suggest you add a comment to that issue in JIRA then so that others know you're working on it. If you run into any issues, please let us know.

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

Successfully merging this pull request may close these issues.

3 participants