Implements dbt_sqlserver_use_dbt_transactions behavior flag#710
Implements dbt_sqlserver_use_dbt_transactions behavior flag#710axellpadilla wants to merge 1 commit into
dbt_sqlserver_use_dbt_transactions behavior flag#710Conversation
dbt_sqlserver_use_dbt_transactions flag
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.
There was a problem hiding this comment.
hooks.sql reads the flag from a different source (flags) than the connection manager (adapter.behavior), and the two agree only while the default is False
|
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. |
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