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.

Sign up for GitHub

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

Fix for pre-hooks outside of transactions #623

Merged
merged 2 commits into from
Dec 18, 2017
Merged

Fix for pre-hooks outside of transactions #623

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
e984caf
Fix for pre-hooks outside of transactions https://github.com/fishtown…
drewbanin Dec 14, 2017
1139955
improve tests
drewbanin Dec 14, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions dbt/include/global_project/macros/materializations/helpers.sql
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,10 @@
{% macro run_hooks(hooks, inside_transaction=True) %}
{% for hook in hooks | selectattr('transaction', 'equalto', inside_transaction) %}
{% if not inside_transaction and loop.first %}
{% call statement(auto_begin=inside_transaction) %}
commit;
{% endcall %}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the more time passes, the more i don't like our implementation of begin/commit in adapters. it's jarring to see an explicit commit query issued here, i'd expect to see adapter.commit(). but, we can't do that because commit raises an exception if no transaction is opened, etc. etc. etc.

this is a reasonable solution given the constraints that we have now, but rebooting our implementation of these concepts in the adapter could fix a lot of downstream issues. naively:

  • begin() and commit() should mimic the behavior of issuing those queries on the underlying database (i.e. should not raise exceptions if a transaction is / is not open)
  • we should implement additional adapter functions to interrogate the connection to find out if we are in a transaction or not. for example, in postgres, we can issue select txid_current(); twice and if the value is the same, we're in a transaction.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Totally agree @cmcarthur -- I do like being explicit about where the begin/commit occurs, but I think our current approach of sprinkling these statements in to fix bugs is not a good or sustainable one. Let's have a think about this over here #629

{% endif %}
{% call statement(auto_begin=inside_transaction) %}
{{ hook.get('sql') }}
{% endcall %}
Expand Down
17 changes: 15 additions & 2 deletions test/integration/014_hook_tests/test_model_hooks.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -95,8 +95,21 @@ def project_config(self):
'macro-paths': ['test/integration/014_hook_tests/macros'],
'models': {
'test': {
'pre-hook': MODEL_PRE_HOOK,
'post-hook': MODEL_POST_HOOK,
'pre-hook': [
# inside transaction (runs second)
MODEL_PRE_HOOK,

# outside transaction (runs first)
{"sql": "vacuum {{ this.schema }}.on_model_hook", "transaction": False},
],

'post-hook':[
# outside transaction (runs second)
{"sql": "vacuum {{ this.schema }}.on_model_hook", "transaction": False},

# inside transaction (runs first)
MODEL_POST_HOOK,
]
}
}
}
Expand Down