Perform logical replication after initial sync #144

judahrand · 2022-01-11T15:16:29Z

Problem

If you select new tables to add to an existing log based extract, it'll do a "logical_initial" full table sync on those new tables each and every time the tap runs. It'll never switch over to log based replication for them.

Another example of this issue is that if intend to run with break_at_end_lsn: False then I have to run the tap twice - once to perform the initial sync and again to start the logical replication.

Proposed changes

This Pull Request contains changes which are simpler and, I believe, more correct than those in #130. Both Pull Requests aim to solve #107.

My changes ensure that whenever a LOG_BASED replication runs the bookmark is initialised correctly and any logical replication which is required is performed.

Types of changes

What types of changes does your code introduce to PipelineWise?
Put an x in the boxes that apply

Bugfix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to not work as expected)
Documentation Update (if none of the other choices apply)

Checklist

Description above provides context of the change
I have added tests that prove my fix is effective or that my feature works
Unit tests for changes (not needed for documentation changes)
CI checks pass with my changes
Bumping version in setup.py is an individual PR and not mixed with feature or bugfix PRs
Commit message/PR title starts with [AP-NNNN] (if applicable. AP-NNNN = JIRA ID)
Branch name starts with AP-NNN (if applicable. AP-NNN = JIRA ID)
Commits follow "How to write a good git commit message"
Relevant documentation is updated including usage instructions

judahrand · 2022-01-11T15:17:21Z

@Samira-El Would you be happy with these changes instead of #130? I believe they should address the same issue.

judahrand · 2022-01-17T10:47:36Z

@Samira-El Could you please review this?

judahrand · 2022-01-18T15:31:21Z

@koszti Would you be willing to review this?

…-tap-postgres into issue-107

judahrand · 2022-01-21T15:14:23Z

@Samira-El I believe this should also be ready for a review. This would also allow the closure of #131 and #130, I believe.

deanmorin · 2022-01-27T01:24:53Z

As the author of #130 I agree that this is a better fix. I've closed that PR in favor of this one.

Is there anything preventing this from being merged @Samira-El ?

deanmorin · 2022-01-27T23:41:26Z

I'm actually getting an error running this branch, I'll need to do some more investigation:

