-
Notifications
You must be signed in to change notification settings - Fork 57
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
Restore transaction semantics used by dbt-redshift prior to 1.5 #475
Conversation
Thank you for your pull request! We could not find a changelog entry for this change. For details on how to document a change, see the dbt-redshift contributing guide. |
Ignore the 3.7 unit test. It's a lie |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A couple spellings to update.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@VersusFacit Does this solve #454 also?
Based on c613aae, it looks like the answer is "yes" 🎉
If so, could you add resolves #454
to the PR so we can auto-close it?
Github is now seeing the resolve 454 properly. TIL just how restrictive the syntax is |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks for this @VersusFacit!
some follow-on opportunities:
- what do end-users need to know about this change/parameter? at the least we need an update to the docs repo's redshift setup page, right?
- might
test_autocommit.py
actually be valuable in the generic adapter zone tests? this exercise of determining root cause has made me more suspicious than ever of how dbt handles transactions generally.
I'm going to post links in issues here -- basically if you used the autocommit flag, I'm taking that away. This should affect so very few people since it wasn't really documented and has only been like for 2 weeks I already updated the docs issue 👉 👉
No because in subtle ways transactions are handled differently by each db. We may find in time that it's worthwhile to do something like that but that would require a nontrivial investigation lift. This is a hornet's nest we need to deliberately schedule time for if we want to learn/refine this subroutine more. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would like to resolve the conversation of autocommit
vs. deactivate_autocommit
prior to merge.
agreed! which is why I've opened dbt-labs/dbt-core#9980 |
8c40dc8
to
5eeeb98
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
* Rename flag and get autocommit on * Add changie * Add test for issue 451 * Add test for issue #454 * Fix spelling. * Revert names * update tests * update the comment to use pep for rationale --------- Co-authored-by: Mila Page <versusfacit@users.noreply.github.com>
…timeout (#3456) ## What are you changing in this pull request and why? This PR is to update the docs for the following changes in dbt-redshift: 1. **[Mapping sslmode in psycopg2 to ssl & sslmode in redshift_connector](dbt-labs/dbt-redshift#439 In dbt-redshift 1.5, Python driver switched from psycopg2 to redshift_connector. redshift_connector has both ssl and sslmode parameters, while psycopg2 only has sslmode. We've made changes in dbt-redshift to convert psycopg2' accepted values of sslmode into ssl and sslmode parameters of redshift_connector. This PR explains the conversion logic and includes explanation of the sslmode flag. #3365 2. **[Adding `autocommit` as a parameter](dbt-labs/dbt-redshift#475 Autocommit is a new flag that is defaulted to True. This PR adds explanation of this change and rationale behind this decision in dbt docs #3401 3. **[Connect_timeout default to None](dbt-labs/dbt-redshift#433 Connect_timeout had the default of 10 seconds previously. We changed this default to None. #3460 <!--- Describe your changes and why you're making them. If linked to an open issue or a pull request on dbt Core, then link to them here! To learn more about the writing conventions used in the dbt Labs docs, see the [Content style guide](https://github.com/dbt-labs/docs.getdbt.com/blob/current/contributing/content-style-guide.md). --> ## Checklist <!-- Uncomment if you're publishing docs for a prerelease version of dbt (delete if not applicable): - [ ] Add versioning components, as described in [Versioning Docs](https://github.com/dbt-labs/docs.getdbt.com/blob/current/contributing/single-sourcing-content.md#versioning-entire-pages) - [ ] Add a note to the prerelease version [Migration Guide](https://github.com/dbt-labs/docs.getdbt.com/tree/current/website/docs/guides/migration/versions) --> - [x] Review the [Content style guide](https://github.com/dbt-labs/docs.getdbt.com/blob/current/contributing/content-style-guide.md) and [About versioning](https://github.com/dbt-labs/docs.getdbt.com/blob/current/contributing/single-sourcing-content.md#adding-a-new-version) so my content adheres to these guidelines. - [ ] Add a checklist item for anything that needs to happen before this PR is merged, such as "needs technical review" or "change base branch."
…labs#475) * Rename flag and get autocommit on * Add changie * Add test for issue 451 * Add test for issue dbt-labs#454 * Fix spelling. * Revert names * update tests * update the comment to use pep for rationale --------- Co-authored-by: Mila Page <versusfacit@users.noreply.github.com>
resolves dbt-labs/dbt-core#451
resolves dbt-labs/dbt-core#454
reverses dbt-labs/dbt-core#458
resolves dbt-labs/dbt-core#425 (and others I'm sure)
Description
Catch-all for all transaction related chicanery.
Certain DDL cannot exist outside a transaction block without being a NO-OP, and when it is in one, it needs
autocommit=True
.The new workflow
This has the net effect of "restoring" the pre-1.5 workflow. Users won't have to do a thing. This will solve a whole bunch of tangentially tickets. If people want true transactions, we can advise them to say so in their profiles:
autocommit=False
After merge
I'll update the relevant docs issue to be about guidance for turning this off. I need to fix the docs issue and we will need to make sure there is a single source of truth out there for these changes.Will require a 1.5 backport.
Checklist
changie new
to create a changelog entry