backport(1.11): transaction-mode fixes — resumable indexes, full-refresh marker, concurrent-incremental deadlock, catalog nolock - #791
Open
axellpadilla wants to merge 5 commits into
Conversation
…the full-refresh marker
dbt_sqlserver_use_dbt_transactions now defaults to True (1.12), so a
model's whole build - from its first statement through its own trailing
adapter.commit() - runs inside one continuous ambient transaction. Two
places still assumed the old autocommit-per-statement behavior:
- sqlserver__create_indexes (the create-path index macro) never split
ONLINE/RESUMABLE builds out of the transaction at all, so a fresh table
with build_options: {resumable: true} hit SQL Server error 574
(RESUMABLE cannot run inside a user transaction). reconcile_indexes had
a creates_no_txn split, but run_query's auto_begin=false only skips
starting a *new* transaction - it doesn't escape one already open.
- The dbt_full_refresh_incomplete crash marker shared the same ambient
transaction as the rebuild it's meant to survive, so a real failure
mid-rebuild rolled the marker back right along with everything else,
defeating its purpose.
Add adapter.commit_if_open()/begin_if_closed() primitives and use them
to flush the ambient transaction around both: a shared
sqlserver__create_indexes_no_txn helper for the index case, and a split
of sqlserver__create_table_as_prebuilt into two real statements (setup
+ marker, then load) for the marker case.
Verified against live SQL Server (pyodbc and mssql-python backends):
removed the class-level dbt_sqlserver_use_dbt_transactions: False
opt-outs from test_index_config.py/test_full_refresh_build.py, which
were an acknowledged stop-gap, and confirmed all cases pass with
transactions on their real default.
(cherry picked from commit 59a6cf6)
Two concurrent incremental models at threads=2 with
dbt_sqlserver_use_dbt_transactions on deadlocked (error 1205) on an X/S
inversion over sys.sysschobjs (index nc1) - user-DB system catalog rows,
not user data. The temp-relation build (CREATE OR ALTER VIEW __dbt_tmp_vw
-> SELECT * INTO __dbt_tmp -> DROP VIEW) ran inside the materialization's
ambient transaction, so its catalog X keylocks were held until commit
while a second worker's name resolution asked for S on them.
Nothing in the temp build needs that transaction: the relation is
throwaway. The transaction was opened accidentally, by a read-only
catalog lookup - find_references in sqlserver__get_drop_sql - which used
statement()'s default auto_begin=True. dbt-adapters' own
relations/drop.sql already wraps the surrounding drop in auto_begin=False;
this override just missed it.
- relation.sql: find_references runs with auto_begin=false, so the temp
build autocommits statement by statement and releases catalog locks as
it goes.
- incremental.sql: the fresh-create / full-refresh statement('main')
batch, which is also create_table_as catalog DDL, runs with
auto_begin=False; commit_if_open/begin_if_closed after it reopen the
ambient transaction for the swap and the tail. The incremental branch
keeps the default auto_begin so its strategy DML stays atomic through
adapter.commit().
- create.sql: OBJECT_ID drop guard on the SELECT * INTO temp path, scoped
to temp/intermediate relations, so a crash-left __dbt_tmp is recovered
next run instead of raising Msg 2714. Fresh creates of a real target
still surface 2714.
- hooks.sql: the COMMIT emitted before the first transaction=false hook is
now guarded with @@TRANCOUNT > 0. It was only ever safe because
something had accidentally auto-begun a transaction; with that removed
it raised Msg 3902.
Pre-hook rollback is preserved: nothing is committed early, we only
decline to open a transaction for a read. A transaction=true pre-hook
still opens one, and the temp build correctly joins it.
Verified on the catalog-scanning repro harness (mssql-python 1.10.0,
threads=2): 9 deadlocks/12 runs before, 0/40 after. Regression gate green
(test_transactions, test_incremental, test_temp_relation_cleanup,
test_xact_abort, test_concurrency, test_concurrent_incremental).
Refs: docs/plan/20260801-dbt-sqlserver-concurrent-incremental-deadlock
(cherry picked from commit 7fe0f06)
SQL Server takes transaction-scoped X locks on the system catalog for every
CREATE/ALTER/DROP. A catalog read from any other session under the default
READ COMMITTED has to wait for those to be released, and the default
LOCK_TIMEOUT is -1, so it waits forever. An un-hinted catalog query in the
adapter is therefore a hard dependency on every other writer in the database -
a second dbt project, an ETL tool, a DBA mid-deployment - and dbt just appears
to hang with no error and no visible failure.
information_schema_hints() (with (nolock)) is the existing answer to this and
is already applied at ~60 sites, including list_relations_without_caching.
These were missed:
metadata.sql get_relation_last_modified sys.objects / sys.schemas
indexes.sql drop_fk_constraints sys.foreign_keys / sys.tables
indexes.sql drop_pk_constraints sys.indexes / sys.tables
indexes.sql drop_fk_references sys.foreign_key_columns + joins
indexes.sql list_nonclustered_rowstore_indexes (both branches)
Measured against a second session holding 40 uncommitted CREATE statements in
the same schema, with LOCK_TIMEOUT 5000 on the reader so blocking is
observable rather than indefinite:
get_relation_last_modified no hint: blocked 5.00s with (nolock): 0.00s
drop_fk_references no hint: blocked 5.02s with (nolock): 0.01s
drop_pk_constraints no hint: blocked 5.01s with (nolock): 0.01s
list_relations (hinted today) no hint: blocked 5.00s with (nolock): 0.00s
The last row is the control: it confirms the existing hints are load-bearing,
not decorative.
with (nolock) is preferred over SET TRANSACTION ISOLATION LEVEL READ
UNCOMMITTED because it is scoped per table reference. It cannot leak a dirty
read onto user data the way a session-level isolation change would.
Deliberately NOT hinted: catalog reads that act as correctness guards rather
than discovery - the schema-exists check before CREATE SCHEMA (schema.sql),
the principal-exists check in apply_grants.sql, and the sys.extended_properties
full-refresh marker (relations/table/create.sql). A dirty read there would
weaken the guard, not just widen discovery. None of them blocked in the
measurements above.
Regression test: tests/functional/adapter/mssql/test_catalog_nolock.py. It
opens a second dbt connection on its own thread, holds 40 uncommitted CREATE
TABLE statements in the test schema, and calls get_relation_last_modified with
LOCK_TIMEOUT 5000. Without the hint it fails with Msg 1222 "Lock request time
out period exceeded"; with it, it returns in milliseconds. Verified to fail
when only the metadata.sql hunk is reverted.
Backport notes (cherry-pick checked against v1.10.1 and v1.9.2):
- metadata.sql and the new test apply cleanly to both. The macro body is
byte-identical there apart from apply_label() vs get_query_options().
- indexes.sql conflicts on two hunks (drop_fk_constraints,
drop_pk_constraints) because master has since hardened those SELECTs with
QUOTENAME and added a schema filter. The conflict is adjacent, unrelated
content: resolve by keeping the older branch's select line and taking only
the {{ information_schema_hints() }} tokens on the from/join lines. The
other three indexes.sql hunks apply cleanly.
- information_schema_hints() itself has existed since 1.4, so no new
machinery is needed on any target branch.
(cherry picked from commit 1596c98)
Split out from the previous commit so that one stays as backportable as possible: this hunk's context lines sit next to the auto_begin=false change from the deadlock fix, and CHANGELOG.md conflicts on any backport by construction. find_references is the same class of query as the ones hinted in the previous commit - read-only discovery over sys.objects / sys.schemas, whose result only drives DROP VIEW IF EXISTS statements, so a dirty read costs nothing. Unlike those, it did NOT block in measurement: its plan seeks sys.sql_expression_dependencies first and reaches sys.objects by object_id on the clustered index, missing the contended name-index range. That is a property of one plan on one dataset, not a guarantee, so hint it for consistency across the discovery class rather than leaving it as the odd one out. CHANGELOG: the catalog hints are filed under v1.11.0, not v1.12.0. v1.11.0 final has not been released (only v1.11.0rc1 is published; the v1.11.0 tag has no release behind it), the hints are independent of the transaction work, and both hint commits cherry-pick clean onto the v1.11.0 tag - verified. The deadlock fix stays under v1.12.0: it is only reachable with dbt_sqlserver_use_dbt_transactions on, which defaults to False in 1.11, and it depends on commit_if_open/begin_if_closed, which 1.11 does not have. (cherry picked from commit d0e76c5)
Mechanical fixups the cherry-picks cannot get right on a 1.11 target: - The three fix commits were authored when the top CHANGELOG section was "### Unreleased" (renamed to "### v1.12.0" by the 1.12.0rc1 prep). Applying them here recreated that section carrying four 1.12-only bullets - dbt-core 1.12 support, Python 3.14, and the two behavior-change defaults. Dropped the section and filed its two bugfix bullets under v1.11.0 instead. - Reworded "dbt_sqlserver_use_dbt_transactions: True (now the default)" to "(opt-in in 1.11, the default from 1.12)". On this branch the flag still defaults to False; these are fixes for users who turn it on. - Reverted the tests/conftest.py hunk that rode along in 59a6cf6. It switches the default test backend from pyodbc to mssql-python, which has nothing to do with these fixes.
This was referenced Aug 1, 2026
Open
axellpadilla
added a commit
that referenced
this pull request
Aug 1, 2026
Both workflows filtered on `branches: [master, v*]`. GitHub matches the whole ref, so `v*` matches a branch literally named `v1.11` but not `release/v1.11` — the naming this repo actually uses. Pull requests targeting a release branch therefore ran pre-commit only: no unit tests and no integration matrix. That left backports as the least-tested changes in the repo, despite shipping first. #791 and #794 currently have no test coverage at all. Adding `release/**` to the push and pull_request filters of both workflows. Stacked pull requests (base is a feature branch rather than master or a release branch) are still uncovered by design; those are validated once rebased, or by dispatching the workflow against the branch.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Backports the three transaction-related fixes to the 1.11 line, ahead of the v1.11.0 final release (only
v1.11.0rc1is published today, so these land inv1.11.0itself — nov1.11.1section needed).dbt_sqlserver_use_dbt_transactionsdefaults toFalseon 1.11, but it is a documented opt-in flag, and all three bugs are live for anyone who turns it on. Flipping the default in 1.12 did not create them — it exposed them. Shipping 1.11.0 with a flag that hits error 574 on resumable index builds, discards the crash marker on a failed rebuild, and deadlocks atthreads> 1 is worse than shipping it fixed, and it makes 1.11 → 1.12 a pure default flip rather than a default flip plus three latent-bug fixes.Commits
All cherry-picked with
-x, so each records its upstream SHA.59a6cf6dbt_full_refresh_incompletecrash marker under dbt-managed transactions. Addscommit_if_open/begin_if_closed/index_needs_own_batch.7fe0f06sys.sysschobjsatthreads> 1.1596c98with (nolock)on the read-only catalog lookups that were missing it, plus a regression test.d0e76c5find_referenceshint.Every one applies to
release/v1.11with no conflicts.Manual fixups in the last commit
Three things a clean cherry-pick cannot get right on a 1.11 target:
### Unreleased(renamed to### v1.12.0by the 1.12.0rc1 prep), so applying them here recreated that section carrying four 1.12-only bullets — dbt-core 1.12 support, Python 3.14, and both behavior-change defaults. Dropped the section; filed its two bugfix bullets under### v1.11.0.Falseon this branch.tests/conftest.pyhunk that rode along in59a6cf6; it switches the default test backend from pyodbc to mssql-python, unrelated to these fixes.Risk
51 new adapter lines and a materialization change into an already-tagged tree. Mitigated by all of it being inert when the flag is off, which is 1.11's default:
commit_if_open/begin_if_closedemit no T-SQL then.Companion PR: #792