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

[source-postgres] CdcPostgresSourceLegacyCtidTest isn't testing against postgres 13 #35267

Closed
xiaohansong opened this issue Feb 14, 2024 · 2 comments
Labels
team/db-dw-sources Backlog for Database and Data Warehouse Sources team type/bug Something isn't working

Comments

@xiaohansong
Copy link
Contributor

https://github.com/airbytehq/airbyte/blob/master/airbyte-integrations/connectors/source-postgres/src/test/java/io/airbyte/integrations/source/postgres/CdcPostgresSourceLegacyCtidTest.java

After we refactored test infra this does not actually test running on legacy postgres versions. We should update that.

I've tried to update that but encountered failures among unit tests: during the initial scan, lsn state in the last state message is different than the previous state message; this only happens on postgres 13 but not 16.

In the test https://github.com/airbytehq/airbyte/blob/master/airbyte-integrations/connectors/source-postgres/src/test/java/io/airbyte/integrations/source/postgres/CdcPostgresSourceTest.java#L199 expects state message stays the same in the same batch. However in postgres 13, the last state message will mutate. See example below:

Previous state message:
"[\"db_dlpenhggyr\",{\"server\":\"db_dlpenhggyr\"}]": "{\"transaction_id\":null,\"lsn\":24834904,\"txId\":678,\"ts_usec\":1707870101598505}"

Final state message generated by PostgresCdcStateHandler:

"[\"db_dlpenhggyr\",{\"server\":\"db_dlpenhggyr\"}]": "{\"transaction_id\":null,\"lsn_proc\":24867752,\"lsn_commit\":24867752,\"lsn\":24867752,\"txId\":678,\"ts_usec\":1707870101598505}"

Some research would be required to understand why the behavior is different, and determine if we have an underlying bug/data loss potential in this case.

@xiaohansong xiaohansong added the team/db-dw-sources Backlog for Database and Data Warehouse Sources team label Feb 14, 2024
@xiaohansong xiaohansong changed the title CdcPostgresSourceLegacyCtidTest isn't testing against postgres 13 [source-postgres] CdcPostgresSourceLegacyCtidTest isn't testing against postgres 13 Feb 14, 2024
@evantahler evantahler added the type/bug Something isn't working label Feb 14, 2024
@evantahler
Copy link
Contributor

evantahler commented Feb 14, 2024

Grooming:

  • we need research: is this a real bug or a testing problem. There are legacy and non-legacy tests. The legacy test is passing, but it shouldn't - it's not testing the right thing - when updated, the test fails. The 'latest' test works ok.
  • If this is a real bug, regroup and decide on priority.

@xiaohansong
Copy link
Contributor Author

We have a 'hack' to advance LSN for version < 15:

https://github.com/airbytehq/airbyte/blob/master/airbyte-integrations/connectors/source-postgres/src/main/java/io/airbyte/integrations/source/postgres/PostgresUtils.java#L198

That will change the state and cause the test to fail. Nothing wrong on prod though.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
team/db-dw-sources Backlog for Database and Data Warehouse Sources team type/bug Something isn't working
Projects
None yet
Development

No branches or pull requests

2 participants