[source-postgres] Disable incremental sync for view in Postgres xmin replication mode #42108
Conversation
| The latest updates on your projects. Learn more about Vercel for Git ↗︎ 1 Skipped Deployment
|
theyueli changed the title 
octavia-squidington-iii added area/connectors 
| } | ||
| | ||
| public static AirbyteStream setFullRefreshToView(final JdbcDatabase database, final AirbyteStream stream) throws SQLException { | ||
| if (isStreamAView(database, stream)) { |
There was a problem hiding this comment.
I think this means we will run this query N times, once for all N streams. Could we load up the list of streams once instead?
There was a problem hiding this comment.
Great idea! I just pushed a commit that implements this idea. Please take another look, thanks!
octavia-squidington-iii added the area/documentation 
| */ | ||
| public static AirbyteStream overrideSyncModes(final AirbyteStream stream, Map<String, List<String>> viewsBySchema) { | ||
| if (isStreamAView(viewsBySchema, stream)) { | ||
| return stream.withSupportedSyncModes(Lists.newArrayList(SyncMode.FULL_REFRESH)); |
There was a problem hiding this comment.
I'm not sure, but I thought in an xmin sync, full refreshes were still incremental... and the therefore still dependent on a usable cursor?
There was a problem hiding this comment.
I think what you mentioned is for the cursor-based sync mode;
maybe I'm wrong, but from what I tested, it does not require specifying a cursor in xmin (see picture below)
There was a problem hiding this comment.
The full refreshes are still incremental (based on CTID), but I didn't think a cursor needed to be explicitly set? So I think fixing the sync modes might work
cc : @xiaohansong who might know this
There was a problem hiding this comment.
yes - cursor is not needed for full refresh. We should support full refresh anyway, for views they would be non resumable.
...ce-postgres/src/main/java/io/airbyte/integrations/source/postgres/PostgresCatalogHelper.java Show resolved Hide resolved
…replication mode (#42108) Fixes airbytehq/airbyte-internal-issues#8559 Fixes airbytehq/oncall#5581 This patch disables incremental sync when a stream is found to be a view. This is done only for Postgres sources configured in xmin mode. This is done by adding a check in discovery() to only allow FULL_REFRESH sync mode.
| @theyueli @xiaohansong If I understand this change correctly, we can no longer sync a view in an incremental mode, right? I think instead of removing the option out right, this change should be implemented in a different way. |
Only when using the xmin replication mode |
| @evantahler precisely, that's what our usecase is. We just have a filter on the start date of the source table in our view, everything else stays as is. |
…es xmin replication mode (airbytehq#42108)" This reverts commit c7a1213
What
Fixes https://github.com/airbytehq/airbyte-internal-issues/issues/8559
Fixes https://github.com/airbytehq/oncall/issues/5581
This patch disables incremental sync when a stream is found to be a view. This is done only for Postgres sources configured in xmin mode.
How
Adding a check in discovery() to only allow
FULL_REFRESHsync mode.Review guide
User Impact
In the UI, user will only see full refresh sync option for view. See the picture below.
Can this PR be safely reverted and rolled back?