TypeError: '<' not supported between instances of 'NoneType' and 'int'
lsn_comitted = min(
File "/project/.meltano/extractors/tap-provider/venv/lib/python3.9/site-packages/tap_postgres/sync_strategies/logical_replication.py", line 684, in sync_tables

judahrand · 2022-01-28T00:52:38Z

I'm actually getting an error running this branch, I'll need to do some more investigation:

TypeError: '<' not supported between instances of 'NoneType' and 'int'
lsn_comitted = min(
File "/project/.meltano/extractors/tap-provider/venv/lib/python3.9/site-packages/tap_postgres/sync_strategies/logical_replication.py", line 684, in sync_tables

Hmmm... Interesting. I'm confused as to how this could be caused by the changes here as the line you're seeing a failure on is also run right at the beginning of that function:
https://github.com/thread/pipelinewise-tap-postgres/blob/64e1f919411993e12148e6b9bd181817f3cf37bf/tap_postgres/sync_strategies/logical_replication.py#L557

So theNone couldn't be present at the start?

Samira-El · 2022-01-28T14:00:02Z

Is there anything preventing this from being merged @Samira-El ?

Time, I don't have time to review yet it 😓. This is crucial tap for us, so PRs need to be properly reviewed.

judahrand · 2022-02-11T11:15:44Z

@Samira-El Any ideas when you might have the time to review this? We're currently running a fork and would of course love to be able to minimize the divergence!

Samira-El · 2022-02-23T10:06:08Z

Hey Judah, I'm running into the same error encountered by Dean:

tap_postgres/sync_strategies/logical_replication.py", line 684, in sync_tables
    lsn_comitted = min(
TypeError: '<' not supported between instances of 'NoneType' and 'int'

I'm following the bug reproduction step in #107

judahrand · 2022-02-23T10:24:21Z

@Samira-El I think my latest commit fixes this issue.

Samira-El · 2022-01-19T15:12:10Z

tests/test_full_table_interruption.py

+        assert CAUGHT_MESSAGES[8]['type'] == 'SCHEMA'
+
+        assert isinstance(CAUGHT_MESSAGES[9], singer.messages.StateMessage)
+        assert CAUGHT_MESSAGES[9].value['bookmarks']['public-COW'].get('xmin') is None
+        assert CAUGHT_MESSAGES[9].value['bookmarks']['public-COW'].get('lsn') == end_lsn
+        assert CAUGHT_MESSAGES[9].value['bookmarks']['public-COW'].get('version') == new_version
+


Can you use TestCase assert* methods instead?

@Samira-El I'm facing this issue and planning on contributing the fix. Is your feedback related to line 268 only? That is change it to assertIsInstance(CAUGHT_MESSAGES[9], singer.messages.StateMessage). Asking this since I see all other assert statements don't use TestCase either.

Samira-El · 2022-04-20T13:13:43Z

Hey folks, just had the time to review this PR, the fix LGTM, I left a comment on the tests tho.

The CI is broken because of a recent merged PR, we will roll it back and rerun the tests on this one.

judahrand mentioned this pull request Jan 11, 2022

Cannot add more tables to existing postgres->snowflake log based extract #107

Open

judahrand force-pushed the issue-107 branch from 5569fec to 45a9a9d Compare January 18, 2022 11:55

Perform logical replication after initial sync

b2857b5

judahrand force-pushed the issue-107 branch from c961aea to 632e63d Compare January 18, 2022 15:30

Actually run the initial LOG_BASED sync in test

d69d686

judahrand force-pushed the issue-107 branch from 632e63d to d69d686 Compare January 18, 2022 15:32

judahrand added 2 commits January 21, 2022 15:11

Merge branch 'master' of https://github.com/transferwise/pipelinewise…

83363ee

…-tap-postgres into issue-107

Update test images to enable wal2json

c3fcc08

Use fixed version of wal2json

64e1f91

deanmorin mentioned this pull request Jan 27, 2022

Fixes https://github.com/transferwise/pipelinewise-tap-postgres/issues/107 #130

Closed

13 tasks

Ensure lsn_comitted is defined

f7fbdb2

judahrand and others added 2 commits February 23, 2022 10:24

Merge branch 'master' into issue-107

eaf87ad

Merge branch 'master' into issue-107

4a46a43

Samira-El suggested changes Apr 20, 2022

View reviewed changes

Merge branch 'master' into issue-107

26d7a13

josescuderoh mentioned this pull request Apr 24, 2023

Perform logical replication after initial sync (continuation) #218

Closed

13 tasks

josescuderoh mentioned this pull request May 1, 2023

Perform logical replication after initial sync (continuation) #219

Open

13 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Perform logical replication after initial sync #144

Perform logical replication after initial sync #144

judahrand commented Jan 11, 2022 •

edited

Loading

judahrand commented Jan 11, 2022

judahrand commented Jan 17, 2022

judahrand commented Jan 18, 2022

judahrand commented Jan 21, 2022

deanmorin commented Jan 27, 2022

deanmorin commented Jan 27, 2022

judahrand commented Jan 28, 2022 •

edited

Loading

Samira-El commented Jan 28, 2022

judahrand commented Feb 11, 2022

Samira-El commented Feb 23, 2022 •

edited

Loading

judahrand commented Feb 23, 2022

Samira-El Jan 19, 2022

josescuderoh Apr 21, 2023 •

edited

Loading

Samira-El commented Apr 20, 2022

Perform logical replication after initial sync #144

Are you sure you want to change the base?

Perform logical replication after initial sync #144

Conversation

judahrand commented Jan 11, 2022 • edited Loading

Problem

Proposed changes

Types of changes

Checklist

judahrand commented Jan 11, 2022

judahrand commented Jan 17, 2022

judahrand commented Jan 18, 2022

judahrand commented Jan 21, 2022

deanmorin commented Jan 27, 2022

deanmorin commented Jan 27, 2022

judahrand commented Jan 28, 2022 • edited Loading

Samira-El commented Jan 28, 2022

judahrand commented Feb 11, 2022

Samira-El commented Feb 23, 2022 • edited Loading

judahrand commented Feb 23, 2022

Samira-El Jan 19, 2022

Choose a reason for hiding this comment

josescuderoh Apr 21, 2023 • edited Loading

Choose a reason for hiding this comment

Samira-El commented Apr 20, 2022

judahrand commented Jan 11, 2022 •

edited

Loading

judahrand commented Jan 28, 2022 •

edited

Loading

Samira-El commented Feb 23, 2022 •

edited

Loading

josescuderoh Apr 21, 2023 •

edited

Loading