source-postgres-batch: Paying down some technical debt #2406
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This more or less completely replaces the source-postgres-batch test suite, which previously...well, it existed. The new tests should exercise much more of the connector's behavior, including how it discovers and serializes all notable column types.
This modifies source-postgres-batch to use a function which takes a resource as input and returns the query template to use, where previously there was just a single constant template. The reason for this is that in the near future it would be nice to only use the XMIN incremental query if the cursor is ["xmin"] and otherwise use a full-refresh or cursor-incremental query instead, and since we're already omitting the default query from discovered resources there is no reason to try and shoehorn all of that intelligence into an overly complicated Go template.
willdonnelly
force-pushed
the
wgd/batch-postgres-tech-debt-20250214
branch
from
6623764 to
e931ede
Compare
As discussed in the comment, this is a workaround for a cause of test snapshot instability. The fact that the workaround is needed is itself indicative of a bug, but for now I just want tests to demonstrate current behavior.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description:
This PR completely replaces the
source-postgres-batchtest suite with a new much more comprehensive version, and adds the necessary docker-compose / ci.yaml setup bits to actually run all of those tests in CI builds. This will hopefully let us make future source-postgres-batch changes with more confidence that we haven't broken anything important.This PR also replaces
DefaultQueryTemplatewith aSelectQueryTemplatefunction to match what MySQL will have after its corresponding tech-debt PR is merged. The rationale for using a function here is that currently the Postgres connector only really works if you either:That's not great, ideally we would want to either not give users the option to use a cursor at all, or we'd want it to Just Work if you set a non-xmin cursor. And I would like to add support for discovering views in the near future, and views can't use XMIN for incremental syncs, so that means we should make things work as intended if the user doesn't specify their own query template but modifies the cursor.
Which means that we'll need to conditionally switch behavior and only use the xmin-incremental query when the cursor is
["xmin"]. And we could either implement that logic as part of an increasingly-horrible Go template, or we could just implement it in Go. I'm choosing the latter, but since this PR is supposed to have no user-visible functional impact this is just the preparatory refactoring.Workflow steps:
No user-visible changes intended.
This change is