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

redshift RA3 nodes support cross-db references, but dbt prevents this in compilation #3179

Closed
Tracked by #742
mike-weinberg opened this issue Mar 18, 2021 · 5 comments
Labels

Comments

@mike-weinberg
Copy link

https:/fishtown-analytics/dbt/blob/841b2115a42cc20658ec69ba129d515412993d10/core/dbt/adapters/base/relation.py#L440-L442

this is tricky because there is a long term fork in redshift behavior based on node type. RA3 nodes support cross db references (this is now GA) but gen2 node types do not. the redshift/postgres connector may need to be subclassed for ra3 node types with the db-ref-count omitted. It might be possible to distinguish between node types by issuing a version command.

If simplicity of implementation is preferred, an override could maybe be added to the project.yml spec (like, force-enable-cross-db=true or something as an option which defaults to false for the next release and automatic detection could be added in a future release?

I don't know the codebase well enough to assess what strategy would create the least tech debt.

@jtcohen6
Copy link
Contributor

Thanks for opening @mike-weinberg, and for the thoughtful writeup!

It might be possible to distinguish between node types by issuing a version command.

Yes, this feels like a major input: If you're talking to an RA3 node, how do you know? Does it tell you, by returning something specific in select version();? Would this information be more readily available if we switched to using the Redshift python driver instead of reusing the dbt-postgres logic (which connects via psycopg2)?

If that type of detection is possible, and it's something dbt can do once and store in the RedshiftConnectionManager or RedshiftAdapter object, there's some nice overlap with the best-case resolution for #3168.

Otherwise, I'd be in favor of a ra3: true | false config in profiles.yml. I agree that this fork in Redshift behavior feels significant, long-term, and not limited to a one-time thing we'd much rather hack around and quickly forget.

@mike-weinberg
Copy link
Author

@jtcohen6 I agree, ra3: true feels like a right-sized fix for the context. I don't love that it implies a behavior change rather than being an explicit switch, but i suppose this is likely not the last feature that will be ra3 only (I think the system tables are also forking).

I think there might be some value in adding a check at dbt startup to make sure that ra3=true ... is actually true, because customers can change node types, but that's not need-to-have functionality, it simply helps ensure that error handling follows the least-sad path

@jtcohen6
Copy link
Contributor

jtcohen6 commented Apr 8, 2021

I think there might be some value in adding a check at dbt startup to make sure that ra3=true ... is actually true, because customers can change node types, but that's not need-to-have functionality, it simply helps ensure that error handling follows the least-sad path

@mike-weinberg Right on. Some folks from the Redshift team shared a system query that can verify the current node type. It looks about the way you'd expect.

@salmonsd
Copy link

salmonsd commented Jul 29, 2021

Following up here, this seems to be resolved in #3408 and should be included is v0.21.

cc: @jtcohen6

@jtcohen6
Copy link
Contributor

You're right! This will be released in v0.21.0-b1 for starters, and it will require flipping on a ra3: true parameter in the Redshift profile. I'm going to close this issue :)

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

No branches or pull requests

3 participants