Implements dbt_sqlserver_use_dbt_transactions behavior flag#710
Conversation
Benjamin-Knight
left a comment
There was a problem hiding this comment.
The DML work I did has an explicit transaction in it, this probably should only be emitted when this flag is false because when DBT is handling the transaction all it does it create a nested transaction that doesn't add anything to the process.
I can't see the XACT_ABORT work that @joshmarkovic identified as being an issue. If we want DBT to handle transaction should be have this flag emit the XACT_ABORT setting as well?
I think we need a test to cover snapshots as they call adapter.commit in their code path.
|
I remember someone stating that there are parts of DBT which we may want to not run inside the transaction which is now DBT managed, mostly post hooks, are we planning on shipping this without that resolved? Right now this work would break online and resumable post hook indices that are declared. |
…dependency) Unstacks the indexes feature from PR dbt-msft#710: the branch now builds on current master alone. The transaction-coexistence work is flag-agnostic, so it needs no dependency on dbt_sqlserver_use_dbt_transactions: - Index reconciliation runs in an @@TRANCOUNT-aware batch (owns its own transaction under autocommit, joins an open one if dbt manages the model transaction). - ONLINE/RESUMABLE indexes build in the materialization's post-commit phase via sqlserver__create_indexes_post_commit (auto_begin=False + @@TRANCOUNT guard), so they never run inside a transaction. Seeds documented as a known limitation. - index_needs_own_batch: single source of truth for the online/resumable split. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
@Benjamin-Knight noted the other comments to check. About the parts of dbt, because post hooks that should be resumable, should be stated as not in transaction?, this feature was fixed in #709, and probably nobody is using it, we were doing no transaction practices but because it was not posible, this feature will probably remain optional for a long time (probably only defaulted in v2.1.0?) and recommendations of better future practices should be documented. |
…saction support Adds a behavior flag that makes dbt transaction hooks (begin/commit/rollback) emit real T-SQL statements instead of being no-ops. The flag is stored at both class level (for dbt-core's classmethod-based _rollback_handle) and instance level (for add_begin_query/add_commit_query). - add_begin_query emits BEGIN TRANSACTION when flag is true - add_commit_query emits IF @@TRANCOUNT > 0 COMMIT TRANSACTION when true - _rollback_handle uses T-SQL ROLLBACK with cursor when true; both paths fire RollbackFailed on exception; cursor.close() is try/finally protected - hooks.sql reads flag via adapter.behavior and commits conditionally - table_dml_refresh.sql comment updated for nested transaction semantics - Unit tests: begin/commit/rollback behavior under both flag states - Functional tests: table/view/incremental materializations, rollback with and without flag, after-commit hooks, failed-then-successful run
734ba60 to
5e9b033
Compare
…g is on; add snapshot functional test
|
@Benjamin-Knight I agree XACT_ABORT is related, but I would treat it as a follow-up. This PR makes dbt transaction boundaries real; XACT_ABORT changes how SQL Server reacts to runtime errors inside those boundaries. If we add it, it should be explicit, tested, and probably scoped/restored carefully, lest better isolate that change. Fixed other comments and did a rework to formalize final state and commets. Thanks. |
Adds an opt-in flag that makes dbt's transaction hooks real at the SQL Server level, replacing the current no-op behavior, couldn't find the exact reason this was removed and this stopsome issues we currently have with left over temp tables, dirty reads of empty tables, non-standard behaviors.
What it does
When
dbt_sqlserver_use_dbt_transactions: trueis set indbt_project.yml:add_begin_query()emitsBEGIN TRANSACTIONinstead of being a no-opadd_commit_query()emitsIF @@TRANCOUNT > 0 COMMIT TRANSACTIONinstead of being a no-op_rollback_handle()emitsIF @@TRANCOUNT > 0 ROLLBACK TRANSACTIONwith error handling matching the base class pattern (firesRollbackFailedevent on failure)When
false(default), behavior is unchanged —begin/commitremain no-ops and each statement is auto-committed by the ODBC driver.Why opt-in
Enabling real transactions changes the atomicity guarantees and maps to default dbt standard behavior. Without the flag, each SQL statement commits individually — a failure mid-materialization doesn't roll back earlier DDL/DML. With the flag, the entire model run is wrapped in a single T-SQL transaction, and failures roll back everything.
This includes the DML refresh materialization (
table_dml_refresh.sql), which has its own explicitBEGIN TRANSACTIONfor the atomic DELETE+INSERT swap. With the flag on, this nests inside dbt's outer transaction. SQL Server handles this correctly — only the outermost COMMIT actually commits, and a rollback always rolls back all nesting levels — so scratch tables from a partially-executed materialization are cleaned up on failure.Other changes
table_dml_refresh.sqlto reflect the flagadd_begin_query/add_commit_queryflag gatingCloses #708