diff --git a/CHANGELOG.md b/CHANGELOG.md index 985e73b2..b772a996 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -19,9 +19,17 @@ - Fix a `view` model silently skipping a rebuild when text was removed from the *start* of its body (e.g. deleting a leading comment or CTE). The skip test compared the stored definition against the model with `endswith()`, so any edit whose new body was a tail of the old one looked unchanged: `dbt run` reported `PASS` but the change never reached the database, and `--full-refresh` did not fix it. The header (`CREATE [OR ALTER] VIEW AS`) is now split off at its separating ` AS ` and the body compared exactly. The comparison also no longer lowercases or strips whitespace, both of which made genuinely different bodies (a string literal differing only in case, or any literal containing spaces) compare equal; where the definition cannot be parsed with certainty the view is rebuilt rather than skipped. - Fix snapshots failing on their second and later runs with `Invalid object name '..._dbt_tmp'`, and contract-enforced models silently losing their in-transaction `pre_hook` writes. `get_column_schema_from_query` reads a query's column shape by executing it, then returned without fetching the rows or closing the cursor. Closing a cursor whose result set the server is still producing makes the driver cancel the request, and SQL Server answers that cancel by rolling back the open transaction, since every connection runs `SET XACT_ABORT ON` (#718). Nothing is raised for any of it, so the snapshot lost the staging table it had just built and failed against it a statement later. The probe now drains and closes its cursor, as does the row-count probe in `expand_column_types`. Only queries opening with a CTE were affected - anything else is wrapped as `select * from (...) where 1 = 0` by `sqlserver__get_empty_subquery_sql` and returns no rows - which is why snapshot staging queries (`with snapshot_query as ...`, both `check` and `timestamp` strategies) and CTE-headed contract models were the ones that broke. - Fix models failing with `Incorrect syntax near '\'` when the schema name needs delimiters, such as a domain-qualified `domain\user`. The clustered columnstore index name embeds the schema and was emitted as a bare identifier, so the generated DDL did not parse. [#409](https://github.com/dbt-msft/dbt-sqlserver/issues/409) +- Fix `table_refresh_method: dml` holding a `Sch-M` lock on its scratch table for the whole load, blocking metadata readers in every other session on the database for as long as the model took to build. Two independent causes: the scratch table was built by one fused `SELECT * INTO`, which holds `Sch-M` on the new object from the moment the statement starts until it finishes rather than for the instant of creation; and that build ran inside the materialization's ambient transaction, which held the lock through to the trailing `COMMIT` regardless — so fixing either alone would have changed nothing. `Sch-M` is the one lock mode incompatible with the `Sch-S` lock every metadata reader takes, so any session reading that object's metadata blocked — including database-wide `sys` / `INFORMATION_SCHEMA` scans, a concurrent dbt run's catalog and column lookups, and SSMS's object explorer, none of which asked for the scratch table by name. The scratch table is now created empty *before the in-transaction pre-hooks run* and loaded afterwards by a separate `INSERT ... WITH (TABLOCK)` (still minimally logged, as `SELECT INTO` was), so the create autocommits and releases its lock in an instant while the load — which takes an exclusive table lock, compatible with `Sch-S` — joins a `transaction: true` pre-hook's transaction and keeps rolling back with it. The `DELETE`+`INSERT` swap now commits together with the in-transaction post-hooks, before index reconciliation, masks, grants and `persist_docs`, so the target's exclusive locks no longer span those; the swap itself remains atomic, and index/mask reconciliation reconverges on the next run if it fails. [#819](https://github.com/dbt-msft/dbt-sqlserver/issues/819) +- Apply the same split to the default `table` build and every other `create_table_as` caller (incremental full refreshes, first builds and temp builds, snapshots): the table is created empty and loaded by a separate `INSERT ... WITH (TABLOCK)` rather than by a fused `SELECT * INTO`. On the `table`, `incremental` and `snapshot` materializations the create is staged ahead of the in-transaction pre-hooks and autocommits, so neither the new table's `Sch-M` nor the tmp view's is held through the load — the view is now dropped after the cutover commits, since an uncommitted `DROP VIEW` blocks catalog scans just as an uncommitted `CREATE` does. Because the create commits standalone, a crashed run can leave a `__dbt_tmp` intermediate behind; the existing `OBJECT_ID` guard for adapter-generated throwaways already drops it on the next run rather than failing with `Msg 2714`. [#819](https://github.com/dbt-msft/dbt-sqlserver/issues/819) +- Stop the adapter's read-only catalog probes opening a dbt-managed transaction. A `call statement(...)` block defaults to `auto_begin=True`, so a probe issued when none is running opens one, even though it only reads. This is not observable in a materialization today — every build path deliberately holds a transaction open across its tail — but it makes the probes a latent hazard for any caller that runs one outside a transaction, and it blocks moving the mask and index reconciliation out of the cutover transaction, since a single probe would silently pull all of the DDL after it back inside (the `Sch-M` window on the live target that [#819](https://github.com/dbt-msft/dbt-sqlserver/issues/819) is about). `get_columns_in_relation`, `get_mask_index_key_columns`, `get_unmaskable_columns`, `get_existing_principals`, `describe_indexes`, and the two macros that are currently unreferenced (`find_references` in `indexes.sql`, `list_nonclustered_rowstore_indexes`) now all pass `auto_begin=False`, as `find_references` in `relation.sql` already did; each still joins a transaction that is already open, so callers that legitimately run inside one are unaffected. A unit test enforces the rule so a new probe cannot reintroduce it. +- Fix a failed *first* build of an `incremental` model leaving an empty table behind under the model's real name, which then silently lost data. The fresh-create branch built straight into the target rather than into a `__dbt_tmp` intermediate, so it had neither a rename swap nor the `OBJECT_ID` drop guard (that guard only covers adapter-generated throwaways). Since the build was split into an empty `CREATE` plus a separate `INSERT ... WITH (TABLOCK)` and declines to open the ambient transaction, the two statements commit independently — so a load that failed left the empty `CREATE` committed. dbt's next run then saw a relation that existed and was not a view, took the append/merge branch, and merged that run's window into an empty table: no error was raised, and every row the first build should have loaded was gone. Fresh creates now build into the intermediate and swap, as full refreshes already did, so a failed load leaves no target and the next run does a fresh create. Note the swap means a first build's clustered columnstore index is now named from the intermediate (`___dbt_tmp_cci`), matching what a `--full-refresh` has always produced. [#819](https://github.com/dbt-msft/dbt-sqlserver/issues/819) +- Split `sqlserver__create_table_as` into `sqlserver__get_create_table_stage_sql` (the `USE`, the temp view, and the empty `CREATE`) and `sqlserver__get_create_table_load_sql` (the `INSERT ... WITH (TABLOCK)`, optionally the view drop, and the clustered columnstore index), so a caller can put a transaction boundary between creating a table and loading it — locks are held to commit rather than to end-of-statement, so an empty `CREATE` sharing a transaction with its load holds the new object's `Sch-M` for the whole load. `sqlserver__create_table_as` is now exactly the two halves back to back and remains the entry point for callers that want one batch — the `dml` schema-change rebuild, and `pre_hook_transaction_scope: build` — so the statements they run are unchanged in content and order. The one visible difference is batching: the create and the load previously shared a single `EXEC` literal and now have one each, which shows up in compiled SQL artifacts. [#819](https://github.com/dbt-msft/dbt-sqlserver/issues/819) +- Move the transaction boundary so a model build no longer holds a `Sch-M` lock across its slow work, completing the [#819](https://github.com/dbt-msft/dbt-sqlserver/issues/819) fix on `table`, `incremental` and `snapshot` alike. The transaction now covers the in-transaction pre-hooks, the load, the cutover, fresh-table masks and the in-transaction post-hooks — the unit a hook declaring `transaction: true` asks to be atomic with — and index creation or reconciliation, grants, denies and `persist_docs` run after it commits, so `sp_rename`'s `Sch-M` on the live table no longer spans the index builds. `full_refresh_build: prebuilt` also no longer reopens the ambient transaction before its load: the clustered design is created on the empty table and the `INSERT` runs autocommitted, where previously the `Sch-M` taken on the live name was held through the load, masks and post-hooks. **Two behaviour changes to be aware of.** In-transaction post-hooks now run *before* index creation on all three materializations (they already ran before grants and `persist_docs`); a post-hook that needs indexes present should declare `transaction: false`, which runs it after the whole tail. And if you create indexes from post-hooks — the idiom that predates the `indexes` config — `drop_unmanaged_indexes: true` will now drop them in the same run on the persisted-table paths, and such an index on a masked column trips the index-key check on SQL Server before 2022. Data masks deliberately stay *inside* the cutover transaction on paths that build a new table, so a mask failure rolls the swap back and the old masked table keeps serving rather than leaving the new one live and exposed. A snapshot's first build now goes through the `__dbt_tmp` intermediate and is renamed into place, as a `table` build does, so a failed first load leaves no empty snapshot table behind for the next run to merge into; its clustered columnstore index is therefore named from the intermediate (`___dbt_tmp_cci`). See [docs/transaction_scope.md](docs/transaction_scope.md). +- Add the `pre_hook_transaction_scope` model config (`load` | `build`). Schema resolution — the tmp view and the empty `CREATE` — needs the model SQL to bind, so under the default `load` it runs *before* the in-transaction pre-hooks; a `transaction: true` pre-hook that creates an object the model reads therefore fails at that step with `Invalid object name`. On a `snapshot` the same ordering applies to the `check` strategy's own column-shape query, which runs the snapshot's SQL on every run after the first. Declare that hook `transaction: false` (those run before the stage, and on a snapshot before the strategy resolves) or set `build`, which stages inside the hook's transaction as before — at the cost of holding the new table's `Sch-M` for the whole load. `build` never begins a transaction of its own: with no `transaction: true` pre-hook there is nothing to join, so the create and the load autocommit exactly as under `load`, and a folder-level `+pre_hook_transaction_scope: build` costs nothing on the models underneath it that have no hooks. Both scopes keep a `transaction: true` pre-hook atomic with the load. The config is inert on `full_refresh_build: prebuilt`, whose setup must follow the hooks and which commits them with its in-progress marker regardless; the incremental `--full-refresh` of an existing table likewise commits them with its marker, so neither path can roll a pre-hook back whatever the scope. +- Make the incremental materialization's trailing `adapter.commit()` explicit about its precondition. `adapter.commit()` raises when no transaction is open, and every branch above it only happened to leave one open — the swap's renames, the `prebuilt` path's trailing load statement, or the append path's `statement('main')`. That is balance by coincidence, and a branch ending on a statement that declines the ambient transaction would break it; `begin_if_closed()` now states the requirement instead, as the `table` materialization already did. - Fix identifiers built inside string literals not being quoted, which broke schema names containing a `.` or a `"`. `OBJECT_ID('schema.table')` returns `NULL` rather than erroring for such a name, so the failures were silent: the drop-before-create guards in `create_table_as` treated an existing table as absent (then hit `Msg 2714`), and the mask introspection in `apply_masks` found no columns, so configured masks were never applied. `sp_rename` was affected too, failing the table rename-swap with `No item by the name of ...`. All now pass quoted, qualified names. [#785](https://github.com/dbt-msft/dbt-sqlserver/issues/785) - Fix the `sqlserver__openquery` macro to quote linked-server names through `adapter.quote()`, keeping its generated identifier style consistent with the rest of the adapter. -- Fix a model with `table_refresh_method: dml` losing its clustered columnstore index - and, under an enforced contract, its `NOT NULL`s as well - the first time its schema changed. That path builds a scratch table and, when the columns no longer match, renames it into position; the scratch was built with `SELECT * INTO`, which copies no index and no constraint and infers nullability from the query rather than from the contract, and `create_indexes` only builds what the `indexes` config names, never the `as_columnstore` CCI. So the model came back stripped and stayed that way, since every later run matched the new schema and took the DELETE+INSERT path. That branch now rebuilds the scratch table through `create_table_as` - the way every other build path in the adapter creates a table - which carries the full column DDL and the columnstore index across the swap. On a schema-change run - and only there - that costs a second execution of the model's SQL (once for the `SELECT ... INTO` schema probe, once for the rebuild) and one extra columnstore build. The rebuild bulk-loads the same way the model would on any other build path - `SELECT ... INTO` for an ordinary model, `CREATE TABLE` plus `INSERT ... WITH (TABLOCK)` under an enforced contract - so it gives up no minimal logging to do it. Steady-state refreshes are unchanged. A table already stripped by this bug is not repaired by upgrading: its schema still matches, so it stays on the DELETE+INSERT path, and index reconciliation protects an existing columnstore index without ever creating a missing one. To rebuild it, temporarily set `full_refresh_build: prebuilt` and run with `--full-refresh`. +- Fix a model with `table_refresh_method: dml` losing its clustered columnstore index - and, under an enforced contract, its `NOT NULL`s as well - the first time its schema changed. That path builds a scratch table and, when the columns no longer match, renames it into position; the scratch was built with `SELECT * INTO`, which copies no index and no constraint and infers nullability from the query rather than from the contract, and `create_indexes` only builds what the `indexes` config names, never the `as_columnstore` CCI. So the model came back stripped and stayed that way, since every later run matched the new schema and took the DELETE+INSERT path. That branch now rebuilds the scratch table through `create_table_as` - the way every other build path in the adapter creates a table - which carries the full column DDL and the columnstore index across the swap. On a schema-change run - and only there - that costs a second execution of the model's SQL (once for the scratch load that serves as the schema probe, once for the rebuild) and one extra columnstore build. The rebuild bulk-loads the same way the model would on any other build path - an empty create followed by `INSERT ... WITH (TABLOCK)` - so it gives up no minimal logging to do it. Steady-state refreshes are unchanged. A table already stripped by this bug is not repaired by upgrading: its schema still matches, so it stays on the DELETE+INSERT path, and index reconciliation protects an existing columnstore index without ever creating a missing one. To rebuild it, temporarily set `full_refresh_build: prebuilt` and run with `--full-refresh`. #### Under the hood diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index d183aedf..703fe775 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -32,6 +32,19 @@ The functional tests require a running SQL Server instance. You can easily spin make server ``` +If you would rather not run Docker, there is a rootless [podman](https://podman.io/) +equivalent that builds the same image and passes the same environment, so +`test.env` works unchanged: + +```shell +make server-podman # build and start +make server-podman-logs # follow init; ready at "user creation completed" +make server-podman-stop # remove the container +``` + +Override `MSSQL_VERSION` to test against another release, e.g. +`make server-podman MSSQL_VERSION=2019`. + ### Backend requirements at a glance | Backend | Python package | Debian/Ubuntu system packages | diff --git a/Makefile b/Makefile index 2ad05fef..1ccc08ce 100644 --- a/Makefile +++ b/Makefile @@ -1,5 +1,8 @@ .DEFAULT_GOAL:=help THREADS ?= auto +MSSQL_VERSION ?= 2022 +PODMAN_IMAGE ?= dbt-sqlserver-mssql:$(MSSQL_VERSION) +PODMAN_CONTAINER ?= dbt-sqlserver-mssql .PHONY: dev dev: ## Installs adapter in develop mode along with development dependencies @@ -57,6 +60,33 @@ server: ## Spins up a local MS SQL Server instance for development. Docker-compo @\ docker compose up -d +# Podman equivalents of `server`, for anyone who would rather not run Docker +# Desktop. They build the same devops/server.Dockerfile and pass the same +# environment docker-compose.yml does, so test.env works unchanged. +.PHONY: server-podman +server-podman: ## Spins up the same SQL Server instance under rootless podman. + @\ + podman build -t $(PODMAN_IMAGE) --build-arg MSSQL_VERSION=$(MSSQL_VERSION) \ + -f devops/server.Dockerfile devops && \ + podman rm -f $(PODMAN_CONTAINER) >/dev/null 2>&1 || true; \ + podman run -d --name $(PODMAN_CONTAINER) \ + -e ACCEPT_EULA=Y \ + -e SA_PASSWORD='L0calTesting!' \ + -e COLLATION='SQL_Latin1_General_CP1_CS_AS' \ + --env-file test.env \ + -p 1433:1433 \ + $(PODMAN_IMAGE) + +.PHONY: server-podman-stop +server-podman-stop: ## Removes the podman SQL Server instance. + @\ + podman rm -f $(PODMAN_CONTAINER) + +.PHONY: server-podman-logs +server-podman-logs: ## Tails the podman SQL Server logs (init completes on "user creation completed"). + @\ + podman logs -f $(PODMAN_CONTAINER) + .PHONY: clean clean: ## Removes ignored files and build artifacts from the repo. @echo "cleaning repo" diff --git a/README.md b/README.md index c98963b4..2fb024dd 100644 --- a/README.md +++ b/README.md @@ -177,6 +177,21 @@ The same setting is also honoured via `vars:` for backwards compatibility; the b Safe expansions are further gated by `column_type_expansion_max_rows` (default 1,000,000 rows) to avoid long-running operations on large tables. +### `pre_hook_transaction_scope` + +_(default: `load`)_ Where a `table`, `incremental` or `snapshot` build resolves its schema +(the tmp view and the empty `CREATE`) relative to its in-transaction pre-hooks. +`load` stages it before them, so the new table's `Sch-M` lock is released in an +instant and the load blocks no metadata reader in other sessions; a +`transaction: true` pre-hook still rolls back with a failed load, except on the +two paths that commit a full-refresh marker before the load +(`full_refresh_build: prebuilt`, and an incremental `--full-refresh` of an +existing table), where neither scope can roll it back. `build` stages +it inside the hook's transaction, for the one case `load` cannot serve: a +`transaction: true` pre-hook that creates an object the model reads. See +[docs/transaction_scope.md](docs/transaction_scope.md) for the full flow and +the post-hook ordering change. + ### `dbt_sqlserver_use_dbt_transactions` _(default: `true`)_ Makes dbt's transaction hooks real at the SQL Server level by emitting `BEGIN TRANSACTION` / `COMMIT TRANSACTION` through the adapter's `add_begin_query` and `add_commit_query` methods. diff --git a/dbt/include/sqlserver/macros/adapters/apply_denies.sql b/dbt/include/sqlserver/macros/adapters/apply_denies.sql index 7843eea2..56834bac 100644 --- a/dbt/include/sqlserver/macros/adapters/apply_denies.sql +++ b/dbt/include/sqlserver/macros/adapters/apply_denies.sql @@ -52,7 +52,9 @@ {#- Lower-cased names of every database principal, for the existence guard. -#} {% macro sqlserver__get_existing_principals() %} - {% call statement('get_existing_principals', fetch_result=True) %} + {#- Read-only probe: auto_begin=False so it cannot open the ambient + transaction - see sqlserver__get_columns_in_relation (#819). -#} + {% call statement('get_existing_principals', fetch_result=True, auto_begin=False) %} select name from sys.database_principals {{ information_schema_hints() }} {% endcall %} {% set result = [] %} diff --git a/dbt/include/sqlserver/macros/adapters/apply_masks.sql b/dbt/include/sqlserver/macros/adapters/apply_masks.sql index c534344e..ebefa74b 100644 --- a/dbt/include/sqlserver/macros/adapters/apply_masks.sql +++ b/dbt/include/sqlserver/macros/adapters/apply_masks.sql @@ -42,7 +42,13 @@ columnstore index (it reports every column as included, never as a key), so a normal columnstore table has no index-key columns and masks apply freely. -#} {% macro sqlserver__get_mask_index_key_columns(relation) %} - {% call statement('get_mask_index_key_columns', fetch_result=True) %} + {#- Read-only probe: auto_begin=False so it cannot OPEN the ambient + transaction. It still joins one that is already open, so callers + that legitimately run inside a transaction are unaffected; what it + stops is a probe in the post-cutover tail reopening a transaction + that the following mask/index DDL then joins and holds to commit + (dbt-msft/dbt-sqlserver#819). -#} + {% call statement('get_mask_index_key_columns', fetch_result=True, auto_begin=False) %} select distinct col.name as name from sys.index_columns ic {{ information_schema_hints() }} inner join sys.columns col {{ information_schema_hints() }} @@ -61,7 +67,9 @@ {#- Columns that DDM cannot mask at all (a mask ALTER would fail): computed, FILESTREAM, sparse COLUMN_SET, and Always Encrypted columns. -#} {% macro sqlserver__get_unmaskable_columns(relation) %} - {% call statement('get_unmaskable_columns', fetch_result=True) %} + {#- Read-only probe: auto_begin=False so it cannot open the ambient + transaction - see sqlserver__get_columns_in_relation (#819). -#} + {% call statement('get_unmaskable_columns', fetch_result=True, auto_begin=False) %} select col.name as name from sys.columns col {{ information_schema_hints() }} where col.object_id = OBJECT_ID('{{ escape_single_quotes(relation.include(database=False)) }}') diff --git a/dbt/include/sqlserver/macros/adapters/columns.sql b/dbt/include/sqlserver/macros/adapters/columns.sql index d43ad581..eb611cce 100644 --- a/dbt/include/sqlserver/macros/adapters/columns.sql +++ b/dbt/include/sqlserver/macros/adapters/columns.sql @@ -90,7 +90,13 @@ {% macro sqlserver__get_columns_in_relation(relation) -%} {% set query_label = get_query_options() %} - {% call statement('get_columns_in_relation', fetch_result=True) %} + {#- Read-only probe: auto_begin=False so it cannot OPEN the ambient + transaction. It still joins one that is already open, so callers + that legitimately run inside a transaction are unaffected; what it + stops is a probe in the post-cutover tail reopening a transaction + that the following mask/index DDL then joins and holds to commit + (dbt-msft/dbt-sqlserver#819). -#} + {% call statement('get_columns_in_relation', fetch_result=True, auto_begin=False) %} {{ get_use_database_sql(relation.database) }} select c.name collate database_default as column_name, diff --git a/dbt/include/sqlserver/macros/adapters/indexes.sql b/dbt/include/sqlserver/macros/adapters/indexes.sql index 53e14a80..b042aa2c 100644 --- a/dbt/include/sqlserver/macros/adapters/indexes.sql +++ b/dbt/include/sqlserver/macros/adapters/indexes.sql @@ -200,7 +200,9 @@ {% macro drop_fk_indexes_on_table(relation) -%} - {% call statement('find_references', fetch_result=true) %} + {#- Read-only probe: auto_begin=False so it cannot open the ambient + transaction - see sqlserver__get_columns_in_relation (#819). -#} + {% call statement('find_references', fetch_result=true, auto_begin=false) %} {{ get_use_database_sql(relation.database) }} SELECT obj.name AS FK_NAME, sch.name AS [schema_name], @@ -232,7 +234,9 @@ {% endmacro %} {% macro sqlserver__list_nonclustered_rowstore_indexes(relation) -%} - {% call statement('list_nonclustered_rowstore_indexes', fetch_result=True) -%} + {#- Read-only probe: auto_begin=False so it cannot open the ambient + transaction - see sqlserver__get_columns_in_relation (#819). -#} + {% call statement('list_nonclustered_rowstore_indexes', fetch_result=True, auto_begin=False) -%} SELECT i.name AS index_name , i.name + '__dbt_backup' as index_new_name @@ -381,7 +385,13 @@ {% macro sqlserver__describe_indexes(relation) %} - {% call statement('describe_indexes', fetch_result=True) -%} + {#- Read-only probe: auto_begin=False so it cannot OPEN the ambient + transaction. It still joins one that is already open, so callers + that legitimately run inside a transaction are unaffected; what it + stops is a probe in the post-cutover tail reopening a transaction + that the following mask/index DDL then joins and holds to commit + (dbt-msft/dbt-sqlserver#819). -#} + {% call statement('describe_indexes', fetch_result=True, auto_begin=False) -%} select i.[name] as [name], case when i.[type] = 1 then 'clustered' diff --git a/dbt/include/sqlserver/macros/materializations/hooks.sql b/dbt/include/sqlserver/macros/materializations/hooks.sql index 8da27177..f2240946 100644 --- a/dbt/include/sqlserver/macros/materializations/hooks.sql +++ b/dbt/include/sqlserver/macros/materializations/hooks.sql @@ -21,3 +21,38 @@ {% endif %} {% endfor %} {% endmacro %} + + +{% macro sqlserver__pre_hook_transaction_scope() -%} + {#- + Resolve pre_hook_transaction_scope: where schema resolution (the tmp view + and the empty CREATE) sits relative to the in-transaction pre-hooks. + Shared by table, incremental and snapshot. Full detail in + docs/transaction_scope.md. + + load (default) build + ------------------------------------ ------------------------------------ + stage autocommit BEGIN + BEGIN |- in-tx pre-hooks + |- in-tx pre-hooks |- stage Sch-M on new object + |- load X table lock only |- load ... held to COMMIT + |- cutover, masks, in-tx post-hooks |- cutover, masks, in-tx post-hooks + COMMIT COMMIT + |- view drops, indexes, grants, docs |- view drops, indexes, grants, docs + + Both keep a transaction: true pre-hook atomic with the load. load fixes + #819 (Sch-M conflicts with the Sch-S every metadata reader takes; an X + table lock does not) but needs the model SQL to bind before the hooks run: a + transaction: true pre-hook that creates an object the model reads fails + at the stage with Msg 208. Remedies: transaction: false on that hook + (outside-tx hooks run before the stage), or build for that model. + -#} + {%- set scope = config.get('pre_hook_transaction_scope', 'load') -%} + {%- if scope not in ['load', 'build'] -%} + {{ exceptions.raise_compiler_error( + "Invalid pre_hook_transaction_scope '" ~ scope ~ "'. " + "Valid values are: 'load' (default), 'build'." + ) }} + {%- endif -%} + {{ return(scope) }} +{%- endmacro %} diff --git a/dbt/include/sqlserver/macros/materializations/models/incremental/incremental.sql b/dbt/include/sqlserver/macros/materializations/models/incremental/incremental.sql index 174f42f5..e1ce0e9a 100644 --- a/dbt/include/sqlserver/macros/materializations/models/incremental/incremental.sql +++ b/dbt/include/sqlserver/macros/materializations/models/incremental/incremental.sql @@ -12,6 +12,38 @@ {%- set unique_key = config.get('unique_key') -%} {%- set full_refresh_mode = (should_full_refresh() or existing_relation.is_view) -%} {%- set on_schema_change = incremental_validate_on_schema_change(config.get('on_schema_change'), default='ignore') -%} + {%- set full_refresh_build = config.get('full_refresh_build', 'heap_then_index') -%} + + {#- Decide the build branch up front, from state that is fixed before any + hook runs, so schema resolution can be staged ahead of the in-transaction + pre-hooks (see pre_hook_transaction_scope below). + 'prebuilt' - in-place build into the target's clustered design + 'create' - build the intermediate from scratch and rename-swap it in + (first build, --full-refresh, view -> table) + 'append' - the incremental strategy's DML against the target -#} + {%- if existing_relation is none -%} + {%- set branch = 'prebuilt' if full_refresh_build == 'prebuilt' else 'create' -%} + {%- elif full_refresh_mode -%} + {#- explicit --full-refresh only; view->table conversions keep the default path -#} + {%- set branch = 'prebuilt' if (full_refresh_build == 'prebuilt' and should_full_refresh()) else 'create' -%} + {%- else -%} + {%- set branch = 'append' -%} + {%- endif -%} + {#- a table built fresh this run carries no masks or indexes yet -#} + {%- set fresh_build = branch != 'append' -%} + + {#- load: stage before the in-tx pre-hooks; build: stage after them. See + sqlserver__pre_hook_transaction_scope for the two flows. Covers the + append branch's __dbt_temp build too. Inert on prebuilt, whose setup + must follow the hooks (a hook may read {{ this }} before the rebuild + drops it). -#} + {%- set pre_hook_transaction_scope = sqlserver__pre_hook_transaction_scope() -%} + {%- set stage_before_hooks = pre_hook_transaction_scope == 'load' and branch != 'prebuilt' -%} + {%- set build_relation = temp_relation if branch == 'append' else intermediate_relation -%} + {%- set build_is_temporary = branch == 'append' -%} + {%- set tmp_vw_relation = build_relation.incorporate( + path={"identifier": build_relation.identifier ~ '__dbt_tmp_vw'}, type='view' + ) -%} -- the temp_ and backup_ relations should not already exist in the database; get_relation -- will return None in that case. Otherwise, we get a relation that we can drop @@ -26,38 +58,28 @@ {{ run_hooks(pre_hooks, inside_transaction=False) }} + {#- Stage now: nothing is open (outside-tx hooks autocommit, the contract + probe never begins), so with auto_begin=False each statement autocommits + and the new object's Sch-M ends with its statement (#819). -#} + {% if stage_before_hooks %} + {%- set stage_sql = sqlserver__get_create_table_stage_sql(build_is_temporary, build_relation, sql) -%} + {% call statement('create_table_stage', auto_begin=False) %} + {{ stage_sql }} + {% endcall %} + {% endif %} + -- `BEGIN` happens here: {{ run_hooks(pre_hooks, inside_transaction=True) }} {% set to_drop = [] %} - {% set prebuilt_handled = false %} - {#- true only where the statement('main') batch below carries create_table_as - DDL, i.e. the fresh-create / full-refresh branches. The incremental - branch's strategy DML stays transactional, so it leaves this false. -#} - {% set build_sql_is_create_table_as = false %} - - {% if existing_relation is none %} - {% if config.get('full_refresh_build', 'heap_then_index') == 'prebuilt' %} - {#- first build: load straight into the clustered design. Calls its own - statement() blocks (including 'main') rather than returning SQL to - run below, so it can commit its in-progress marker independently of - the load that follows - see the macro for why. -#} - {% do sqlserver__create_table_as_prebuilt(target_relation, sql) %} - {% set prebuilt_cache_add = true %} - {% set prebuilt_handled = true %} - {% else %} - {% set build_sql = get_create_table_as_sql(False, target_relation, sql) %} - {% set build_sql_is_create_table_as = true %} - {% endif %} - {% elif full_refresh_mode %} - {#- the target is marked as having a full refresh in flight (blocking - normal runs until one completes), but only AFTER anything that can - fail on config alone - a pure config error must not mark a healthy - table -#} - {% if config.get('full_refresh_build', 'heap_then_index') == 'prebuilt' and should_full_refresh() %} + + {% if branch == 'prebuilt' %} + {% if existing_relation is not none %} {#- in-place full refresh: drop the existing table, rebuild the target - directly with no intermediate or swap (explicit --full-refresh - only; view->table conversions keep the default path) -#} + directly with no intermediate or swap. The target is marked as + having a full refresh in flight (blocking normal runs until one + completes), but only AFTER anything that can fail on config alone - + a pure config error must not mark a healthy table -#} {% do sqlserver__assert_no_unguarded_self_reference(target_relation, sql) %} {#- validate the index config BEFORE marking or dropping anything -#} {% do adapter.validate_indexes( @@ -69,34 +91,83 @@ {% do sqlserver__mark_full_refresh_incomplete(existing_relation) %} {% endif %} {% do adapter.drop_relation(existing_relation) %} - {% do sqlserver__create_table_as_prebuilt(target_relation, sql) %} - {% set prebuilt_cache_add = true %} - {% set prebuilt_handled = true %} + {% endif %} + {#- Calls its own statement() blocks (including 'main') rather than + returning SQL to run below, so it can commit its in-progress marker + independently of the load that follows - see the macro for why. -#} + {% do sqlserver__create_table_as_prebuilt(target_relation, sql) %} + {#- the prebuilt path lands the table via raw SQL, not a cache-maintaining + adapter method (rename_relation/drop_relation), so register it here to + keep dbt's relation cache in sync with the database. On the + full-refresh branch this also re-adds the target that drop_relation + removed from the cache. -#} + {% do adapter.cache_added(target_relation) %} + + {% elif branch == 'create' %} + {#- Build into the intermediate and swap, never straight into the target: + the create and the load commit independently, so a failed load would + leave an EMPTY table under the real name, and the next run would take + the append branch and merge into it - silent data loss. Staging into + __dbt_tmp leaves no target on failure and gets the OBJECT_ID drop + guard for free. -#} + {% if existing_relation is not none and existing_relation.type == 'table' %} + {#- marks the refresh in flight and commits that on its own - taking any + transaction: true pre-hook with it, so build cannot deliver rollback + here (docs/transaction_scope.md) -#} + {% do sqlserver__mark_full_refresh_incomplete(existing_relation) %} + {% endif %} + {% if stage_before_hooks %} + {#- The stage committed before the hooks. The load joins a pre-hook's + transaction if one is open, else autocommits; X table lock either + way. The tmp view is dropped on the tail, after the commit. -#} + {%- set load_sql = sqlserver__get_create_table_load_sql(False, intermediate_relation, sql, drop_tmp_view=False) -%} + {% call statement("main", auto_begin=False) %} + {{ load_sql }} + {% endcall %} + {#- statement() writes target/run/ for 'main' only; put the CREATE back. -#} + {% do write(stage_sql ~ '\n' ~ load_sql) %} {% else %} - {% set build_sql = get_create_table_as_sql(False, intermediate_relation, sql) %} - {% set build_sql_is_create_table_as = true %} - {% if existing_relation.type == 'table' %} - {% do sqlserver__mark_full_refresh_incomplete(existing_relation) %} - {% endif %} - {% set need_swap = true %} + {#- build: create and load inside the pre-hook's transaction, Sch-M held + for the whole load (#819). Chosen explicitly - and only ever taken + from a hook: auto_begin=False, so with no transactional pre-hook + this batch autocommits rather than opening a transaction of its + own and holding that Sch-M for a model that asked for nothing. -#} + {%- set stage_sql = sqlserver__get_create_table_stage_sql(False, intermediate_relation, sql) -%} + {%- set load_sql = sqlserver__get_create_table_load_sql(False, intermediate_relation, sql) -%} + {% call statement("main", auto_begin=False) %} + {{ stage_sql }} + {{ load_sql }} + {% endcall %} {% endif %} - {% else %} + {#- the swap and the tail need a transaction; with no pre-hook one, + nothing above left one open -#} + {% do adapter.begin_if_closed() %} + + {#- nothing to back up on a first build (sp_rename on a missing name is + Msg 15225); only queue a backup for dropping when one was made -#} + {% if existing_relation is not none %} + {% do adapter.rename_relation(target_relation, backup_relation) %} + {% do to_drop.append(backup_relation) %} + {% endif %} + {% do adapter.rename_relation(intermediate_relation, target_relation) %} + {% else %} {#- refuse to append onto a table whose last full refresh never completed -#} {% if existing_relation.type == 'table' %} {% do sqlserver__assert_no_incomplete_full_refresh(existing_relation) %} {% endif %} - {#- The temp build is all catalog DDL (CREATE OR ALTER VIEW / SELECT * - INTO / DROP VIEW) and must not share the ambient transaction with the - strategy DML: held to commit, its sysschobjs X keylocks deadlock a - second worker. Nothing opens a transaction here - run_query never - auto-begins, and find_references (relation.sql) no longer does either - - so each statement autocommits and drops its catalog locks as it - finishes. The strategy DML below still runs transactionally, via - statement('main')'s default auto_begin through to adapter.commit(). -#} - {% do run_query(get_create_table_as_sql(True, temp_relation, sql)) %} + {#- Only the temp load runs here; its create was staged before the hooks. + It joins a pre-hook's transaction (X lock on the temp table) or + autocommits, so its catalog locks never wait for the strategy DML's + commit - held that long they deadlocked a second worker. The strategy + DML below is transactional through to adapter.commit(). -#} + {% if stage_before_hooks %} + {% do run_query(sqlserver__get_create_table_load_sql(True, temp_relation, sql, drop_tmp_view=False)) %} + {% else %} + {% do run_query(get_create_table_as_sql(True, temp_relation, sql)) %} + {% endif %} {% set contract_config = config.get('contract') %} {% if not contract_config or not contract_config.enforced %} @@ -119,49 +190,54 @@ {% set strategy_arg_dict = ({'target_relation': target_relation, 'temp_relation': temp_relation, 'unique_key': unique_key, 'dest_columns': dest_columns, 'incremental_predicates': incremental_predicates }) %} {% set build_sql = strategy_sql_macro_func(strategy_arg_dict) %} + {% call statement("main") %} + {{ build_sql }} + {% endcall %} + {% do to_drop.append(temp_relation) %} {% endif %} - {% if not prebuilt_handled %} - {% if build_sql_is_create_table_as %} - {#- Same reason as the temp build above: this batch is create_table_as - catalog DDL, so letting statement() open the ambient transaction - would hold its sysschobjs X keylocks until adapter.commit() and - deadlock a second worker. -#} - {% call statement("main", auto_begin=False) %} - {{ build_sql }} - {% endcall %} - {% else %} - {% call statement("main") %} - {{ build_sql }} - {% endcall %} - {% endif %} - {% if build_sql_is_create_table_as %} - {#- Reopen the ambient transaction the batch above declined to start, - so the swap and the tail (grants/persist_docs/masks/indexes/ - post-hooks) keep their semantics and adapter.commit() below has a - matching BEGIN rather than raising Msg 3902. No-op when the flag is - off. -#} - {% do adapter.commit_if_open() %} - {% do adapter.begin_if_closed() %} - {% endif %} + {% set mask_config = adapter.resolve_masks(model, config.get('masks')) %} + {% if fresh_build %} + {#- Fresh table: masks before create_indexes (a mask cannot be added to + an index key column), and inside the transaction - the table carries + no masks yet, so a failure after the commit would leave it live and + exposed. -#} + {% do apply_masks(target_relation, mask_config) %} {% endif %} - {% if need_swap %} - {% do adapter.rename_relation(target_relation, backup_relation) %} - {% do adapter.rename_relation(intermediate_relation, target_relation) %} - {% do to_drop.append(backup_relation) %} + {{ run_hooks(post_hooks, inside_transaction=True) }} + + {#- Atomic unit ends here, as in table.sql: in-tx pre-hooks, load or + strategy DML, cutover, fresh-table masks, in-tx post-hooks. Index work, + grants, denies and persist_docs run outside, so sp_rename's Sch-M on + the live target does not span the index builds (#819). A post-hook + that needs the indexes: transaction: false. -#} + {% do adapter.commit_if_open() %} + + {#- tmp view, dropped by name now that nothing is open (an uncommitted + DROP VIEW blocks catalog scans like an uncommitted CREATE) -#} + {% if stage_before_hooks %} + {% call statement('drop_tmp_view', auto_begin=False) -%} + {{ get_use_database_sql(tmp_vw_relation.database) }} + DROP VIEW IF EXISTS {{ tmp_vw_relation.include(database=False) }}; + {%- endcall %} {% endif %} - {% if prebuilt_cache_add %} - {#- the prebuilt path lands the table via raw SQL, not a cache-maintaining - adapter method (rename_relation/drop_relation), so register it here to - keep dbt's relation cache in sync with the database. On the - full-refresh branch this also re-adds the target that drop_relation - removed from the cache. -#} - {% do adapter.cache_added(target_relation) %} + {% if fresh_build %} + {% do create_indexes(target_relation) %} + {% else %} + {# Table persisted across this run: converge its indexes on the config, + then reconcile masks (index drops land first). The table already + carries its previous masks, so a failure here exposes nothing. #} + {% do sqlserver__reconcile_indexes(target_relation) %} + {% do apply_masks(target_relation, mask_config) %} {% endif %} + {#- an ONLINE/RESUMABLE index build leaves a transaction open; close it so + grants and persist_docs do not run inside one held to commit -#} + {% do adapter.commit_if_open() %} + {% set should_revoke = should_revoke(existing_relation, full_refresh_mode) %} {% do apply_grants(target_relation, grant_config, should_revoke=should_revoke) %} @@ -172,20 +248,9 @@ {% do persist_docs(target_relation, model) %} - {% set mask_config = adapter.resolve_masks(model, config.get('masks')) %} - {% if existing_relation is none or existing_relation.is_view or should_full_refresh() %} - {# Freshly built table: mask before creating (rowstore) indexes, since a - mask cannot be added to a column an index depends on (all versions). #} - {% do apply_masks(target_relation, mask_config) %} - {% do create_indexes(target_relation) %} - {% else %} - {# Table persisted across this run: converge its indexes on the config, - then reconcile masks (index drops land first). #} - {% do sqlserver__reconcile_indexes(target_relation) %} - {% do apply_masks(target_relation, mask_config) %} - {% endif %} - - {{ run_hooks(post_hooks, inside_transaction=True) }} + {#- adapter.commit() raises with nothing open, and apply_grants only opens + one when grants are configured; state the precondition -#} + {% do adapter.begin_if_closed() %} -- `COMMIT` happens here {% do adapter.commit() %} diff --git a/dbt/include/sqlserver/macros/materializations/models/table/table.sql b/dbt/include/sqlserver/macros/materializations/models/table/table.sql index 690841be..10bf8b36 100644 --- a/dbt/include/sqlserver/macros/materializations/models/table/table.sql +++ b/dbt/include/sqlserver/macros/materializations/models/table/table.sql @@ -38,17 +38,56 @@ and existing_relation.type == 'table' ) -%} + {#- load: stage before the in-tx pre-hooks; build: stage after them. See + sqlserver__pre_hook_transaction_scope for the two flows. Inert on + prebuilt, whose setup must follow the hooks (a hook may read {{ this }} + before the rebuild drops it). -#} + {%- set pre_hook_transaction_scope = sqlserver__pre_hook_transaction_scope() -%} + {%- set stage_before_hooks = pre_hook_transaction_scope == 'load' and not use_prebuilt -%} + {%- set tmp_vw_relation = intermediate_relation.incorporate( + path={"identifier": intermediate_relation.identifier ~ '__dbt_tmp_vw'}, type='view' + ) -%} + -- drop the temp relations if they exist already in the database {{ drop_relation_if_exists(preexisting_intermediate_relation) }} {{ drop_relation_if_exists(preexisting_backup_relation) }} {{ run_hooks(pre_hooks, inside_transaction=False) }} + {#- Stage now: nothing is open (outside-tx hooks autocommit, the contract + probe never begins), so with auto_begin=False each statement autocommits + and the new object's Sch-M ends with its statement (#819). -#} + {% if stage_before_hooks %} + {% if use_dml_refresh %} + {% set dml_stage = sqlserver__table_dml_refresh_stage(target_relation, sql) %} + {% else %} + {%- set stage_sql = sqlserver__get_create_table_stage_sql(False, intermediate_relation, sql) -%} + {% call statement('create_table_stage', auto_begin=False) -%} + {{ stage_sql }} + {%- endcall %} + {% endif %} + {% endif %} + -- `BEGIN` happens here: {{ run_hooks(pre_hooks, inside_transaction=True) }} + {#- Resolved once: the rename and prebuilt paths apply masks inside the + cutover transaction, the dml path reconciles them outside it. -#} + {% set mask_config = adapter.resolve_masks(model, config.get('masks')) %} + {#- 'create' = fresh table, mask-then-create-index. 'reconcile' = the table + persisted, so indexes converge on config first and masks follow (an index + drop has to land before a column it covers can be masked). -#} + {% set index_strategy = 'create' %} + {% if use_dml_refresh %} - {{ sqlserver__table_dml_refresh(target_relation, sql) }} + {% if not stage_before_hooks %} + {% set dml_stage = sqlserver__table_dml_refresh_stage(target_relation, sql) %} + {% endif %} + {#- Leaves the swap's transaction open for the tail to close after the + post-hooks; returns schema_match (picks the tail's index strategy) and + the scratch table for the tail to drop after the commit. -#} + {% set dml_result = sqlserver__table_dml_refresh(target_relation, sql, dml_stage) %} + {% set index_strategy = 'reconcile' if dml_result['schema_match'] else 'create' %} {% elif use_prebuilt %} {#- in-place rebuild: drop the existing table, then build the target directly with no intermediate or swap -#} @@ -80,23 +119,39 @@ relation cache stays in sync with the database -#} {% do adapter.cache_added(target_relation) %} - {#-- Apply masks after the load but before create_indexes, mirroring the - standard build path so masks on nonclustered-index key columns land - before those indexes exist (mask-then-index). prebuilt builds the - clustered design inside create_table_as_prebuilt before we get here: - a CCI exposes no key columns so masks apply freely, but a mask on a - clustered *rowstore* key column cannot be added after the fact and - apply_masks raises a descriptive index-key error (recovery: switch - that model to the default heap_then_index). --#} - {% set mask_config = adapter.resolve_masks(model, config.get('masks')) %} + {#- Masks before create_indexes (a mask cannot be added to an index key + column). prebuilt already built its clustered design: a CCI exposes + no key columns, but a mask on a clustered rowstore key column fails + here with a descriptive error (recovery: heap_then_index). -#} {% do apply_masks(target_relation, mask_config) %} - - {% do create_indexes(target_relation) %} {% else %} -- build model - {% call statement('main') -%} - {{ get_create_table_as_sql(False, intermediate_relation, sql) }} - {%- endcall %} + {% if stage_before_hooks %} + {#- The stage committed before the hooks. The load joins a pre-hook's + transaction if one is open, else autocommits; X table lock either + way. The tmp view is dropped on the tail, after the commit. -#} + {%- set load_sql = sqlserver__get_create_table_load_sql(False, intermediate_relation, sql, drop_tmp_view=False) -%} + {% call statement('main', auto_begin=False) -%} + {{ load_sql }} + {%- endcall %} + {#- statement() writes target/run/ for 'main' only; put the CREATE back. -#} + {% do write(stage_sql ~ '\n' ~ load_sql) %} + {% else %} + {#- build: create and load inside the pre-hook's transaction, Sch-M held + for the whole load (#819). Chosen explicitly - and only ever taken + from a hook: auto_begin=False, so with no transactional pre-hook + this batch autocommits rather than opening a transaction of its + own and holding that Sch-M for a model that asked for nothing. -#} + {%- set stage_sql = sqlserver__get_create_table_stage_sql(False, intermediate_relation, sql) -%} + {%- set load_sql = sqlserver__get_create_table_load_sql(False, intermediate_relation, sql) -%} + {% call statement('main', auto_begin=False) -%} + {{ stage_sql }} + {{ load_sql }} + {%- endcall %} + {% endif %} + {#- the renames and the tail need a transaction; with no pre-hook one, + nothing above left one open -#} + {% do adapter.begin_if_closed() %} -- cleanup {% if existing_relation is not none %} @@ -110,19 +165,64 @@ {{ adapter.rename_relation(intermediate_relation, target_relation) }} - {#-- Apply data masks before create_indexes: a mask cannot be added to a - column an index depends on (documented for all SQL Server versions; - the fix is to mask first, then create the index — exactly this order), - so masking must happen while the (rowstore) indexes do not yet exist. - The clustered columnstore index built during CTAS is fine — columnstore - columns are reported as included, not index keys, and can be masked. --#} - {% set mask_config = adapter.resolve_masks(model, config.get('masks')) %} + {#- Masks before create_indexes (a mask cannot be added to an index key + column; a CCI exposes none), and INSIDE the cutover transaction: this + table carries no masks yet, so a failure after the commit would leave + it live and exposed. Rolling the swap back keeps the old, masked table + serving. -#} {% do apply_masks(target_relation, mask_config) %} + {% endif %} + + {{ run_hooks(post_hooks, inside_transaction=True) }} + + {#- Atomic unit ends here: in-tx pre-hooks, load, cutover, fresh-table + masks, in-tx post-hooks - what a transaction: true hook asks to be + atomic with. The rest is adapter housekeeping and runs outside, so + sp_rename's Sch-M on the live target does not span the index builds + (#819). A post-hook that needs the indexes: transaction: false. -#} + {% do adapter.commit_if_open() %} + + {#- Tmp views, dropped by name now that nothing is open. build scope + dropped its own in the fused batch; the dml fallback rebuild dropped + its own too; IF EXISTS covers both. -#} + {% if use_dml_refresh %} + {% call statement('dml_refresh_drop_view', auto_begin=False) -%} + {{ get_use_database_sql(dml_stage['tmp_vw_relation'].database) }} + DROP VIEW IF EXISTS {{ dml_stage['tmp_vw_relation'].include(database=False) }}; + {%- endcall %} + {% elif stage_before_hooks %} + {% call statement('drop_tmp_view', auto_begin=False) -%} + {{ get_use_database_sql(tmp_vw_relation.database) }} + DROP VIEW IF EXISTS {{ tmp_vw_relation.include(database=False) }}; + {%- endcall %} + {% endif %} + {#- Index work outside the cutover transaction. reconcile (dml swap: the + table persisted): indexes converge on config first so a drop lands + before apply_masks re-masks a column it covered; previous masks stay in + place on failure. create (fresh table): masks were applied inside the + transaction above. -#} + {% if index_strategy == 'reconcile' %} + {% do sqlserver__reconcile_indexes(target_relation) %} + {% do apply_masks(target_relation, mask_config) %} + {% else %} {% do create_indexes(target_relation) %} {% endif %} - {{ run_hooks(post_hooks, inside_transaction=True) }} + {#- sqlserver__create_indexes_no_txn ends with begin_if_closed, so an + ONLINE/RESUMABLE index leaves a transaction open here. Close it, or the + grants and persist_docs below would run inside one held to the commit - + putting back part of the window this tail exists to remove. -#} + {% do adapter.commit_if_open() %} + + {#- dml scratch table, dropped after the commit so its catalog locks end + with the statement. -#} + {% if use_dml_refresh and dml_result['refresh_relation'] is not none %} + {% call statement('dml_refresh_cleanup_post', auto_begin=False) -%} + {{ get_use_database_sql(dml_result['refresh_relation'].database) }} + DROP TABLE IF EXISTS {{ dml_result['refresh_relation'] }}; + {%- endcall %} + {% endif %} {% set should_revoke = should_revoke(existing_relation, full_refresh_mode=True) %} {% do apply_grants(target_relation, grant_config, should_revoke=should_revoke) %} @@ -135,6 +235,12 @@ {% do persist_docs(target_relation, model) %} + {#- apply_grants opens a transaction of its own (dbt's call_dcl_statements + uses the default auto_begin), so one is usually open by now - but not + when a model configures no grants. adapter.commit() raises if it finds + nothing open, so state the precondition rather than relying on that. -#} + {% do adapter.begin_if_closed() %} + -- `COMMIT` happens here {{ adapter.commit() }} diff --git a/dbt/include/sqlserver/macros/materializations/models/table/table_dml_refresh.sql b/dbt/include/sqlserver/macros/materializations/models/table/table_dml_refresh.sql index f7dc0aa8..26d0b92e 100644 --- a/dbt/include/sqlserver/macros/materializations/models/table/table_dml_refresh.sql +++ b/dbt/include/sqlserver/macros/materializations/models/table/table_dml_refresh.sql @@ -1,60 +1,85 @@ -{% macro sqlserver__table_dml_refresh(target_relation, sql) %} - {# - The DELETE + INSERT swap below (dml_refresh_swap) is only safe because - every connection sets SET XACT_ABORT ON at session level (see - dbt/adapters/sqlserver/sqlserver_connections.py, xact_abort credential, - dbt-msft/dbt-sqlserver#718). Without it, a run-time error partway - through the swap (e.g. a NOT NULL/constraint violation on the INSERT) - only aborts that statement, not the batch — the DELETE can still - commit, silently emptying the target. Do not add a per-macro - SET XACT_ABORT ON here; the session-level default is the single source - of truth, and do not "simplify" this back into an unguarded batch. - - DML-only table refresh for use under RCSI. - - Instead of rename-swap (which uses DDL and creates a window where the - table name doesnt resolve), this macro: - 1. Builds new data into a scratch table via SELECT INTO (minimally logged) - 2. Compares schemas — if columns changed, falls back to rename-swap - 3. Swaps data via DELETE + INSERT inside an explicit transaction - (RCSI ensures concurrent readers see old data until COMMIT) - 4. Cleans up the scratch table - - The scratch table is a regular table with a __dbt_refresh suffix, - not a global temp table. This avoids cross-session visibility issues - and ensures cleanup on failure (DROP IF EXISTS at the start of each run). - #} - +{% macro sqlserver__table_dml_refresh_stage(target_relation, sql) %} + {#- + Schema resolution for the dml refresh: drop leftovers by name, create + the tmp view over the model SQL, create the scratch table EMPTY. Every + statement passes auto_begin=False. table.sql calls this before the in-tx + pre-hooks under load (nothing open: each statement autocommits, so the + scratch table's Sch-M ends with its create) and after them under build. + Returns the two relations the load and the tail need. + -#} {%- set refresh_relation = target_relation.incorporate( path={"identifier": target_relation.identifier ~ '__dbt_refresh'} ) -%} {%- set tmp_vw_relation = refresh_relation.incorporate( - path={"identifier": refresh_relation.identifier ~ '__dbt_tmp_vw'} + path={"identifier": refresh_relation.identifier ~ '__dbt_tmp_vw'}, type='view' ) -%} - {#- Query hint for the grant-taking data-movement statements below (SELECT INTO - and the swap INSERT). get_query_options() emits the OPTION (...) clause and - terminates it with ';', matching how create_table_as appends it. -#} - {%- set query_label = get_query_options(parse_options=True) -%} - - {# Clean up any leftovers from a prior failed run #} - {% call statement('dml_refresh_cleanup_pre') -%} + {% call statement('dml_refresh_cleanup_pre', auto_begin=False) -%} + {{ get_use_database_sql(refresh_relation.database) }} DROP VIEW IF EXISTS {{ tmp_vw_relation.include(database=False) }}; DROP TABLE IF EXISTS {{ refresh_relation }}; {%- endcall %} {# Build new data into scratch table via temp view (handles CTEs in model SQL) #} - {# Named 'main' because dbt requires a statement('main') call in every materialization #} - {% call statement('dml_refresh_create_view') -%} + {% call statement('dml_refresh_create_view', auto_begin=False) -%} {{ get_create_view_as_sql(tmp_vw_relation, sql) }} {%- endcall %} - {% call statement('main') -%} - SELECT * INTO {{ refresh_relation }} FROM {{ tmp_vw_relation }} {{ query_label }} + {#- Create the scratch table empty. It is never contract-enforced: contracts + describe the model's target, and this table exists only to stage rows + for the swap, which then inserts into the real (already contracted) + target. -#} + {% call statement('dml_refresh_create_scratch', auto_begin=False) -%} + {{ sqlserver__get_create_table_empty_sql(refresh_relation, tmp_vw_relation, sql, false) }} {%- endcall %} - {% call statement('dml_refresh_drop_view') -%} - DROP VIEW IF EXISTS {{ tmp_vw_relation.include(database=False) }}; + {{ return({'refresh_relation': refresh_relation, 'tmp_vw_relation': tmp_vw_relation}) }} +{% endmacro %} + + +{% macro sqlserver__table_dml_refresh(target_relation, sql, stage) %} + {#- + XACT_ABORT. The DELETE + INSERT swap below is only safe because every + connection runs SET XACT_ABORT ON at session level (#718): without it a + run-time error in the INSERT aborts that statement only, and the DELETE + can still commit, silently emptying the target. Do not add a per-macro + SET, and do not fold the swap back into an unguarded batch. + + Flow (#819). Sch-M conflicts with the Sch-S every metadata reader takes, + so no statement that holds one may share a transaction with the load. + + stage (table.sql, before the in-tx hooks) autocommit + |- DROP leftovers, CREATE VIEW, SELECT TOP 0 * INTO Sch-M ends per statement + BEGIN first in-tx pre-hook, else the swap + |- INSERT scratch WITH (TABLOCK) X table lock only + |- schema compare read-only probes + |- DELETE target + INSERT target X on the target + |- in-tx post-hooks atomic with the swap + COMMIT table.sql + |- DROP VIEW, reconcile indexes, masks, DROP scratch, grants, docs + + RCSI keeps concurrent readers on the old rows until COMMIT. On a schema + change the swap is skipped and the scratch table is rebuilt and renamed + into place instead (below). The scratch table is a real table with a + __dbt_refresh suffix, not a global temp table, so it is visible for the + schema compare and droppable by name on the next run. + -#} + + {%- set refresh_relation = stage['refresh_relation'] -%} + {%- set tmp_vw_relation = stage['tmp_vw_relation'] -%} + + {#- Query hint for the grant-taking data-movement statements below (the scratch + load and the swap INSERT; not the empty create, which moves no rows). + get_query_options() emits the OPTION (...) clause and terminates it with + ';', matching how create_table_as appends it. -#} + {%- set query_label = get_query_options(parse_options=True) -%} + + {#- Named 'main' because dbt requires a statement('main') call in every + materialization, and this is the statement worth having there: it is the + one that moves the rows, so adapter_response still reports a meaningful + row count. -#} + {% call statement('main', auto_begin=False) -%} + {{ sqlserver__get_tablock_insert_sql(refresh_relation, tmp_vw_relation, query_label, false) }} {%- endcall %} {# Compare schemas: if columns differ, fall back to rename-swap #} @@ -72,10 +97,10 @@ {%- set target_columns = adapter.get_columns_in_relation(target_relation) -%} {%- set column_list = target_columns | map(attribute='quoted') | join(', ') -%} - {# Atomic DML swap — RCSI protects concurrent readers #} - {# When dbt_sqlserver_use_dbt_transactions is off (default), autocommit #} - {# means we need the explicit BEGIN/COMMIT. When the flag is on, dbt #} - {# already wraps the statement call in a transaction, so skip it. #} + {#- The swap. With dbt-managed transactions off, the in-batch + BEGIN/COMMIT makes it atomic; with them on (default) this statement's + auto_begin opens the transaction unless a pre-hook already did, and + table.sql commits it after the in-tx post-hooks. -#} {% call statement('dml_refresh_swap') -%} {% if not adapter.behavior.dbt_sqlserver_use_dbt_transactions %} BEGIN TRANSACTION; @@ -88,34 +113,31 @@ {% endif %} {%- endcall %} - {# Cleanup scratch table #} - {% call statement('dml_refresh_cleanup_post') -%} - DROP TABLE IF EXISTS {{ refresh_relation }}; - {%- endcall %} - - {# The target table persisted (no rebuild), so converge its indexes on - the config. Runs after the swap's self-contained transaction. #} - {% do sqlserver__reconcile_indexes(target_relation) %} - - {# Persisted-table path: masks already exist from the prior build; this - reconciles any config change. Runs after reconcile so index drops land - first. #} - {% do apply_masks(target_relation, mask_config) %} + {#- Deliberately left open: table.sql commits after the in-tx post-hooks, + so a transaction: true post-hook is atomic with the swap. Index and + mask reconciliation run on the tail, outside; if they fail the new + rows are committed with indexes not yet converged, and the next run + converges them (both reconcile against config, not a delta). -#} {% else %} {# Schema changed — fall back to rename-swap for this run #} {{ log("Schema change detected for " ~ target_relation ~ " — falling back to rename-swap", info=true) }} + {#- The scratch load joined no transaction of its own; open one for this + branch's renames and drops, which stay transactional. -#} + {% do adapter.begin_if_closed() %} + {%- set backup_relation_type = target_relation.type -%} {%- set backup_relation = make_backup_relation(target_relation, backup_relation_type) -%} {{ drop_relation_if_exists(backup_relation) }} - {#- The scratch table above came from SELECT * INTO, which is the right - shape for the schema probe and the wrong one for the object that is - about to be renamed into position: it copies no constraint and no - index, and takes nullability from the query rather than from a - contract. Left as-is it silently strips the model of its clustered - columnstore index - create_indexes only builds what the `indexes` + {#- The scratch table above came from the empty-create load (SELECT TOP 0 + INTO plus the TABLOCK insert), which is the right shape for the schema + probe and the wrong one for the object that is about to be renamed into + position: it copies no constraint and no index, and takes nullability + from the query rather than from a contract. Left as-is it silently + strips the model of its clustered columnstore index - create_indexes + only builds what the `indexes` config names, never the as_columnstore CCI - and, under a contract, of its NOT NULLs and inline constraints too. None of it came back on a later run, because every later run matched the new schema and took the @@ -128,7 +150,7 @@ rare, so one extra build here is much the cheaper trade. It is not free, though: the model's SQL runs a second time here, the - SELECT * INTO above having already run it once as the schema probe. + scratch load above having already run it once as the schema probe. Any side effect in that SQL therefore happens twice, and the two runs are not interchangeable - the schema decision came from the first, the table renamed into position comes from the second. A model whose column @@ -138,7 +160,8 @@ back into one, at the cost of changing how the probe behaves - a separate change, not this one. - create_table_as builds and drops its own __dbt_tmp_vw. -#} + create_table_as builds and drops its own __dbt_tmp_vw - the same name + as the stage's view, which it replaces. -#} {{ drop_relation_if_exists(refresh_relation) }} {% call statement('dml_refresh_rebuild') -%} {{ get_create_table_as_sql(False, refresh_relation, sql) }} @@ -152,16 +175,26 @@ {{ adapter.rename_relation(refresh_relation, target_relation) }} - {# Freshly rebuilt (no masks carried), so apply masks before - create_indexes — a mask cannot be added to a column an index depends - on (documented for all SQL Server versions). #} + {#- Freshly rebuilt above (no masks carried), so apply masks before + create_indexes — a mask cannot be added to a column an index depends + on (documented for all SQL Server versions). Applied here, inside the + cutover transaction, rather than on table.sql's tail: this table is new + and unmasked, so a mask failure after the cutover committed would leave + it live with the columns exposed. create_indexes runs on the tail + instead; index_strategy='create' keeps the mask-then-index order. -#} {% do apply_masks(target_relation, mask_config) %} - {% do create_indexes(target_relation) %} - {{ drop_relation_if_exists(backup_relation) }} {# scratch table is now the target, nothing to drop #} {% endif %} + {#- schema_match picks the tail's index strategy: reconcile on the swap + path, create on the fallback (masked above, mask-then-index order). + refresh_relation is the scratch table for the tail to drop after the + commit; none on the fallback, where it was renamed into the target. -#} + {{ return({ + 'schema_match': schema_match, + 'refresh_relation': refresh_relation if schema_match else none + }) }} {% endmacro %} diff --git a/dbt/include/sqlserver/macros/materializations/snapshots/helpers.sql b/dbt/include/sqlserver/macros/materializations/snapshots/helpers.sql index 0f1e908b..66f122c7 100644 --- a/dbt/include/sqlserver/macros/materializations/snapshots/helpers.sql +++ b/dbt/include/sqlserver/macros/materializations/snapshots/helpers.sql @@ -14,24 +14,39 @@ {% endmacro %} -{% macro build_snapshot_staging_table(strategy, temp_snapshot_relation, target_relation) %} - {% set temp_relation = make_temp_relation(target_relation) %} - {{ adapter.drop_relation(temp_relation) }} - - {% set select = snapshot_staging_table(strategy, temp_snapshot_relation, target_relation) %} - - {% set tmp_tble_vw_relation = temp_relation.incorporate(path={"identifier": temp_relation.identifier ~ '__dbt_tmp_vw'}, type='view')-%} - -- Dropping temp view relation if it exists - {{ adapter.drop_relation(tmp_tble_vw_relation) }} - - {% call statement('build_snapshot_staging_relation') %} - {{ get_create_table_as_sql(True, temp_relation, select) }} - {% endcall %} +{% macro sqlserver__snapshot_stage(strategy, temp_snapshot_relation, temp_snapshot_relation_sql, + target_relation, target_relation_exists, + build_relation, build_is_temporary) %} + {#- + Schema resolution for a snapshot run, in dependency order: the view + over the user SQL (rendering the staging select probes it), the build + select, then the tmp view and the empty CREATE of what this run builds. + Never begins a transaction (auto_begin=False throughout): ahead of the + in-tx pre-hooks each statement autocommits and the new object's Sch-M + ends with it (load scope, #819); after them these join the transaction + the hooks opened (build scope). A build scope with no transactional + pre-hook has nothing open to join, so it autocommits too rather than + taking a lock nothing asked for. + Returns the build select and the rendered stage SQL, so the + materialization can write the whole build to the compiled artifact. + -#} + {{ adapter.drop_relation(temp_snapshot_relation) }} + {% call statement('create temp_snapshot_relation', auto_begin=False) -%} + {{ get_create_view_as_sql(temp_snapshot_relation, temp_snapshot_relation_sql) }} + {%- endcall %} + + {% if not target_relation_exists %} + {% set build_sql = build_snapshot_table(strategy, temp_snapshot_relation) %} + {% else %} + {% set build_sql = snapshot_staging_table(strategy, temp_snapshot_relation, target_relation) %} + {% endif %} - -- Dropping temp view relation if it exists - {{ adapter.drop_relation(tmp_tble_vw_relation) }} + {%- set stage_sql = sqlserver__get_create_table_stage_sql(build_is_temporary, build_relation, build_sql) -%} + {% call statement('create_table_stage', auto_begin=False) -%} + {{ stage_sql }} + {%- endcall %} - {% do return(temp_relation) %} + {% do return({'build_sql': build_sql, 'stage_sql': stage_sql}) %} {% endmacro %} diff --git a/dbt/include/sqlserver/macros/materializations/snapshots/snapshot.sql b/dbt/include/sqlserver/macros/materializations/snapshots/snapshot.sql index 13e01f2f..12ebcb73 100644 --- a/dbt/include/sqlserver/macros/materializations/snapshots/snapshot.sql +++ b/dbt/include/sqlserver/macros/materializations/snapshots/snapshot.sql @@ -17,8 +17,22 @@ {% do exceptions.relation_wrong_type(target_relation, 'table') %} {%- endif -%} + {#- load: stage before the in-tx pre-hooks; build: stage after them. See + sqlserver__pre_hook_transaction_scope for the two flows. -#} + {%- set pre_hook_transaction_scope = sqlserver__pre_hook_transaction_scope() -%} + {%- set stage_before_hooks = pre_hook_transaction_scope == 'load' -%} + + {#- Hooks run before the strategy work below, which talks to the database: + the check strategy runs the snapshot's own SQL to compare column shapes + (snapshot_check_all_get_existing_columns), so a hook that creates what + the snapshot reads has to have run by then - on every run but the first, + where the missing target skips that check. Under 'load' only the in-tx + hooks are held back, to after the stage: that is the scope's documented + trade, and the stage binds the same SQL anyway. -#} {{ run_hooks(pre_hooks, inside_transaction=False) }} - {{ run_hooks(pre_hooks, inside_transaction=True) }} + {% if not stage_before_hooks %} + {{ run_hooks(pre_hooks, inside_transaction=True) }} + {% endif %} {% set strategy_macro = strategy_dispatch(strategy_name) %} {% set strategy = strategy_macro(model, "snapshotted_data", "source_data", config, target_relation_exists) %} @@ -28,96 +42,136 @@ schema=model.schema, identifier=target_table+"_snapshot_staging_temp_view", type='view') -%} - - -- Create a temporary view to manage if user SQl uses CTE + -- A view over the user SQL, so a query that opens with a CTE can be read from {% set temp_snapshot_relation_sql = model['compiled_code'] %} - {{ adapter.drop_relation(temp_snapshot_relation) }} - - {% call statement('create temp_snapshot_relation') -%} - {{ get_create_view_as_sql(temp_snapshot_relation, temp_snapshot_relation_sql) }} - {%- endcall %} + {#- First build: through the __dbt_tmp intermediate, renamed into place, + as table does - the create and the load commit independently, so + building straight into the target would leave an EMPTY snapshot table + under the real name after a failed load, for the next run to merge + into. Later runs: build the __dbt_temp staging table and merge it. -#} {% if not target_relation_exists %} + {% set build_relation = make_intermediate_relation(target_relation) %} + {% set build_is_temporary = false %} + {{ drop_relation_if_exists(load_cached_relation(build_relation)) }} + {% else %} + {% set columns = get_snapshot_table_column_names() %} + {% set meta = config.get("snapshot_meta_column_names") %} + {% if meta %} + {% if meta.dbt_valid_from %}{% do columns.update({"dbt_valid_from": meta.dbt_valid_from}) %}{% endif %} + {% if meta.dbt_valid_to %}{% do columns.update({"dbt_valid_to": meta.dbt_valid_to}) %}{% endif %} + {% if meta.dbt_scd_id %}{% do columns.update({"dbt_scd_id": meta.dbt_scd_id}) %}{% endif %} + {% if meta.dbt_updated_at %}{% do columns.update({"dbt_updated_at": meta.dbt_updated_at}) %}{% endif %} + {% if meta.dbt_is_deleted %}{% do columns.update({"dbt_is_deleted": meta.dbt_is_deleted}) %}{% endif %} + {% endif %} + {{ adapter.valid_snapshot_target(target_relation, columns) }} + {% set staging_table = make_temp_relation(target_relation) %} + {% set build_relation = staging_table %} + {% set build_is_temporary = true %} + {{ adapter.drop_relation(staging_table) }} + {% endif %} + {%- set tmp_vw_relation = build_relation.incorporate( + path={"identifier": build_relation.identifier ~ '__dbt_tmp_vw'}, type='view' + ) -%} + + {#- load: nothing is open here, so sqlserver__snapshot_stage autocommits + each statement and the new object's Sch-M ends with it (#819). + build: the in-tx pre-hooks above ran first, so these statements join + their transaction and that Sch-M is held for the load. Chosen + explicitly - with no transactional pre-hook nothing is open either way + and the stage never begins a transaction of its own. -#} + {% set stage = sqlserver__snapshot_stage( + strategy, temp_snapshot_relation, temp_snapshot_relation_sql, + target_relation, target_relation_exists, + build_relation, build_is_temporary) %} + + {% if stage_before_hooks %} + {{ run_hooks(pre_hooks, inside_transaction=True) }} + {% endif %} + {% set build_sql = stage['build_sql'] %} + {% set stage_sql = stage['stage_sql'] %} - {% set build_sql = build_snapshot_table(strategy, temp_snapshot_relation) %} - {% set build_or_select_sql = build_sql %} + {{ check_time_data_types(build_sql) }} - -- naming a temp relation - {% set tmp_relation_view = target_relation.incorporate(path={"identifier": target_relation.identifier ~ '__dbt_tmp_vw'}, type='view')-%} - -- SQL server adapter uses temp relation because of lack of CTE support for CTE in CTAS, Insert - -- drop temp relation if exists - {{ adapter.drop_relation(tmp_relation_view) }} - {% set final_sql = get_create_table_as_sql(False, target_relation, build_sql) %} - {{ adapter.drop_relation(tmp_relation_view) }} + {#- The load joins a pre-hook's transaction if one is open, else + autocommits; X table lock either way. Tmp views are dropped on the + tail, after the commit. -#} + {%- set load_sql = sqlserver__get_create_table_load_sql(build_is_temporary, build_relation, build_sql, drop_tmp_view=False) -%} + {% if not target_relation_exists %} + {% call statement('main', auto_begin=False) -%} + {{ load_sql }} + {%- endcall %} + {#- statement() writes target/run/ for 'main' only; put the CREATE back -#} + {% do write(stage_sql ~ '\n' ~ load_sql) %} + {#- the rename and the tail need a transaction; with no pre-hook one, + nothing above left one open -#} + {% do adapter.begin_if_closed() %} + {% do adapter.rename_relation(build_relation, target_relation) %} {% else %} + {% do run_query(load_sql) %} + + -- this may no-op if the database does not require column expansion + {% set expansion_max_rows = config.get('column_type_expansion_max_rows', 1000000) %} + {% do adapter.expand_target_column_types(from_relation=staging_table, + to_relation=target_relation, + max_rows=expansion_max_rows) %} + + {% set remove_columns = ['dbt_change_type', 'DBT_CHANGE_TYPE', 'dbt_unique_key', 'DBT_UNIQUE_KEY'] %} + {% if unique_key | is_list %} + {% for key in strategy.unique_key %} + {{ remove_columns.append('dbt_unique_key_' + loop.index|string) }} + {{ remove_columns.append('DBT_UNIQUE_KEY_' + loop.index|string) }} + {% endfor %} + {% endif %} + {% set missing_columns = adapter.get_missing_columns(staging_table, target_relation) + | rejectattr('name', 'in', remove_columns) + | list %} + {% if missing_columns|length > 0 %} + {{log("Missing columns length is: "~ missing_columns|length)}} + {% do create_columns(target_relation, missing_columns) %} + {% endif %} + {% set source_columns = adapter.get_columns_in_relation(staging_table) + | rejectattr('name', 'in', remove_columns) + | list %} + {% set quoted_source_columns = [] %} + {% for column in source_columns %} + {% do quoted_source_columns.append(adapter.quote(column.name)) %} + {% endfor %} + {% set final_sql = snapshot_merge_sql( + target = target_relation, + source = staging_table, + insert_cols = quoted_source_columns + ) + %} + {% call statement('main') %} + {{ final_sql }} + {% endcall %} + {% endif %} - {% set columns = get_snapshot_table_column_names() %} - {% set meta = config.get("snapshot_meta_column_names") %} - {% if meta %} - {% if meta.dbt_valid_from %}{% do columns.update({"dbt_valid_from": meta.dbt_valid_from}) %}{% endif %} - {% if meta.dbt_valid_to %}{% do columns.update({"dbt_valid_to": meta.dbt_valid_to}) %}{% endif %} - {% if meta.dbt_scd_id %}{% do columns.update({"dbt_scd_id": meta.dbt_scd_id}) %}{% endif %} - {% if meta.dbt_updated_at %}{% do columns.update({"dbt_updated_at": meta.dbt_updated_at}) %}{% endif %} - {% if meta.dbt_is_deleted %}{% do columns.update({"dbt_is_deleted": meta.dbt_is_deleted}) %}{% endif %} - {% endif %} - {{ adapter.valid_snapshot_target(target_relation, columns) }} - {% set build_or_select_sql = snapshot_staging_table(strategy, temp_snapshot_relation, target_relation) %} - {% set staging_table = build_snapshot_staging_table(strategy, temp_snapshot_relation, target_relation) %} - -- this may no-op if the database does not require column expansion - {% set expansion_max_rows = config.get('column_type_expansion_max_rows', 1000000) %} - {% do adapter.expand_target_column_types(from_relation=staging_table, - to_relation=target_relation, - max_rows=expansion_max_rows) %} - - {% set remove_columns = ['dbt_change_type', 'DBT_CHANGE_TYPE', 'dbt_unique_key', 'DBT_UNIQUE_KEY'] %} - {% if unique_key | is_list %} - {% for key in strategy.unique_key %} - {{ remove_columns.append('dbt_unique_key_' + loop.index|string) }} - {{ remove_columns.append('DBT_UNIQUE_KEY_' + loop.index|string) }} - {% endfor %} - {% endif %} - {% set missing_columns = adapter.get_missing_columns(staging_table, target_relation) - | rejectattr('name', 'in', remove_columns) - | list %} - {% if missing_columns|length > 0 %} - {{log("Missing columns length is: "~ missing_columns|length)}} - {% do create_columns(target_relation, missing_columns) %} - {% endif %} - {% set source_columns = adapter.get_columns_in_relation(staging_table) - | rejectattr('name', 'in', remove_columns) - | list %} - {% set quoted_source_columns = [] %} - {% for column in source_columns %} - {% do quoted_source_columns.append(adapter.quote(column.name)) %} - {% endfor %} - {% set final_sql = snapshot_merge_sql( - target = target_relation, - source = staging_table, - insert_cols = quoted_source_columns - ) - %} + {% set mask_config = adapter.resolve_masks(model, config.get('masks')) %} + {% if not target_relation_exists %} + {#- Fresh table: masks before create_indexes (a mask cannot be added to + an index key column), and inside the transaction - the table carries + no masks yet, so a failure after the commit would leave it live and + exposed. -#} + {% do apply_masks(target_relation, mask_config) %} {% endif %} - {{ check_time_data_types(build_or_select_sql) }} - {% call statement('main') %} - {{ final_sql }} - {% endcall %} - {{ adapter.drop_relation(temp_snapshot_relation) }} - {% set should_revoke = should_revoke(target_relation_exists, full_refresh_mode=False) %} - {% do apply_grants(target_relation, grant_config, should_revoke=should_revoke) %} + {{ run_hooks(post_hooks, inside_transaction=True) }} - {#-- Re-apply object-level DENYs after grants. --#} - {% set deny_config = adapter.resolve_denies(model, config.get('denies')) %} - {% do apply_denies(target_relation, deny_config, should_revoke=should_revoke) %} + {#- Atomic unit ends here, as in table and incremental: in-tx pre-hooks, + load or merge, cutover, fresh-table masks, in-tx post-hooks. Index + work, grants, denies and persist_docs run outside (#819). -#} + {% do adapter.commit_if_open() %} - {% do persist_docs(target_relation, model) %} + {{ adapter.drop_relation(temp_snapshot_relation) }} + {% call statement('drop_tmp_view', auto_begin=False) -%} + {{ get_use_database_sql(tmp_vw_relation.database) }} + DROP VIEW IF EXISTS {{ tmp_vw_relation.include(database=False) }}; + {%- endcall %} - {% set mask_config = adapter.resolve_masks(model, config.get('masks')) %} {% if not target_relation_exists %} - {# Freshly built snapshot table: mask before creating (rowstore) indexes, - since a mask cannot be added to a column an index depends on (all versions). #} - {% do apply_masks(target_relation, mask_config) %} {% do create_indexes(target_relation) %} {% else %} {# Snapshot table persisted: converge its indexes on the config, then @@ -126,7 +180,20 @@ {% do apply_masks(target_relation, mask_config) %} {% endif %} - {{ run_hooks(post_hooks, inside_transaction=True) }} + {#- an ONLINE/RESUMABLE index build leaves a transaction open; close it so + grants and persist_docs do not run inside one held to commit -#} + {% do adapter.commit_if_open() %} + + {% set should_revoke = should_revoke(target_relation_exists, full_refresh_mode=False) %} + {% do apply_grants(target_relation, grant_config, should_revoke=should_revoke) %} + + {#-- Re-apply object-level DENYs after grants. --#} + {% set deny_config = adapter.resolve_denies(model, config.get('denies')) %} + {% do apply_denies(target_relation, deny_config, should_revoke=should_revoke) %} + + {% do persist_docs(target_relation, model) %} + + {% do adapter.begin_if_closed() %} {{ adapter.commit() }} {% if staging_table is defined %} diff --git a/dbt/include/sqlserver/macros/relations/table/create.sql b/dbt/include/sqlserver/macros/relations/table/create.sql index 31f2ecfa..be0772e7 100644 --- a/dbt/include/sqlserver/macros/relations/table/create.sql +++ b/dbt/include/sqlserver/macros/relations/table/create.sql @@ -1,5 +1,105 @@ -{% macro sqlserver__create_table_as(temporary, relation, sql) -%} - {%- set query_label = get_query_options(parse_options=True) -%} +{% macro sqlserver__get_create_table_empty_sql(relation, tmp_relation, sql, contract_enforced) -%} + {#- + SQL that creates `relation` empty, with no rows loaded. Pair it with + sqlserver__get_tablock_insert_sql to build a table in two statements + instead of one fused `SELECT * INTO`. + + Why the split matters: a fused `SELECT ... INTO` both creates the object + and loads it, so SQL Server holds the object's Sch-M lock from the moment + the statement starts until it finishes. Sch-M is the one mode + incompatible with the Sch-S lock every metadata reader takes, so a slow + load blocks metadata readers in *other* sessions for its whole duration + (dbt-msft/dbt-sqlserver#819). Creating the object empty takes Sch-M for + an instant; the load that follows takes no Sch-M at all. + + Splitting only pays off when the two statements do not share an open + transaction - locks are held to commit, not to end-of-statement - so + callers must either run the pair in autocommit or commit between them. + See each call site for how it does that. + + Returns SQL; it executes nothing, wraps nothing in EXEC() and manages no + transaction, so callers keep control of batching and lock boundaries. + + `contract_enforced` is a parameter rather than a config lookup on + purpose: sqlserver__create_table_as suppresses contracts for temporary + relations, so the call sites do not agree on how to derive it. Deriving + it here would silently change behaviour for temp builds. + + Only valid inside a model materialization: the contract branch reads the + ambient `model` context var via get_assert_columns_equivalent, which also + raises on a column mismatch. Keep that assertion in this macro only, so + it fires exactly once per build. + -#} + {%- if contract_enforced -%} + CREATE TABLE {{ relation }} + {{ get_assert_columns_equivalent(sql) }} + {{ build_columns_constraints(relation) }} + {%- else -%} + SELECT TOP 0 * INTO {{ relation }} FROM {{ tmp_relation }} + {%- endif -%} +{%- endmacro %} + + +{% macro sqlserver__get_tablock_insert_sql(relation, tmp_relation, query_label, contract_enforced) -%} + {#- + SQL that bulk-loads `relation` from `tmp_relation`. The companion to + sqlserver__get_create_table_empty_sql above; see it for why the create + and the load are separate statements. + + WITH (TABLOCK) is what keeps this minimally logged under the simple and + bulk-logged recovery models, matching the `SELECT * INTO` it replaces: + minimal logging needs a table lock on a heap with no nonclustered + indexes. Do not drop the hint to "reduce blocking" - it costs log volume, + and an X table lock is compatible with Sch-S anyway, so it never blocks + the metadata readers this split exists to protect. + + `query_label` is the OPTION (...) clause from + get_query_options(parse_options=True). It rides the data-movement + statement, not the empty create, because that is the statement whose plan + takes a memory grant and whose label people search for. + + Returns SQL; executes nothing. `contract_enforced` is a parameter for the + reason given on the create macro. + + Only valid inside a model materialization: the contract branch reads the + ambient `model` context var for its column list. + -#} + {%- if contract_enforced -%} + {%- set list_columns -%} + {%- for column in model['columns'] -%} + {{ adapter.quote(column) }}{{ ", " if not loop.last }} + {%- endfor -%} + {%- endset -%} + INSERT INTO {{ relation }} WITH (TABLOCK) ({{ list_columns }}) + SELECT {{ list_columns }} FROM {{ tmp_relation }} {{ query_label }} + {%- else -%} + INSERT INTO {{ relation }} WITH (TABLOCK) + SELECT * FROM {{ tmp_relation }} {{ query_label }} + {%- endif -%} +{%- endmacro %} + + +{% macro sqlserver__get_create_table_stage_sql(temporary, relation, sql) -%} + {#- + First half of a table build: everything up to and including creating the + object, loading no rows. Pair with sqlserver__get_create_table_load_sql. + + The split exists so a caller can put a transaction boundary between + creating the object and loading it. Locks are held to commit, not to + end-of-statement, so an empty CREATE that shares a transaction with the + load holds the new object's Sch-M for the whole load - blocking every + metadata reader in every other session (#819). Committing after this half + releases it in an instant, since SELECT TOP 0 moves no rows. + + Callers that want one batch call sqlserver__create_table_as, which is + exactly this half followed by the load half; it emits the same statements + in the same order, so snapshots and the incremental temp build are + unaffected by the split. + + Note the render-time `adapter.drop_relation` below: it is a side effect, + not emitted SQL, and lives here so it fires exactly once per build + whether the halves are rendered separately or through create_table_as. + -#} {%- set full_refresh_build = config.get('full_refresh_build', 'heap_then_index') -%} {%- if full_refresh_build not in ['heap_then_index', 'prebuilt'] -%} {{ exceptions.raise_compiler_error( @@ -26,40 +126,63 @@ {{ get_use_database_sql(relation.database) }} {{ get_create_view_as_sql(tmp_relation, sql) }} - {%- set table_name -%} - {{ relation }} + {%- set contract_config = config.get('contract') -%} + {#- not plain `contract_config.enforced`: contracts are suppressed for temp + builds, and the shared macros take the resolved flag as a parameter -#} + {%- set contract_enforced = contract_config.enforced and (not temporary) -%} + + {#- EXEC(), not a bare statement: CREATE VIEW above must be the first + statement in its batch, and a bare create referencing that just-made + view would fail compilation in the same batch. Deferring through EXEC + is what lets both live in one batch when the halves are concatenated. -#} + {%- set query -%} + {#- the drop guard is deliberately not applied on the contract branch, + which has never had one -#} + {%- if build_into_temp and not contract_enforced -%} + IF OBJECT_ID('{{ escape_single_quotes(relation.include(database=False)) }}', 'U') IS NOT NULL + EXEC('DROP TABLE {{ relation }}'); + {%- endif -%} + {{ sqlserver__get_create_table_empty_sql(relation, tmp_relation, sql, contract_enforced) }} {%- endset -%} + EXEC('{{- escape_single_quotes(query) -}}') +{%- endmacro %} + + +{% macro sqlserver__get_create_table_load_sql(temporary, relation, sql, drop_tmp_view=True) -%} + {#- + Second half of a table build: load the object the stage half created, + then clean up and add the clustered columnstore index. + + The INSERT takes an X table lock, never Sch-M, so it cannot block the + metadata readers #819 is about - which is why it is safe for this half to + run long, inside a transaction or not. + + The tmp view drop trails the INSERT here (the INSERT reads the view, so + it cannot go in the stage half). A caller whose load runs inside a + pre-hook's transaction passes drop_tmp_view=False and drops the view + after that commit: an uncommitted DROP VIEW blocks catalog scans like + an uncommitted CREATE (#819). A view left behind by a failed run is + harmless - the stage half drops it and uses CREATE OR ALTER. + -#} + {%- set query_label = get_query_options(parse_options=True) -%} + {%- set tmp_relation = relation.incorporate(path={"identifier": relation.identifier ~ '__dbt_tmp_vw'}, type='view') -%} {%- set contract_config = config.get('contract') -%} + {%- set contract_enforced = contract_config.enforced and (not temporary) -%} + + {#- EXEC() for the same batching reason as the stage half: concatenated, + this statement still follows CREATE VIEW inside one batch. -#} {%- set query -%} - {% if contract_config.enforced and (not temporary) %} - CREATE TABLE {{table_name}} - {{ get_assert_columns_equivalent(sql) }} - {{ build_columns_constraints(relation) }} - {% set listColumns %} - {% for column in model['columns'] %} - {{ adapter.quote(column) }}{{ ", " if not loop.last }} - {% endfor %} - {%endset%} - INSERT INTO {{relation}} WITH (TABLOCK) ({{listColumns}}) - SELECT {{listColumns}} FROM {{tmp_relation}} {{ query_label }} - - {% else %} - {%- if build_into_temp -%} - IF OBJECT_ID('{{ escape_single_quotes(relation.include(database=False)) }}', 'U') IS NOT NULL - EXEC('DROP TABLE {{ relation }}'); - {%- endif -%} - SELECT * INTO {{ table_name }} FROM {{ tmp_relation }} {{ query_label }} - {% endif %} + {{ sqlserver__get_tablock_insert_sql(relation, tmp_relation, query_label, contract_enforced) }} {%- endset -%} EXEC('{{- escape_single_quotes(query) -}}') + {% if drop_tmp_view %} {# For some reason drop_relation is not firing. This solves the issue for now. #} EXEC('DROP VIEW IF EXISTS {{ tmp_relation.include(database=False) }}') - - + {% endif %} {% set as_columnstore = config.get('as_columnstore', default=true) %} {% if not temporary and as_columnstore -%} @@ -70,7 +193,22 @@ -#} {{ sqlserver__create_clustered_columnstore_index(relation) }} {% endif %} +{%- endmacro %} + +{% macro sqlserver__create_table_as(temporary, relation, sql) -%} + {#- + One-batch table build: the stage and load halves back to back. + + The halves are the source of truth; this is their concatenation, so + callers that run the whole thing as a single statement (snapshots, + the incremental temp build) get the same statements in the same order + as before the split. Callers that need a transaction boundary between + creating the object and loading it call the halves directly - see + sqlserver__get_create_table_stage_sql for why that matters (#819). + -#} + {{ sqlserver__get_create_table_stage_sql(temporary, relation, sql) }} + {{ sqlserver__get_create_table_load_sql(temporary, relation, sql) }} {% endmacro %} @@ -131,16 +269,11 @@ IF OBJECT_ID('{{ escape_single_quotes(relation.include(database=False)) }}', 'U') IS NOT NULL EXEC('DROP TABLE {{ relation }}'); - {% if contract_enforced %} - {%- set ddl_query -%} - CREATE TABLE {{ relation }} - {{ get_assert_columns_equivalent(sql) }} - {{ build_columns_constraints(relation) }} - {%- endset -%} - EXEC('{{- escape_single_quotes(ddl_query) -}}') - {% else %} - EXEC('SELECT TOP 0 * INTO {{ relation }} FROM {{ tmp_relation }}') - {% endif %} + {#- escape_single_quotes now covers both branches: the empty create is + built from rendered relation names, and an identifier carrying a + single quote would otherwise break out of the EXEC literal -#} + {%- set ddl_query = sqlserver__get_create_table_empty_sql(relation, tmp_relation, sql, contract_enforced) -%} + EXEC('{{- escape_single_quotes(ddl_query) -}}') {# mark the rebuild in progress; removed atomically with the load below #} EXEC sp_addextendedproperty @name = N'dbt_full_refresh_incomplete', @value = '1', @@ -151,13 +284,22 @@ {{ setup_sql }} {%- endcall %} - {#- Commit the marker onto its own, then reopen so the load below (and - the rest of the materialization: grants, persist_docs, its own - trailing adapter.commit()) is unaffected. See adapter.commit_if_open / - begin_if_closed; 'main' above always opens a transaction (default - auto_begin), so this always actually commits here. -#} + {#- Commit the marker on its own ('main' above always opened a transaction, + so this always commits - taking any transaction: true pre-hook with + it, which is why build scope cannot deliver rollback here). The load + below then runs AUTOCOMMITTED: + + commit marker + |- CREATE clustered design on the empty table Sch-M ends with the statement + |- EXEC('BEGIN TRAN; INSERT WITH (TABLOCK); drop marker; COMMIT') + | a real transaction: load and + | unmark stay atomic (#718) + |- DROP VIEW + begin_if_closed masks, post-hooks, trailing commit + + Reopened before the load instead, that Sch-M sat on the LIVE name + until the materialization's trailing commit (#819). -#} {{ adapter.commit_if_open() }} - {{ adapter.begin_if_closed() }} {%- set load_sql -%} {% if as_columnstore %} @@ -166,22 +308,7 @@ {{ sqlserver__get_create_index_sql(relation, prebuilt_ns.clustered_dict) }} {% endif %} - {%- if contract_enforced -%} - {%- set listColumns -%} - {%- for column in model['columns'] -%} - {{ adapter.quote(column) }}{{ ", " if not loop.last }} - {%- endfor -%} - {%- endset -%} - {%- set insert_statement -%} - INSERT INTO {{ relation }} WITH (TABLOCK) ({{ listColumns }}) - SELECT {{ listColumns }} FROM {{ tmp_relation }} {{ query_label }} - {%- endset -%} - {%- else -%} - {%- set insert_statement -%} - INSERT INTO {{ relation }} WITH (TABLOCK) - SELECT * FROM {{ tmp_relation }} {{ query_label }} - {%- endset -%} - {%- endif %} + {%- set insert_statement = sqlserver__get_tablock_insert_sql(relation, tmp_relation, query_label, contract_enforced) %} {#- load and unmark atomically: any failure rolls back and keeps the marker. Relies on the session-level SET XACT_ABORT ON applied at connection open (see #718) so a run-time error here rolls back instead of @@ -199,9 +326,10 @@ EXEC('DROP VIEW IF EXISTS {{ tmp_relation.include(database=False) }}') {%- endset %} - {% call statement('create_table_as_prebuilt_load') -%} + {% call statement('create_table_as_prebuilt_load', auto_begin=False) -%} {{ load_sql }} {%- endcall %} + {{ adapter.begin_if_closed() }} {% endmacro %} @@ -217,19 +345,13 @@ ~ " @level0type = N'SCHEMA', @level0name = N'" ~ relation.schema ~ "'," ~ " @level1type = N'TABLE', @level1name = N'" ~ relation.identifier ~ "'" ) %} - {#- This marker exists to survive a failed rebuild, so it must not ride on - the same transaction as the rebuild it's guarding: if a prior - statement this run (e.g. a pre-hook) already opened the ambient - dbt-managed transaction, a later failure would roll the marker back - right along with it, defeating the point. Commit it, then reopen so - later code sees a transaction open exactly as it would without this - pair (begin_if_closed always leaves one open, whether or not - commit_if_open just found one to close) - commit_if_open alone is a - no-op when run_query above ran standalone, i.e. the common case of - this being the first statement of the run. See adapter.commit_if_open - / begin_if_closed. -#} + {#- The marker exists to survive a failed rebuild, so it must not share a + transaction with it: commit it now (a no-op when run_query ran + standalone). Deliberately NOT reopened - the load that follows must + autocommit so the new table's Sch-M is not held to the trailing + commit (#819); later statements open their own, and the tail states + its precondition with begin_if_closed before adapter.commit(). -#} {{ adapter.commit_if_open() }} - {{ adapter.begin_if_closed() }} {%- endmacro %} diff --git a/docs/transaction_scope.md b/docs/transaction_scope.md new file mode 100644 index 00000000..ffbde4a7 --- /dev/null +++ b/docs/transaction_scope.md @@ -0,0 +1,258 @@ +# Transaction scope and lock behaviour + +This page describes how the SQL Server adapter scopes transactions around a +`table`, `incremental` or `snapshot` build, why the boundaries sit where they do, and +the one knob that moves them — the `pre_hook_transaction_scope` model config. + +Background: [dbt-msft/dbt-sqlserver#819](https://github.com/dbt-msft/dbt-sqlserver/issues/819). + +## The problem + +SQL Server holds a statement's locks until the enclosing transaction commits, +not until the statement finishes. `Sch-M` (schema modification) is the one lock +mode incompatible with `Sch-S`, which *every* metadata reader takes — including +database-wide `sys` / `INFORMATION_SCHEMA` scans, a second dbt run's catalog +lookups, and an SSMS object explorer refresh. None of those asked for your +table by name. + +So a `CREATE` that shares a transaction with the load that follows it blocks +every metadata reader in every other session for the whole load. Measured +against SQL Server 2022, with an open transaction in one session and a +`sys.tables` scan in another: + +| Uncommitted statement | Catalog scan in another session | +|---|---| +| `SELECT * INTO` (fused create and load) | blocked | +| `SELECT TOP 0 * INTO` (empty create) | blocked | +| committed create, then `INSERT ... WITH (TABLOCK)` | **not blocked** | +| `CREATE CLUSTERED COLUMNSTORE INDEX` | blocked | +| `DROP VIEW` | blocked | +| `sp_rename` | blocked | +| `CREATE NONCLUSTERED INDEX` | blocked | + +The load itself is harmless inside a transaction. Everything DDL-shaped that +shares a transaction with it is the problem. + +### Before + +``` +BEGIN ← first in-tx pre-hook statement (or the build itself) +│ +├─ in-tx pre-hooks +├─ CREATE VIEW model__dbt_tmp_vw +├─ SELECT * INTO model__dbt_tmp ← Sch-M on intermediate, held from here +├─ CREATE CLUSTERED COLUMNSTORE INDEX ← Sch-M, also long +├─ sp_rename target → backup ← Sch-M on the live name +├─ sp_rename intermediate → target +├─ apply_masks ← ALTER on the live target +├─ create_indexes ← Sch-M on the live target, long +├─ in-tx post-hooks +├─ grants / denies / persist_docs +COMMIT ← every lock above released here +``` + +Two separate windows, both spanning slow work: the intermediate's `Sch-M` +across the load and the columnstore build, and the live target's `Sch-M` across +the index builds. + +### After + +``` + ├─ outside-tx pre-hooks autocommit + ├─ CREATE OR ALTER VIEW model__dbt_tmp_vw autocommit + ├─ SELECT TOP 0 * INTO model__dbt_tmp autocommit; Sch-M for an instant + │ +BEGIN ← first in-tx pre-hook statement, if any +├─ in-tx pre-hooks +├─ INSERT INTO model__dbt_tmp WITH (TABLOCK) ← X table lock, never Sch-M +├─ CREATE CLUSTERED COLUMNSTORE INDEX ← on a private name (see residual window) +├─ sp_rename target → backup ← Sch-M on the live name +├─ sp_rename intermediate → target +├─ apply_masks ← inside, deliberately (see below) +├─ in-tx post-hooks +COMMIT ← the cutover is atomic; Sch-M released + │ + ├─ DROP VIEW model__dbt_tmp_vw + ├─ create_indexes ← outside; no Sch-M on a live name held to commit + ├─ grants / denies / persist_docs + ├─ drop backup, outside-tx post-hooks +``` + +With no in-transaction pre-hook, `BEGIN` happens at the first `sp_rename` +instead: the load and the columnstore build then autocommit too, and no +`Sch-M` is held for longer than a statement anywhere before the cutover. + +`INSERT ... WITH (TABLOCK)` takes an exclusive *table* lock, which is compatible +with `Sch-S`, so the long load never blocks a metadata reader. The hint is what +keeps the load minimally logged — do not remove it to "reduce blocking". + +The same shape applies to `incremental` on its first build, on `--full-refresh`, +and on the append/merge path, where the `__dbt_temp` build is staged ahead of +the hooks the same way; and to `snapshot`, whose first build goes through the +intermediate and is renamed into place, and whose staging table on later runs +is staged ahead of the hooks and merged inside the transaction. +`table_refresh_method: dml` stages its scratch table ahead of the hooks and +swaps with `DELETE` + `INSERT` inside the transaction. + +## What is atomic with what + +The transaction spans **in-transaction pre-hooks → the load → the cutover → +in-transaction post-hooks**. That is what a hook declaring `transaction: true` +is asking for: atomicity with *the model*. A `transaction: true` pre-hook's +writes roll back with a failed load, exactly as before — on every path except +the two that commit a `dbt_full_refresh_incomplete` marker before the load +(`full_refresh_build: prebuilt`, and an incremental `--full-refresh` of an +existing table). The marker exists to survive a failed rebuild, so it cannot +share the load's transaction, and committing it commits the pre-hook with it. +Neither scope changes that; see [Where the setting is +inert](#pre_hook_transaction_scope). Index reconciliation, +grants, denies and `persist_docs` are the adapter's own housekeeping and were +never part of that promise, so they now run outside it. + +**Masks are the exception.** On a path that builds a brand-new table (the +default rename swap, `full_refresh_build: prebuilt`, an incremental first build +or `--full-refresh`, a snapshot's first build, and the DML fallback), the table carries no masks until +`apply_masks` runs. If that ran after the cutover committed, a mask failure +would leave the newly loaded table live with its columns exposed. Masks +therefore stay inside the transaction on those paths. The ALTERs are cheap next +to an index build. + +On the rename swap and the DML fallback a mask failure rolls the swap back, so +the old, masked table keeps serving. `full_refresh_build: prebuilt` has no swap +to roll back — it drops the target and rebuilds in place — so a mask failure +there leaves a loaded target carrying the `dbt_full_refresh_incomplete` marker, +which blocks normal runs until a `--full-refresh` succeeds. That is the +trade-off `prebuilt` already makes. + +On the persisted-table paths (`table_refresh_method: dml` swap, incremental +append, snapshot merge) the table already carries its masks, so reconciliation runs outside — a +failure there leaves the previous masks in place, exposing nothing. + +## Post-hook ordering changed + +In-transaction post-hooks now run **before** index creation, where they +previously ran after, on `table`, `incremental` and `snapshot`. Masks on fresh tables +are unaffected — they still run before the post-hooks, for the reason in the +previous section. Post-hooks already ran before grants, denies and +`persist_docs` on `table`; on `incremental` and `snapshot` those three moved +after the post-hooks too, so the three materializations now share one tail. + +If a post-hook needs the indexes to exist — it queries the table at scale, or +creates an index of its own — declare it `transaction: false`. That slot runs +after the entire tail: + +```yaml +post_hook: + - sql: "update {{ this }} set ... " + transaction: false +``` + +Two consequences worth knowing if you manage indexes through post-hooks (the +idiom that predates the `indexes` config): + +- With `drop_unmanaged_indexes: true`, an index created by an in-transaction + post-hook on a persisted-table path is now dropped by the same run's + reconciliation. Move it to the `indexes` config. +- An index created by an in-transaction post-hook on a column in your `masks` + config will trip the index-key check on SQL Server versions before 2022, + where previously the ordering happened to avoid it. + +## `pre_hook_transaction_scope` + +Schema resolution — the tmp view, and under an enforced contract the describe +probe — needs the model SQL to *bind*: every object it references must exist. +Running it before the in-transaction pre-hooks is what lets it autocommit, so +that is also the one thing it cannot do: bind against an object a +`transaction: true` pre-hook is about to create. + +| Value | Schema resolution runs | Transaction covers | Pre-hook rolls back with a failed load | #819 fixed | +|---|---|---|---|---| +| `load` (default) | before the in-tx pre-hooks, autocommitted | pre-hooks + load + cutover + post-hooks | yes¹ | yes | +| `build` | inside the pre-hooks' transaction | pre-hooks + create + load + cutover + post-hooks | yes¹ | no | + +¹ Except on the two marker-committing paths described under *Where the setting +is inert* below, where neither scope can deliver rollback. + +Under `load`, a `transaction: true` pre-hook that creates what the model reads +fails at the stage with `Invalid object name`, before any hook has run. Staging +early fixes the intermediate's *column shape*, not just its existence, so the +same ordering shows up in a second, less obvious form: a `transaction: true` +pre-hook that widens or adds a column on a source the model already references +lands a stage built from the old shape, and the load that follows fails with a +truncation or column-count error rather than a clear `Invalid object name`. +Same cause, same two remedies: + +- Declare that hook `transaction: false`. Outside-transaction pre-hooks run + before the stage, so the object exists when the view binds. A staging-table + refresh is rarely something you want rolled back anyway. +- Set `build` on the model. The create then runs after the hooks, inside their + transaction, and holds the new table's `Sch-M` for the whole load — today's + behaviour, for that model only. + +```yaml +models: + my_project: + staging: + +pre_hook_transaction_scope: build +``` +```jinja +{{ config(pre_hook_transaction_scope='build') }} +``` + +Note that a view already cannot reference a `#temp` table, so a pre-hook that +stages what the model reads only ever worked with a permanent object; hooks +that truncate, disable indexes, write audit rows or refresh a source the model +already references bind fine under `load`. + +**Where the setting is inert.** `full_refresh_build: prebuilt` builds in place: +its setup has to run after the hooks (a hook may read `{{ this }}` before the +rebuild drops it), and it commits its in-progress marker — and with it any +`transaction: true` pre-hook — before the load, precisely so the marker +survives a failure. An incremental `--full-refresh` of an existing table marks +it in progress the same way. On both, the pre-hook is durable by the time the +load runs whatever this is set to; use `transaction: false` and handle the +rollback yourself if that matters. The load itself is autocommitted on both, +so neither holds `Sch-M` across it. + +**With no transactional pre-hook** the setting changes nothing: the stage +autocommits either way, and the load autocommits too. `build` means "join the +transaction the hooks opened", not "open one" — with no hook to open it there +is nothing to join, so a folder-level `+pre_hook_transaction_scope: build` +costs nothing on the models underneath it that declare no hooks. Note that +`transaction: true` is dbt's *default* for a pre-hook, so a plain string +pre-hook is a transactional one. + +## Residual window + +`table_refresh_method: dml` keeps one window of its own, on the branch that +detects a schema change: that branch falls back to a full rebuild +(`create_table_as`, a fused create and load) and runs it inside the swap's +transaction, so the scratch table's `Sch-M` is held across that load. It is a +private name, visible to database-wide catalog scans, and it predates this +change — the steady-state `DELETE` + `INSERT` path, which is what a `dml` model +takes on every run where the column shape is unchanged, has no such window. + +With a `transaction: true` pre-hook and `as_columnstore: true` (the default), +the clustered columnstore index is built on the intermediate inside the hook's +transaction, so its `Sch-M` — on a private name, but visible to database-wide +catalog scans — is held from the end of the columnstore build to the cutover +commit. That is bounded by the index build and the cutover, not the load. With +no transactional pre-hook the columnstore build autocommits and there is no +window. Closing it entirely would mean creating the columnstore index on the +empty table and bulk-loading into it, which is the `prebuilt` trade. + +## Caveats + +- `apply_grants` opens a transaction of its own (dbt's `call_dcl_statements` + uses the default `auto_begin`), so grants, denies and `persist_docs` still run + inside one. They are short DCL statements and take no `Sch-M` on the target. +- An `online` or `resumable` index build runs outside any transaction by + necessity, and the adapter closes the transaction that leaves open before + continuing. +- Index and mask reconciliation running outside the cutover means a failure + there leaves the new data committed with indexes not yet converged. Both + reconcile against the config rather than applying a delta, so the next run + converges them. +- A failed load under `load` leaves the empty `__dbt_tmp` intermediate and its + tmp view behind, since both committed before the hook opened the transaction. + The next run drops them before it starts. diff --git a/tests/functional/adapter/dbt/test_constraints.py b/tests/functional/adapter/dbt/test_constraints.py index e628805e..4d8884f7 100644 --- a/tests/functional/adapter/dbt/test_constraints.py +++ b/tests/functional/adapter/dbt/test_constraints.py @@ -473,7 +473,7 @@ def models(self): @pytest.fixture(scope="class") def expected_sql(self): return """ - EXEC(' CREATE OR ALTER VIEW AS -- depends_on: select ''blue'' as color, 1 as id, ''2019-01-01'' as date_day; ') EXEC(' CREATE TABLE ( id int not null , color varchar(100), date_day varchar(100) ) INSERT INTO WITH (TABLOCK) ( "id", "color", "date_day" ) SELECT "id", "color", "date_day" FROM ') EXEC('DROP VIEW IF EXISTS + EXEC(' CREATE OR ALTER VIEW AS -- depends_on: select ''blue'' as color, 1 as id, ''2019-01-01'' as date_day; ') EXEC('CREATE TABLE ( id INT not null , color varchar(100), date_day varchar(100) ) ') EXEC('INSERT INTO WITH (TABLOCK) ("id", "color", "date_day") SELECT "id", "color", "date_day" FROM ') """ # EXEC('DROP view IF EXISTS @@ -593,7 +593,7 @@ def models(self): @pytest.fixture(scope="class") def expected_sql(self): return """ - EXEC(' CREATE OR ALTER VIEW AS -- depends_on: select ''blue'' as color, 1 as id, ''2019-01-01'' as date_day; ') EXEC(' CREATE TABLE ( id int not null , color varchar(100), date_day varchar(100) ) INSERT INTO WITH (TABLOCK) ( "id", "color", "date_day" ) SELECT "id", "color", "date_day" FROM ') EXEC('DROP VIEW IF EXISTS + EXEC(' CREATE OR ALTER VIEW AS -- depends_on: select ''blue'' as color, 1 as id, ''2019-01-01'' as date_day; ') EXEC('CREATE TABLE ( id INT not null, color varchar(100), date_day varchar(100) ) ') EXEC('INSERT INTO WITH (TABLOCK) ("id", "color", "date_day") SELECT "id", "color", "date_day" FROM ') """ def test__model_constraints_ddl(self, project, expected_sql): diff --git a/tests/functional/adapter/mssql/test_failed_load_leftovers.py b/tests/functional/adapter/mssql/test_failed_load_leftovers.py new file mode 100644 index 00000000..04dba9dc --- /dev/null +++ b/tests/functional/adapter/mssql/test_failed_load_leftovers.py @@ -0,0 +1,135 @@ +"""A failed load leaves the staged objects behind; the next run must clear them. + +Under pre_hook_transaction_scope='load' (the default) schema resolution +commits before the load, so a load that fails - or anything after it that +rolls the transaction back - leaves the empty intermediate and its tmp view in +the database. Three mechanisms clean that up on the next run, and these tests +prove each path actually recovers rather than tripping over its own leftovers: + + - the materialization drops the cached preexisting intermediate up front, + - the stage batch carries an OBJECT_ID guard for adapter-generated + throwaways, so a stale relation cache cannot make it hit Msg 2714, and + - the tmp view is dropped by name and re-created with CREATE OR ALTER. + +The dml path drops both of its scratch objects by name before staging. +""" + +import pytest + +from dbt.tests.util import run_dbt + +# `bad: true` puts an unparseable row in the source; the models cast it, so +# the load fails after the empty create has committed. +source_rows_sql = """ +{{ config(materialized='table', as_columnstore=False) }} +select 1 as id, cast('10' as varchar(20)) as txt +union all +select 2 as id, cast({{ "'oops'" if var('bad', true) else "'20'" }} as varchar(20)) as txt +""" + +rename_model_sql = """ +{{ config(materialized='table', as_columnstore=False) }} +select id, cast(txt as int) as val from {{ ref('source_rows') }} +""" + +dml_model_sql = """ +{{ config(materialized='table', as_columnstore=False, table_refresh_method='dml') }} +select id, cast(txt as int) as val from {{ ref('source_rows') }} +""" + +snapshot_sql = """ +{% snapshot snap %} +{{ config(unique_key='id', strategy='check', check_cols='all') }} +select id, cast(txt as int) as val from {{ ref('source_rows') }} +{% endsnapshot %} +""" + + +def _object_exists(project, name): + return ( + project.run_sql(f"select object_id('{project.test_schema}.{name}')", fetch="one")[0] + is not None + ) + + +class TestTableRenamePathRecovers: + @pytest.fixture(scope="class") + def models(self): + return {"source_rows.sql": source_rows_sql, "rename_model.sql": rename_model_sql} + + def test_rerun_clears_leftovers(self, project): + results = run_dbt(["run"], expect_pass=False) + assert {r.node.name: r.status for r in results}["rename_model"] == "error" + assert _object_exists(project, "rename_model__dbt_tmp") + assert _object_exists(project, "rename_model__dbt_tmp__dbt_tmp_vw") + assert not _object_exists(project, "rename_model") + + results = run_dbt(["run", "--vars", "bad: false"]) + assert all(r.status == "success" for r in results) + assert not _object_exists(project, "rename_model__dbt_tmp") + assert not _object_exists(project, "rename_model__dbt_tmp__dbt_tmp_vw") + assert not _object_exists(project, "rename_model__dbt_backup") + rows = project.run_sql( + f"select count(*) from {project.test_schema}.rename_model", fetch="one" + )[0] + assert rows == 2 + + +class TestDmlRefreshPathRecovers: + @pytest.fixture(scope="class") + def models(self): + return {"source_rows.sql": source_rows_sql, "dml_model.sql": dml_model_sql} + + def test_rerun_clears_leftovers(self, project): + # the dml path only applies once the target exists + run_dbt(["run", "--vars", "bad: false"]) + assert _object_exists(project, "dml_model") + + results = run_dbt(["run"], expect_pass=False) + assert {r.node.name: r.status for r in results}["dml_model"] == "error" + assert _object_exists(project, "dml_model__dbt_refresh") + assert _object_exists(project, "dml_model__dbt_refresh__dbt_tmp_vw") + # the target was never touched: the failure was in the scratch load + rows = project.run_sql( + f"select count(*) from {project.test_schema}.dml_model", fetch="one" + )[0] + assert rows == 2 + + results = run_dbt(["run", "--vars", "bad: false"]) + assert all(r.status == "success" for r in results) + assert not _object_exists(project, "dml_model__dbt_refresh") + assert not _object_exists(project, "dml_model__dbt_refresh__dbt_tmp_vw") + + +class TestSnapshotFirstBuildRecovers: + @pytest.fixture(scope="class") + def models(self): + return {"source_rows.sql": source_rows_sql} + + @pytest.fixture(scope="class") + def snapshots(self): + return {"snap.sql": snapshot_sql} + + def test_rerun_clears_leftovers(self, project): + run_dbt(["run"]) + results = run_dbt(["snapshot"], expect_pass=False) + assert results[0].status == "error" + assert _object_exists(project, "snap__dbt_tmp") + assert _object_exists(project, "snap__dbt_tmp__dbt_tmp_vw") + assert _object_exists(project, "snap_snapshot_staging_temp_view") + assert not _object_exists(project, "snap") + + run_dbt(["run", "--vars", "bad: false"]) + results = run_dbt(["snapshot"]) + assert results[0].status == "success" + assert _object_exists(project, "snap") + assert not _object_exists(project, "snap__dbt_tmp") + assert not _object_exists(project, "snap__dbt_tmp__dbt_tmp_vw") + assert not _object_exists(project, "snap_snapshot_staging_temp_view") + + # and a second snapshot run (the merge path) leaves no staging objects + results = run_dbt(["snapshot"]) + assert results[0].status == "success" + assert not _object_exists(project, "snap__dbt_temp") + assert not _object_exists(project, "snap__dbt_temp__dbt_tmp_vw") + assert not _object_exists(project, "snap_snapshot_staging_temp_view") diff --git a/tests/functional/adapter/mssql/test_incremental_failed_first_build.py b/tests/functional/adapter/mssql/test_incremental_failed_first_build.py new file mode 100644 index 00000000..bbb321c0 --- /dev/null +++ b/tests/functional/adapter/mssql/test_incremental_failed_first_build.py @@ -0,0 +1,141 @@ +"""A failed first build of an incremental model must not leave a target behind. + +The fresh-create branch of the incremental materialization builds straight +into ``target_relation`` — no ``__dbt_tmp`` intermediate, so no rename swap and +no ``OBJECT_ID`` drop guard (that guard only covers adapter-generated +throwaways). Since #819 split the build into an empty ``CREATE`` followed by a +separate ``INSERT ... WITH (TABLOCK)``, and the build batch declines to open +the ambient transaction, the two statements autocommit independently: a load +that fails leaves the empty table committed under the model's real name. + +That is silently destructive rather than merely untidy. dbt's next run sees a +relation that exists and is not a view, so it takes the append/merge branch and +merges that run's window into an empty table. Nothing errors, and every row the +first build should have loaded is gone for good. + +The invariant under test is therefore: after a failed first build, the target +does not exist — leaving dbt to do a fresh create next time, which is correct. +""" + +import os + +import pytest + +from dbt.tests.util import run_dbt + +# Rows come from a table rather than inline VALUES on purpose: the empty create +# is `SELECT TOP 0 * INTO ... FROM `, and constant-folded literals could +# let the failing CAST evaluate at create time. Reading from storage guarantees +# TOP 0 touches no rows, so the create succeeds and only the INSERT fails. +source_rows_sql = """ +{{ config(materialized='table', as_columnstore=False) }} +select 1 as id, cast('10' as varchar(20)) as txt +union all +select 2 as id, cast('not_a_number' as varchar(20)) as txt +""" + +# CAST fails on row 2 during the load, never during schema resolution. +failing_incremental_sql = """ +{{ config(materialized='incremental', unique_key='id', as_columnstore=False) }} +select id, cast(txt as int) as val +from {{ ref('source_rows') }} +""" + + +class TestFailedFirstIncrementalBuildLeavesNoTarget: + @pytest.fixture(scope="class") + def models(self): + return { + "source_rows.sql": source_rows_sql, + "failing_incremental.sql": failing_incremental_sql, + } + + def test_failed_first_build_leaves_no_target(self, project): + results = run_dbt(["run"], expect_pass=False) + + statuses = {r.node.name: r.status for r in results} + assert statuses["source_rows"] == "success" + assert statuses["failing_incremental"] == "error" + + object_id = project.run_sql( + f"select OBJECT_ID('{project.test_schema}.failing_incremental')", + fetch="one", + )[0] + assert object_id is None, ( + "the failed load committed an empty table under the model's real " + "name; dbt's next run will take the append branch and merge into " + "it, silently losing every row the first build should have loaded" + ) + + +# The corrected model: same shape, no unconvertible row. +fixed_incremental_sql = """ +{{ config(materialized='incremental', unique_key='id', as_columnstore=False) }} +select id, cast(txt as int) as val +from {{ ref('source_rows') }} +where txt <> 'not_a_number' +""" + +clean_incremental_sql = """ +{{ config(materialized='incremental', unique_key='id', as_columnstore=False) }} +select 1 as id, 10 as val +""" + + +class TestRecoveryAfterFailedFirstBuild: + """The run after a failed first build must load everything, not a window.""" + + @pytest.fixture(scope="class") + def models(self): + return { + "source_rows.sql": source_rows_sql, + "failing_incremental.sql": failing_incremental_sql, + } + + def test_rerun_after_failure_loads_all_rows(self, project): + run_dbt(["run"], expect_pass=False) + + # Repair the model and run again. If the failed build had left an empty + # target behind, this run would take the append branch and merge into + # it; the row it should have loaded from the first build would be lost. + path = os.path.join(project.project_root, "models", "failing_incremental.sql") + with open(path, "w") as handle: + handle.write(fixed_incremental_sql) + + run_dbt(["run"]) + + rows = project.run_sql( + f"select id, val from {project.test_schema}.failing_incremental order by id", + fetch="all", + ) + assert [tuple(row) for row in rows] == [(1, 10)] + + +class TestFirstIncrementalBuildStillSucceeds: + """Guards the swap's rename guard. + + The fresh-create branch now swaps, and the swap's target->backup rename is + only correct when a target exists. Without the guard this is sp_rename + against a missing name (Msg 15225) on the first build of every incremental + model - so a plain happy-path build is the regression test. + """ + + @pytest.fixture(scope="class") + def models(self): + return {"clean_incremental.sql": clean_incremental_sql} + + def test_first_build_and_rerun(self, project): + results = run_dbt(["run"]) + assert len(results) == 1 + + rows = project.run_sql( + f"select id, val from {project.test_schema}.clean_incremental", fetch="all" + ) + assert [tuple(row) for row in rows] == [(1, 10)] + + # Second run takes the append branch against the swapped-in target. + run_dbt(["run"]) + rows = project.run_sql( + f"select count(*) from {project.test_schema}.clean_incremental", fetch="one" + ) + assert rows[0] == 1 diff --git a/tests/functional/adapter/mssql/test_interaction_tablock_query_options.py b/tests/functional/adapter/mssql/test_interaction_tablock_query_options.py index 9ba7be47..af00e5f8 100644 --- a/tests/functional/adapter/mssql/test_interaction_tablock_query_options.py +++ b/tests/functional/adapter/mssql/test_interaction_tablock_query_options.py @@ -7,8 +7,11 @@ The test lives on a dedicated branch that merges both feature branches because neither individual branch can exercise the combined output: #640 alone ignores -query_options config, and #613 alone uses the non-contract `SELECT * INTO` -path which has no TABLOCK to emit. +query_options config, and #613 alone used the non-contract `SELECT * INTO` +path, which had no TABLOCK to emit. + +Since #819 the non-contract path is a split create-then-load too, so it emits +TABLOCK as well; the second class below covers it. """ import os @@ -63,3 +66,33 @@ def test_both_hints_in_compiled_sql(self, project): assert "MAXDOP 1" in sql # The default LABEL is always present. assert "LABEL =" in sql + + +def _run_sql(project, model_name): + target_dir = os.path.join(project.project_root, "target", "run") + for root, _dirs, files in os.walk(target_dir): + if f"{model_name}.sql" in files: + with open(os.path.join(root, f"{model_name}.sql")) as f: + return f.read() + raise AssertionError(f"Could not find compiled {model_name}.sql") + + +class TestNonContractTableWithQueryOptions: + """#819 split the non-contract build too, so it emits TABLOCK and the + OPTION clause on the same statement - the create moves no rows and carries + neither.""" + + @pytest.fixture(scope="class") + def models(self): + return {"plain_with_options.sql": model_sql} + + def test_split_build_keeps_both_hints(self, project): + results = run_dbt(["run"]) + assert results[0].status == "success" + + sql = _run_sql(project, "plain_with_options") + assert "SELECT TOP 0 * INTO" in sql + assert "WITH (TABLOCK)" in sql + assert "MAXDOP 1" in sql + # The fused form is what #819 removed. + assert "SELECT * INTO" not in sql diff --git a/tests/functional/adapter/mssql/test_pre_hook_transaction_scope.py b/tests/functional/adapter/mssql/test_pre_hook_transaction_scope.py new file mode 100644 index 00000000..3919edff --- /dev/null +++ b/tests/functional/adapter/mssql/test_pre_hook_transaction_scope.py @@ -0,0 +1,444 @@ +"""pre_hook_transaction_scope: where schema resolution runs relative to in-transaction pre-hooks. + + 'load' (default) - the tmp view and the empty CREATE run before the in-tx + pre-hooks and autocommit; the load joins the hook's + transaction. No Sch-M spans the load, and the pre-hook + still rolls back with a failed load. + 'build' - the create runs inside the hook's transaction, after + it, so its Sch-M is held for the whole load. Today's + behaviour, kept for a pre-hook that creates what the + model reads. + +Three things are observable from a dbt test and pinned here: + 1. rollback - a transaction: true pre-hook's write is gone after a failed + load under BOTH scopes (the two differ in locks, not in atomicity). + 2. locks - while the load runs, the building session holds no object-level + Sch-M under 'load' and holds one under 'build'. Sch-M is the mode that + blocks the Sch-S every metadata reader takes, so that is the whole of + #819. + 3. bindability - a transaction: true pre-hook that creates the model's + source fails at the stage under 'load', works under 'build', and works + under 'load' once declared transaction: false. + 4. ordering - the snapshot materialization's own database work (the check + strategy runs the snapshot SQL to compare column shapes) still happens + after the pre-hooks each scope promises to have run by then. +""" + +import threading +import time + +import pytest + +from dbt.adapters.contracts.connection import Connection +from dbt.adapters.sqlserver.sqlserver_connections import SQLServerConnectionManager +from dbt.tests.util import run_dbt + +audit_log_sql = """ +{{ config(materialized='table', as_columnstore=False) }} +select cast(0 as int) as marker where 1 = 0 +""" + +# Rows come from a table, not inline literals: the empty create is +# SELECT TOP 0, and constant folding could otherwise evaluate the failing CAST +# at create time rather than during the load. +source_rows_sql = """ +{{ config(materialized='table', as_columnstore=False) }} +select 1 as id, cast('not_a_number' as varchar(20)) as txt +""" + + +def _failing_model(scope): + return f""" +{{{{ config( + materialized='table', as_columnstore=False, + pre_hook_transaction_scope='{scope}', + pre_hook=[{{'sql': "insert into {{{{ ref('audit_log') }}}} (marker) values (1)", + 'transaction': True}}] +) }}}} +select cast(txt as int) as val from {{{{ ref('source_rows') }}}} +""" + + +big_source_sql = """ +{{ config(materialized='table', as_columnstore=False) }} +select top 1500000 + row_number() over (order by (select null)) as id, + replicate('x', 40) as payload +from sys.all_columns a cross join sys.all_columns b cross join sys.all_columns c +""" + + +# hashbytes over a widened payload keeps the load in the seconds range, so the +# probe below samples it many times over +_slow_select = """ +select a.id, a.payload, v.n, hashbytes('SHA2_512', replicate(a.payload, 50)) as h +from {{ ref('big_source') }} a +cross join (values (1), (2), (3), (4)) v(n) +""" + + +def _slow_model(scope, pre_hook=True, materialized="table"): + hook = "pre_hook=[{'sql': \"select 1 as noop\", 'transaction': True}]," if pre_hook else "" + return f""" +{{{{ config( + materialized='{materialized}', as_columnstore=False, + pre_hook_transaction_scope='{scope}', + {hook} +) }}}} +{_slow_select} +""" + + +def _slow_snapshot(scope): + """First build of a snapshot: the stage, then the load - the path + sqlserver__snapshot_stage owns.""" + return f""" +{{% snapshot slow_snap %}} +{{{{ config( + unique_key='id', strategy='check', check_cols=['n'], + as_columnstore=False, + pre_hook_transaction_scope='{scope}' +) }}}} +{_slow_select} +{{% endsnapshot %}} +""" + + +def _staged_by_hook(scope, hook_tx): + return f""" +{{{{ config( + materialized='table', as_columnstore=False, + pre_hook_transaction_scope='{scope}', + pre_hook=[{{'sql': "drop table if exists {{{{ target.schema }}}}.hook_staged; " + "select 1 as id into {{{{ target.schema }}}}.hook_staged", + 'transaction': {hook_tx}}}] +) }}}} +select id from {{{{ target.schema }}}}.hook_staged +""" + + +# -- 1. rollback ------------------------------------------------------------ + + +class _RollbackCase: + @pytest.fixture(scope="class") + def models(self): + return { + "audit_log.sql": audit_log_sql, + "source_rows.sql": source_rows_sql, + "failing_model.sql": _failing_model(self.scope), + } + + def test_pre_hook_write_is_rolled_back(self, project): + run_dbt(["run"], expect_pass=False) + rows = project.run_sql( + f"select count(*) from {project.test_schema}.audit_log", fetch="one" + )[0] + assert rows == 0, ( + f"pre_hook_transaction_scope='{self.scope}' keeps the pre-hook in the " + "load's transaction, so a failed load must roll its write back" + ) + target = project.run_sql( + f"select object_id('{project.test_schema}.failing_model', 'U')", fetch="one" + )[0] + assert target is None, "a failed first build must leave no target behind" + + +class TestLoadScopeRollsBackThePreHook(_RollbackCase): + scope = "load" + + def test_stage_committed_on_its_own(self, project): + """Under 'load' the empty create is durable before the hook runs, so it + survives the rollback; the next run's preexisting-intermediate drop + clears it.""" + run_dbt(["run"], expect_pass=False) + tmp = project.run_sql( + f"select object_id('{project.test_schema}.failing_model__dbt_tmp', 'U')", fetch="one" + )[0] + assert tmp is not None + + +class TestBuildScopeRollsBackThePreHook(_RollbackCase): + scope = "build" + + +# -- 2. locks --------------------------------------------------------------- + + +# Sampled from a second connection while the model builds, once every 0.1s: +# +# is the session running the load request text names this schema + TABLOCK +# holding a blocking lock OBJECT / Sch-M / GRANT on that session +# +# DMVs take no lock on user objects, so this reads the building session +# directly instead of timing out a catalog scan from outside. That matters at +# `-n auto`: another worker's DDL blocks a sys.tables scan no matter what this +# model does, so a timing-out scan measures the suite, not the change. +# +# TABLOCK identifies the load half in both scopes (it is the only statement in +# the materialization that carries the hint) and under 'build' the fused batch +# carries the empty CREATE with it. `session_id <> @@spid` drops the probe's +# own request, whose text contains both literals. +_SAMPLE_SQL = """ +with loading as ( + select r.session_id + from sys.dm_exec_requests r + cross apply sys.dm_exec_sql_text(r.sql_handle) t + where r.session_id <> @@spid + and t.text like '%{schema}%' + and t.text like '%TABLOCK%' +) +select + (select count(*) from loading), + (select count(*) from loading + where exists (select 1 from sys.dm_tran_locks l + where l.request_session_id = loading.session_id + and l.resource_type = 'OBJECT' + and l.request_mode = 'Sch-M' + and l.request_status = 'GRANT')) +""" + + +def _probe_connection(project): + """A second session, opened through the adapter's own connect path so it + speaks whichever backend the profile names, but owned by this test rather + than by dbt's connection manager: run_dbt closes every connection that + manager knows about, which would pull this one out from under the probe + thread mid-query.""" + connection = Connection( + type="sqlserver", + name="sch_m_probe", + state="init", + transaction_open=False, + handle=None, + credentials=project.adapter.config.credentials, + ) + SQLServerConnectionManager.open(connection) + return connection.handle + + +def _probe(project, stop, samples, failures): + """Append True/False - Sch-M held or not - once per sample taken while the + load is running; ignore every sample taken when it is not.""" + try: + handle = _probe_connection(project) + sql = _SAMPLE_SQL.format(schema=project.test_schema) + try: + while not stop.is_set(): + cursor = handle.cursor() + try: + cursor.execute(sql) + loading, holding_sch_m = cursor.fetchone() + finally: + cursor.close() + if loading: + samples.append(bool(holding_sch_m)) + time.sleep(0.1) + finally: + handle.close() + except BaseException as e: # noqa: BLE001 - a probe that dies silently lies + failures.append(e) + + +class _LockCase: + pre_hook = True + materialized = "table" + + @pytest.fixture(scope="class") + def models(self): + return { + "big_source.sql": big_source_sql, + "slow_model.sql": _slow_model(self.scope, self.pre_hook, self.materialized), + } + + def _build_the_model(self): + return run_dbt(["run", "--select", "slow_model"]) + + def _sch_m_during_the_load(self, project): + # deliberately not a skip: this is the only functional guard on the + # lock #819 is about, and a silent skip on a login without the + # permission would retire it with no signal at all + assert project.run_sql( + "select has_perms_by_name(null, null, 'VIEW SERVER STATE')", fetch="one" + )[0], "these tests read sys.dm_exec_requests; grant the test login VIEW SERVER STATE" + + run_dbt(["run", "--select", "big_source"]) + + samples, failures = [], [] + stop = threading.Event() + probe = threading.Thread(target=_probe, args=(project, stop, samples, failures)) + probe.start() + try: + results = self._build_the_model() + finally: + stop.set() + probe.join() + + assert not failures, f"the lock probe failed: {failures[0]!r}" + assert results[0].status == "success" + assert len(samples) >= 5, "the load finished before the probe could sample it" + return samples + + +class _NoSchM(_LockCase): + def test_no_sch_m_during_the_load(self, project): + samples = self._sch_m_during_the_load(project) + held = samples.count(True) + assert held == 0, f"Sch-M held during {held} of {len(samples)} samples: {samples}" + + +class TestLoadScopeDoesNotBlockCatalogReaders(_NoSchM): + """load: the create committed before the load, and the INSERT takes an X + table lock, which no metadata reader conflicts with.""" + + scope = "load" + + +class TestBuildScopeWithoutAnInTxHookHoldsNothing(_NoSchM): + """build with no transactional pre-hook: there is no transaction to join, + so the create and the load autocommit exactly as under load. Pins what + docs/transaction_scope.md promises - a folder-level +build must not take + the lock for models underneath it that have no hooks.""" + + scope, pre_hook = "build", False + + +class TestIncrementalBuildScopeWithoutAnInTxHookHoldsNothing(_NoSchM): + """The same promise on incremental's fresh-build branch.""" + + scope, pre_hook, materialized = "build", False, "incremental" + + +class TestSnapshotBuildScopeWithoutAnInTxHookHoldsNothing(_NoSchM): + """And on a snapshot's first build, whose stage is sqlserver__snapshot_stage.""" + + scope, pre_hook = "build", False + + @pytest.fixture(scope="class") + def models(self): + return {"big_source.sql": big_source_sql} + + @pytest.fixture(scope="class") + def snapshots(self): + return {"slow_snap.sql": _slow_snapshot(self.scope)} + + def _build_the_model(self): + return run_dbt(["snapshot"]) + + +class TestBuildScopeBlocksCatalogReaders(_LockCase): + scope = "build" + + def test_sch_m_spans_the_load(self, project): + samples = self._sch_m_during_the_load(project) + # The create shares the pre-hook's transaction, so its Sch-M is held to + # commit - held CONTINUOUSLY from the create to the end of the load, + # which is what this asserts, rather than merely "held in most + # samples". The request is matched by its batch text, so it is visible + # for a moment at each end while no Sch-M is held yet - and the two + # DMV reads in one sample are not atomic, so the sample that lands on + # the commit can see the request still running with its locks already + # gone. One sample at each end is allowed to miss; none in between. + assert True in samples, f"Sch-M never held during the load: {samples}" + first = samples.index(True) + last = len(samples) - 1 - samples[::-1].index(True) + held = samples[first : last + 1] + assert all(held), f"Sch-M released mid-load: {samples}" + assert len(samples) - len(held) <= 2, f"Sch-M held for only part of the load: {samples}" + assert len(held) >= 5, f"too little of the load sampled: {samples}" + + +# -- 3. bindability --------------------------------------------------------- + + +class _StagedByHook: + @pytest.fixture(scope="class") + def models(self): + return {"staged.sql": _staged_by_hook(self.scope, self.hook_tx)} + + +class TestLoadScopeFailsWhenAnInTxHookStagesTheSource(_StagedByHook): + scope, hook_tx = "load", "True" + + def test_invalid_object_at_the_stage(self, project): + results = run_dbt(["run"], expect_pass=False) + assert "Invalid object name" in str(results[0].message) + + +class TestLoadScopeWorksWhenTheHookIsOutsideTheTransaction(_StagedByHook): + scope, hook_tx = "load", "False" + + def test_passes(self, project): + assert run_dbt(["run"])[0].status == "success" + + +class TestBuildScopeWorksWhenAnInTxHookStagesTheSource(_StagedByHook): + scope, hook_tx = "build", "True" + + def test_passes(self, project): + assert run_dbt(["run"])[0].status == "success" + + +class TestInvalidScopeIsRejected: + @pytest.fixture(scope="class") + def models(self): + return { + "bad_scope.sql": """ +{{ config(materialized='table', pre_hook_transaction_scope='schema') }} +select 1 as id +""" + } + + def test_invalid_value_raises(self, project): + results = run_dbt(["run"], expect_pass=False) + assert "pre_hook_transaction_scope" in str(results[0].message) + + +# -- 4. ordering ------------------------------------------------------------ + + +def _hook_sourced_snapshot(scope, hook_tx): + """A check-strategy snapshot whose source a pre-hook creates. + + The check strategy runs the snapshot's own SQL to compare column shapes + (snapshot_check_all_get_existing_columns), but only once the target + exists - so this binds trivially on the first run and only reaches the + strategy probe on the second. Both escape hatches from 'load' have to + survive that: transaction: false, and scope 'build'. + """ + return f""" +{{% snapshot hook_sourced_snap %}} +{{{{ config( + unique_key='id', strategy='check', check_cols='all', + pre_hook_transaction_scope='{scope}', + pre_hook=[{{'sql': "drop table if exists {{{{ target.schema }}}}.hook_sourced; " + "select 1 as id, cast('a' as varchar(10)) as txt " + "into {{{{ target.schema }}}}.hook_sourced", + 'transaction': {hook_tx}}}] +) }}}} +select id, txt from {{{{ target.schema }}}}.hook_sourced +{{% endsnapshot %}} +""" + + +class _HookSourcedSnapshot: + @pytest.fixture(scope="class") + def snapshots(self): + return {"hook_sourced_snap.sql": _hook_sourced_snapshot(self.scope, self.hook_tx)} + + def test_second_run_still_binds(self, project): + assert run_dbt(["snapshot"])[0].status == "success" + # the source belongs to the hook, so take it away again: otherwise the + # first run's copy is still there and the strategy probe binds against + # it whether the hook has run or not + project.run_sql(f"drop table if exists {project.test_schema}.hook_sourced") + # the run that reaches the check strategy's own query + assert run_dbt(["snapshot"])[0].status == "success" + + +class TestLoadScopeRunsOutsideTxHooksBeforeTheStrategy(_HookSourcedSnapshot): + scope, hook_tx = "load", "False" + + +class TestBuildScopeRunsInTxHooksBeforeTheStrategy(_HookSourcedSnapshot): + scope, hook_tx = "build", "True" diff --git a/tests/functional/adapter/mssql/test_query_options.py b/tests/functional/adapter/mssql/test_query_options.py index 00e936b7..69552c12 100644 --- a/tests/functional/adapter/mssql/test_query_options.py +++ b/tests/functional/adapter/mssql/test_query_options.py @@ -447,13 +447,28 @@ def test_parameterization_rejected_as_invalid_key(self, project): def test_options_render_on_both_dml_statements(self, project): """A table_refresh_method='dml' model takes the DML-refresh path on the second (steady-state) run, not the create_table_as path. That path has two - grant-taking statements — the 'main' SELECT INTO scratch build and the + grant-taking statements — the 'main' scratch load and the 'dml_refresh_swap' INSERT — and BOTH must carry the query_options hint. + Since #819 the scratch build is two statements rather than one fused + `SELECT * INTO`: an empty `SELECT TOP 0 * INTO`, which moves no rows and + so deliberately carries no hint, then the 'main' + `INSERT ... WITH (TABLOCK)` that loads it. The hint rides the statement + that moves the rows and takes the memory grant, which is 'main'. + Only the 'main' statement lands in target/run; the swap runs via its own statement() call. So this asserts against the executed SQL captured from the debug log, matching each statement bounded by its terminating ';' so a hint on one statement can't be mistaken for a hint on the other. + + That last property needs care now that BOTH statements are + `INSERT ... SELECT ... FROM ` — a bare + `INSERT INTO ... __dbt_refresh ... OPTION` pattern matches either one, so + the swap assertion would pass on the scratch load even if the swap had + lost its hint entirely. They are told apart by what each selects FROM: + the scratch load reads the tmp view (`...__dbt_refresh__dbt_tmp_vw`) and + is the only one with `WITH (TABLOCK)`; the swap reads the scratch table + (`...__dbt_refresh`, with no `__dbt_tmp_vw` suffix). """ # First run creates the table via the standard create path. run_dbt(["run", "--select", "dml_refresh_model"]) @@ -464,23 +479,30 @@ def test_options_render_on_both_dml_statements(self, project): _, logs = run_dbt_and_capture(["--debug", "run", "--select", "dml_refresh_model"]) - # 'main' — SELECT * INTO FROM , must carry the hint. + # 'main' — INSERT INTO WITH (TABLOCK) SELECT * FROM . # [^;]* keeps the match inside the single statement (its only ';' is the - # terminator that follows the OPTION clause). + # terminator that follows the OPTION clause). WITH (TABLOCK) and the + # tmp view both mark this as the scratch load, not the swap. main_match = re.search( - r"SELECT \* INTO[^;]*__dbt_refresh__dbt_tmp_vw[^;]*OPTION \([^;]*MAXDOP 1", + r"INSERT INTO[^;]*WITH \(TABLOCK\)[^;]*" + r"FROM[^;]*__dbt_refresh__dbt_tmp_vw[^;]*OPTION \([^;]*MAXDOP 1", logs, re.IGNORECASE, ) assert main_match is not None, ( - "query_options missing from the 'main' SELECT INTO statement of the DML refresh" + "query_options missing from the 'main' scratch-load INSERT of the DML refresh" ) - # 'dml_refresh_swap' — INSERT ... SELECT ... FROM , must carry - # the hint too. The INSERT...SELECT spans newlines but has no interior - # ';', so [^;]* stays within it and won't reach the main statement above. + # 'dml_refresh_swap' — INSERT INTO (cols) SELECT cols FROM + # , must carry the hint too. The INSERT...SELECT spans newlines + # but has no interior ';', so [^;]* stays within it and won't reach the + # scratch load above. The negative lookahead is what excludes that load: + # it selects FROM the tmp view, whose name continues past __dbt_refresh + # with __dbt_tmp_vw, while the swap selects FROM the scratch table, where + # __dbt_refresh ends the name. swap_match = re.search( - r"INSERT INTO[^;]*SELECT[^;]*__dbt_refresh[^;]*OPTION \([^;]*MAXDOP 1", + r"INSERT INTO[^;]*FROM[^;]*__dbt_refresh(?!__dbt_tmp_vw)" + r"[^;]*OPTION \([^;]*MAXDOP 1", logs, re.IGNORECASE, ) diff --git a/tests/functional/adapter/mssql/test_table_refresh_method.py b/tests/functional/adapter/mssql/test_table_refresh_method.py index 98e5c628..69c8a8b0 100644 --- a/tests/functional/adapter/mssql/test_table_refresh_method.py +++ b/tests/functional/adapter/mssql/test_table_refresh_method.py @@ -275,6 +275,33 @@ def test_dml_refresh_updates_data(self, project): # Scratch table should be cleaned up assert not table_exists(project, "dml_model__dbt_refresh") + def test_scratch_load_is_not_fused_with_its_create(self, project): + """#819: the scratch build must not be a single `SELECT * INTO`. + + Fused, it holds Sch-M on the scratch table for the whole load and + blocks every metadata reader in every other session. Split, `main` is + the load INSERT and the create is its own statement. + """ + run_dbt(["run"]) + write_model(project, "dml_model.sql", dml_model_v2_sql) + results = run_dbt(["run"]) + assert results[0].status == "success" + + run_dir = os.path.join(project.project_root, "target", "run") + for root, _dirs, files in os.walk(run_dir): + if "dml_model.sql" in files: + with open(os.path.join(root, "dml_model.sql")) as f: + main_sql = f.read() + break + else: + raise AssertionError("Could not find the compiled run SQL for dml_model") + + # `main` is the load, and the load only moves rows. + assert "INSERT INTO" in main_sql + assert "WITH (TABLOCK)" in main_sql + assert "SELECT * INTO" not in main_sql + assert "SELECT TOP 0" not in main_sql + def test_schema_change_falls_back_to_rename(self, project): # First run — creates the table results = run_dbt(["run"]) diff --git a/tests/unit/adapters/mssql/test_probe_auto_begin.py b/tests/unit/adapters/mssql/test_probe_auto_begin.py new file mode 100644 index 00000000..8d68204e --- /dev/null +++ b/tests/unit/adapters/mssql/test_probe_auto_begin.py @@ -0,0 +1,85 @@ +"""Read-only catalog probes must not open the ambient transaction (#819). + +A `{% call statement(...) %}` defaults to ``auto_begin=True``, so a probe that +omits the flag OPENS the dbt-managed transaction when none is running. That is +harmless in isolation, but the materialization tail (masks, index +reconciliation, grants, persist_docs) runs *after* the cutover commits, and any +statement it issues with ``auto_begin=False`` merely JOINS whatever is open. So +a single probe reopening a transaction drags every mask ALTER and index build +that follows back inside it, held to the trailing ``adapter.commit()`` - which +is the exact Sch-M window on the live target that #819 is about. + +Probes are pure reads and never need a transaction of their own, so the rule is +simply that they all pass ``auto_begin=False``. This test is a source-level +guard: it fails when a new probe is added without the flag, which a runtime +assertion in a functional test would only catch on the paths it happens to +exercise. +""" + +import re +from pathlib import Path + +import pytest + +MACRO_ROOT = Path(__file__).parents[4] / "dbt" / "include" / "sqlserver" / "macros" + +# Probes that may still open a transaction, with the reason. These run during +# cache population or `dbt docs generate`, never inside a materialization tail, +# so they cannot drag mask/index DDL into a transaction. Revisit as a #819 +# follow-up rather than widening this list. +KNOWN_EXCEPTIONS = { + "list_relations_without_caching": "cache population, not a materialization tail", + "get_relation_without_caching": "cache population, not a materialization tail", + "last_modified": "source freshness, not a materialization tail", + "catalog": "dbt docs generate, not a materialization tail", +} + +# Guards against the regex silently matching nothing if the macro style changes. +MINIMUM_PROBES_EXPECTED = 15 + +CALL_STATEMENT = re.compile(r"\{%-?\s*call\s+statement\(\s*(?P[^)]*)\)") +NAME = re.compile(r"""^\s*['"](?P[^'"]+)['"]""") + + +def _probes(): + """Yield (path, lineno, name, args) for every fetch_result call statement.""" + for path in sorted(MACRO_ROOT.rglob("*.sql")): + for lineno, line in enumerate(path.read_text().splitlines(), start=1): + match = CALL_STATEMENT.search(line) + if not match: + continue + args = match.group("args") + if "fetch_result" not in args.lower(): + continue + name_match = NAME.match(args) + name = name_match.group("name") if name_match else "" + yield path, lineno, name, args + + +ALL_PROBES = list(_probes()) + + +def test_probe_scan_found_statements(): + """The scan itself must keep working if macro formatting changes.""" + assert len(ALL_PROBES) >= MINIMUM_PROBES_EXPECTED, ( + f"only found {len(ALL_PROBES)} fetch_result probes under {MACRO_ROOT}; " + "the call-statement regex has probably gone stale" + ) + + +@pytest.mark.parametrize( + "path,lineno,name,args", + ALL_PROBES, + ids=[f"{p.name}:{ln}:{n}" for p, ln, n, _ in ALL_PROBES], +) +def test_probe_declines_the_ambient_transaction(path, lineno, name, args): + if name in KNOWN_EXCEPTIONS: + pytest.skip(f"{name}: {KNOWN_EXCEPTIONS[name]}") + assert re.search(r"auto_begin\s*=\s*[Ff]alse", args), ( + f"{path.relative_to(MACRO_ROOT.parents[3])}:{lineno} statement " + f"'{name}' is a read-only probe but does not pass auto_begin=False, so " + "it opens the ambient transaction and the materialization tail's " + "mask/index DDL will join it and hold it to commit (#819). Add " + "auto_begin=False, or add the statement to KNOWN_EXCEPTIONS with the " + "reason it can never run in a materialization tail." + ) diff --git a/tests/unit/adapters/mssql/test_table_build_sql.py b/tests/unit/adapters/mssql/test_table_build_sql.py new file mode 100644 index 00000000..0e83669c --- /dev/null +++ b/tests/unit/adapters/mssql/test_table_build_sql.py @@ -0,0 +1,608 @@ +"""The shared table-build SQL macros (#819). + +``sqlserver__get_create_table_empty_sql`` and +``sqlserver__get_tablock_insert_sql`` are the one place that decides how a +table gets created and loaded. Three call sites share them, so the emitted +shape is pinned here rather than in each functional suite. + +These render the real macro file through Jinja2 - no database connection +required - with stubs for the ambient dbt context the macros touch. +""" + +import re +from pathlib import Path + +import jinja2 +import pytest + +from dbt.adapters.sqlserver.sqlserver_adapter import SQLServerAdapter +from dbt.adapters.sqlserver.sqlserver_relation import SQLServerRelation + +CREATE_SQL = ( + Path(__file__).parents[4] + / "dbt" + / "include" + / "sqlserver" + / "macros" + / "relations" + / "table" + / "create.sql" +) + +# The macros under test reach for these; everything else in create.sql lives +# inside macros we never call, so it stays unresolved harmlessly. +_STUBS = """ +{% macro get_assert_columns_equivalent(sql) %}(/* assert_columns_equivalent */){% endmacro %} +{% macro build_columns_constraints(relation) %}(/* columns_constraints */){% endmacro %} +""" + +QUERY_LABEL = "OPTION (LABEL = 'dbt-sqlserver', MAXDOP 1);" + + +class _Adapter: + quote = staticmethod(SQLServerAdapter.quote) + + +def _render(call, **context): + """Render a call against the real create.sql plus the stubs above.""" + source = _STUBS + CREATE_SQL.read_text() + "\n" + call + env = jinja2.Environment( + undefined=jinja2.StrictUndefined, + extensions=["jinja2.ext.do"], # create.sql uses {% do %}, as dbt's env does + ) + template = env.from_string(source) + return " ".join( + template.render( + adapter=_Adapter(), + model={"columns": {"id": {}, "my col": {}}}, + **context, + ).split() + ) + + +@pytest.fixture +def target(): + return SQLServerRelation.create(database="db", schema="sch", identifier="rel", type="table") + + +@pytest.fixture +def tmp_vw(): + return SQLServerRelation.create( + database="db", schema="sch", identifier="rel__dbt_tmp_vw", type="view" + ) + + +def _create(contract_enforced): + return ( + "{{ sqlserver__get_create_table_empty_sql(" + f"target, tmp_vw, 'select 1 as id', {str(contract_enforced).lower()}) }}}}" + ).replace("}}}}", "}}") + + +def _insert(contract_enforced): + return ( + "{{ sqlserver__get_tablock_insert_sql(" + f"target, tmp_vw, query_label, {str(contract_enforced).lower()}) }}}}" + ).replace("}}}}", "}}") + + +# -- the empty create -- + + +def test_empty_create_moves_no_rows(target, tmp_vw): + """TOP 0 is the whole point: Sch-M is held for an instant, not for the load.""" + sql = _render(_create(False), target=target, tmp_vw=tmp_vw) + assert sql == 'SELECT TOP 0 * INTO "db"."sch"."rel" FROM "db"."sch"."rel__dbt_tmp_vw"' + + +def test_empty_create_under_contract_emits_ddl(target, tmp_vw): + sql = _render(_create(True), target=target, tmp_vw=tmp_vw) + assert sql.startswith('CREATE TABLE "db"."sch"."rel"') + assert "assert_columns_equivalent" in sql + assert "columns_constraints" in sql + # The contract create is pure DDL - it must not move rows. + assert "INSERT" not in sql + assert "SELECT TOP 0" not in sql + + +def test_empty_create_carries_no_query_label(target, tmp_vw): + """The OPTION clause rides the data-movement statement, not the create.""" + for contract_enforced in (True, False): + sql = _render(_create(contract_enforced), target=target, tmp_vw=tmp_vw) + assert "OPTION" not in sql + assert "LABEL" not in sql + + +# -- the load -- + + +def test_load_is_tablock_insert(target, tmp_vw): + sql = _render(_insert(False), target=target, tmp_vw=tmp_vw, query_label=QUERY_LABEL) + assert sql == ( + 'INSERT INTO "db"."sch"."rel" WITH (TABLOCK) ' + 'SELECT * FROM "db"."sch"."rel__dbt_tmp_vw" ' + QUERY_LABEL + ) + + +def test_load_under_contract_names_columns_on_both_sides(target, tmp_vw): + sql = _render(_insert(True), target=target, tmp_vw=tmp_vw, query_label=QUERY_LABEL) + assert sql == ( + 'INSERT INTO "db"."sch"."rel" WITH (TABLOCK) ("id", "my col") ' + 'SELECT "id", "my col" FROM "db"."sch"."rel__dbt_tmp_vw" ' + QUERY_LABEL + ) + + +@pytest.mark.parametrize("contract_enforced", [True, False]) +def test_load_keeps_tablock_and_query_label(target, tmp_vw, contract_enforced): + """Minimal logging needs the table lock; #613's OPTION clause has to survive + alongside it (see test_interaction_tablock_query_options.py).""" + sql = _render( + _insert(contract_enforced), target=target, tmp_vw=tmp_vw, query_label=QUERY_LABEL + ) + assert "WITH (TABLOCK)" in sql + assert "MAXDOP 1" in sql + assert "LABEL =" in sql + + +@pytest.mark.parametrize("contract_enforced", [True, False]) +def test_load_never_creates_the_table(target, tmp_vw, contract_enforced): + """Fusing the create back into the load is the bug in #819.""" + sql = _render( + _insert(contract_enforced), target=target, tmp_vw=tmp_vw, query_label=QUERY_LABEL + ) + assert "CREATE TABLE" not in sql + assert " INTO " not in sql.replace("INSERT INTO", "INSERT") + + +@pytest.mark.parametrize("contract_enforced", [True, False]) +def test_load_does_not_reassert_the_contract(target, tmp_vw, contract_enforced): + """get_assert_columns_equivalent raises on mismatch, so it belongs to the + create macro alone - once per build, not twice.""" + sql = _render( + _insert(contract_enforced), target=target, tmp_vw=tmp_vw, query_label=QUERY_LABEL + ) + assert "assert_columns_equivalent" not in sql + + +# -- the whole create_table_as batch -- +# +# The two macros above are strings; this renders the batch that actually ships, +# to catch a missing statement terminator or a broken EXEC literal. + +_BATCH_STUBS = """ +{% macro get_query_options(parse_options=False) %}OPTION (LABEL = 'dbt-sqlserver');{% endmacro %} +{% macro get_use_database_sql(database) %}USE {{ database }};{% endmacro %} +{% macro get_create_view_as_sql(relation, sql) %}CREATE OR ALTER VIEW {{ relation }} AS {{ sql }}; +{% endmacro %} +{% macro escape_single_quotes(value) %}{{ value | replace("'", "''") }}{% endmacro %} +{% macro sqlserver__create_clustered_columnstore_index(relation) %}CREATE CLUSTERED COLUMNSTORE +INDEX cci ON {{ relation }};{% endmacro %} +""" + + +class _Config: + def __init__(self, contract_enforced=False, **values): + self._values = values + self._contract = type("Contract", (), {"enforced": contract_enforced})() + + def get(self, key, default=None): + if key == "contract": + return self._contract + return self._values.get(key, default) + + +class _BatchAdapter(_Adapter): + def drop_relation(self, relation): + return "" + + +def _render_batch(temporary, relation, config): + source = ( + _BATCH_STUBS + + _STUBS + + CREATE_SQL.read_text() + + "\n{{ sqlserver__create_table_as(temporary, relation, 'select 1 as id') }}" + ) + env = jinja2.Environment(undefined=jinja2.StrictUndefined, extensions=["jinja2.ext.do"]) + return env.from_string(source).render( + adapter=_BatchAdapter(), + model={"columns": {"id": {}}}, + config=config, + temporary=temporary, + relation=relation, + ) + + +def test_create_table_as_batch_terminates_the_drop_guard(target): + """A statement sharing an EXEC literal with the create needs a terminator. + + The create and the load no longer share one (see the stage/load split), so + the create is now last in its literal and needs nothing after it. The drop + guard still precedes it there on throwaway builds, though, and an + unterminated guard makes the literal a parse error. + """ + throwaway = SQLServerRelation.create( + database="db", schema="sch", identifier="rel__dbt_tmp", type="table" + ) + normalized = " ".join(_render_batch(False, throwaway, _Config()).split()) + assert "IF OBJECT_ID(" in normalized + create_start = normalized.index("SELECT TOP 0 * INTO") + assert normalized[:create_start].rstrip().endswith(";"), normalized[:create_start] + + +def test_create_table_as_escapes_every_exec_literal(target): + """The load's OPTION clause must not break out of its EXEC literal. + + The create and the load used to share one EXEC; since the stage/load split + they have one each, so the escaping that protects the load now has to come + from the load half rather than from a shared wrapper. That is the invariant + worth pinning - the count below only documents which three there are. + """ + batch = _render_batch(False, target, _Config()) + # stage: the empty create. load: the insert, then the tmp view drop. + assert batch.count("EXEC('") == 3 + # The label's quotes are doubled, proving the load text was escaped. + assert "''dbt-sqlserver''" in batch + assert "'dbt-sqlserver'" not in batch.replace("''dbt-sqlserver''", "") + + +def test_create_table_as_orders_load_before_the_columnstore_index(target): + """heap_then_index: load the heap, then build the CCI over it.""" + batch = _render_batch(False, target, _Config(as_columnstore=True)) + assert batch.index("INSERT INTO") < batch.index("CREATE CLUSTERED COLUMNSTORE") + + +def test_create_table_as_temp_build_is_split_too(target): + """The incremental temp build is the path that most needs the split: it + already autocommits, so the create's Sch-M goes as soon as it finishes.""" + temp = SQLServerRelation.create( + database="db", schema="sch", identifier="rel__dbt_tmp", type="table" + ) + batch = " ".join(_render_batch(True, temp, _Config(contract_enforced=True)).split()) + # Contracts are suppressed for temp builds, so this is the non-contract pair. + assert "SELECT TOP 0 * INTO" in batch + assert "INSERT INTO" in batch + assert "CREATE TABLE" not in batch + + +# -- lock discipline at the call sites -- +# +# Splitting the create from the load only helps if the two statements do not +# share an open transaction: locks are held to commit, not to end-of-statement. +# The call sites earn that with auto_begin=False and explicit commit +# boundaries, and these tests keep it that way (#819). + +DML_REFRESH_SQL = CREATE_SQL.parents[2] / "materializations" / "models" / "table" +DML_REFRESH_SQL = DML_REFRESH_SQL / "table_dml_refresh.sql" + +SWAP_MARKER = "statement('dml_refresh_swap'" + + +def _dml_refresh_source(): + source = DML_REFRESH_SQL.read_text() + assert SWAP_MARKER in source, "the swap statement is the boundary these tests split on" + before_swap, after_swap = source.split(SWAP_MARKER, 1) + return source, before_swap, after_swap + + +def test_dml_refresh_scratch_build_is_split(): + source, _before, _after = _dml_refresh_source() + assert "sqlserver__get_create_table_empty_sql" in source + assert "sqlserver__get_tablock_insert_sql" in source + # The fused form is the bug: one statement that both creates and loads. + assert "SELECT * INTO {{ refresh_relation }}" not in source + + +def test_dml_refresh_declines_the_ambient_transaction_until_the_swap(): + """Every statement in the scratch build has to autocommit, or its catalog + locks are held to the materialization's trailing commit anyway.""" + _source, before_swap, _after = _dml_refresh_source() + # `call statement(`, so prose mentioning statement('main') is not a match. + statements = re.findall(r"call statement\((.*?)\)", before_swap, re.DOTALL) + assert statements, "expected the scratch build to issue statements" + offenders = [s for s in statements if "auto_begin=False" not in s] + assert not offenders, ( + "statements before the swap must pass auto_begin=False so they do not " + f"open the ambient transaction (#819): {offenders}" + ) + + +MACRO_ROOT = CREATE_SQL.parents[2] +TABLE_SQL = MACRO_ROOT / "materializations" / "models" / "table" / "table.sql" + + +@pytest.mark.parametrize("macro_file", sorted(MACRO_ROOT.rglob("*.sql")), ids=lambda p: p.name) +def test_no_macro_fuses_a_create_with_its_load(macro_file): + """`SELECT * INTO ` creates and loads in one statement, holding + Sch-M on the new object for the length of the load (#819). Build the table + empty and load it separately - the two macros at the top of create.sql. + + Matches `INTO {{`, so prose describing the fused form does not count. + """ + offenders = [ + f" {macro_file.name}:{lineno}: {line.strip()}" + for lineno, line in enumerate(macro_file.read_text().splitlines(), start=1) + if re.search(r"(?i)SELECT\s+\*\s+INTO\s+\{\{", line) + ] + assert not offenders, ( + "use sqlserver__get_create_table_empty_sql + " + "sqlserver__get_tablock_insert_sql instead of a fused SELECT * INTO " + "(#819):\n" + "\n".join(offenders) + ) + + +def test_table_materialization_stages_before_the_in_transaction_pre_hooks(): + """The create's Sch-M must be released before any pre-hook opens the transaction. + + auto_begin=False only declines to OPEN a transaction; it still joins one a + pre-hook left open, and then the create's Sch-M would be held to commit for + the length of the load. So the stage runs BEFORE run_hooks(inside_transaction=True), + where nothing is open and it autocommits, and the load joins the hook's + transaction afterwards (X table lock only). Committing the pre-hook early + instead would cost every transaction: true pre-hook its rollback. + """ + source = TABLE_SQL.read_text() + outside_hooks = source.find("run_hooks(pre_hooks, inside_transaction=False)") + stage = source.find("call statement('create_table_stage', auto_begin=False)") + dml_stage = source.find("sqlserver__table_dml_refresh_stage(target_relation, sql)") + in_tx_hooks = source.find("run_hooks(pre_hooks, inside_transaction=True)") + load = source.find("call statement('main', auto_begin=False)") + begin = source.find("adapter.begin_if_closed()") + rename = source.find("adapter.rename_relation") + assert -1 < outside_hooks < stage < in_tx_hooks < load < begin < rename, ( + "stage after the outside-transaction hooks and before the in-transaction " + "ones; load after them; reopen before the renames so the cutover is " + "transactional and adapter.commit() has a matching BEGIN" + ) + assert -1 < outside_hooks < dml_stage < in_tx_hooks, ( + "the dml scratch build is staged ahead of the in-transaction hooks too" + ) + between = source[stage:in_tx_hooks] + assert "commit_if_open" not in between, ( + "no commit between the stage and the hooks: the stage autocommits on its " + "own, and a commit here would be a no-op that reads as if it were needed" + ) + + +def test_table_materialization_writes_the_whole_build_to_the_artifact(): + """statement() writes compiled SQL for 'main' only. + + On the split path 'main' is the load, so target/run/ would hold the INSERT + without the CREATE that precedes it - and the constraint tests read exactly + that file. Write both halves back over it. + """ + source = TABLE_SQL.read_text() + assert "write(stage_sql ~" in source + + +def test_tmp_view_is_dropped_after_the_cutover_commits(): + """An uncommitted DROP VIEW holds Sch-M on the view, and a database-wide + catalog scan blocks on that just as on a table. With the load inside a + pre-hook's transaction, the view drop has to wait for the commit.""" + source = TABLE_SQL.read_text() + assert "drop_tmp_view=False" in source, "the split load must not drop the view in-batch" + after_post_hooks = source.split("run_hooks(post_hooks, inside_transaction=True)", 1)[1] + commit = after_post_hooks.find("adapter.commit_if_open()") + view_drop = after_post_hooks.find("DROP VIEW IF EXISTS") + indexes = after_post_hooks.find("create_indexes(target_relation)") + assert -1 < commit < view_drop < indexes + + +def test_masks_stay_inside_the_cutover_transaction_on_fresh_builds(): + """A brand-new table carries no masks until apply_masks runs. + + If that ran after the cutover committed, a mask failure would leave the + newly loaded table live with the columns exposed. Index builds move out of + the transaction; masks on fresh tables must not. + """ + source = TABLE_SQL.read_text() + # Anchor past the build's own stage/load commit, which is not the cutover. + after_swap = source.split( + "adapter.rename_relation(intermediate_relation, target_relation)", 1 + )[1] + masks = after_swap.find("apply_masks(target_relation, mask_config)") + post_hooks = after_swap.find("run_hooks(post_hooks, inside_transaction=True)") + cutover_commit = after_swap.find("adapter.commit_if_open()") + assert -1 < masks < post_hooks < cutover_commit, ( + "masks belong inside the cutover transaction, before the in-transaction " + "post-hooks and the commit that closes the atomic unit" + ) + + +def test_dml_refresh_leaves_the_swap_transaction_for_the_tail(): + """The macro must not close the swap's transaction itself. + + It used to, which meant an in-transaction post-hook - running back in + table.sql, after the macro returned - was NOT atomic with the swap it was + written to accompany. The tail now owns that boundary. + """ + _source, _before, after_swap = _dml_refresh_source() + assert "adapter.commit_if_open()" not in after_swap, ( + "the swap's transaction is closed by table.sql after the post-hooks, not inside this macro" + ) + assert "sqlserver__reconcile_indexes" not in after_swap, ( + "index reconciliation moved to the common tail, outside the transaction" + ) + + +def test_dml_refresh_swap_locks_do_not_span_reconciliation(): + """The DELETE holds X locks on the target until commit. + + Index and mask reconciliation must sit outside that window, which now means + after the tail's commit rather than after one inside the macro. + """ + source = TABLE_SQL.read_text() + after_swap = source.split("run_hooks(post_hooks, inside_transaction=True)", 1)[1] + commit = after_swap.find("adapter.commit_if_open()") + reconcile = after_swap.find("sqlserver__reconcile_indexes") + assert -1 < commit < reconcile, "commit the cutover before reconciling indexes" + + +def test_dml_scratch_table_is_dropped_after_the_cutover_commits(): + """Dropping it inside the transaction would put its catalog locks back in + the window the tail exists to clear.""" + source = TABLE_SQL.read_text() + after_swap = source.split("run_hooks(post_hooks, inside_transaction=True)", 1)[1] + commit = after_swap.find("adapter.commit_if_open()") + drop = after_swap.find("dml_refresh_cleanup_post") + assert -1 < commit < drop + + +# -- the stage / load split (#819) -- +# +# sqlserver__create_table_as is split at the seam between creating the object +# and loading it, so a caller can put a transaction boundary between the two. +# The halves are the source of truth and create_table_as is their +# concatenation, so the callers that still want one batch (snapshots, the +# incremental temp build) keep getting exactly that. + +_SPLIT_STUBS = ( + """ +{% macro get_use_database_sql(database) %}USE [{{ database }}];{% endmacro %} +{% macro get_create_view_as_sql(relation, sql) %} +EXEC('CREATE OR ALTER VIEW {{ relation }} AS {{ sql }}') +{%- endmacro %} +{% macro escape_single_quotes(value) %}{{ value | replace("'", "''") }}{% endmacro %} +{% macro get_query_options(parse_options=False) %}""" + + QUERY_LABEL + + """{% endmacro %} +{% macro sqlserver__create_clustered_columnstore_index(relation) %} +/* CCI on {{ relation }} */ +{%- endmacro %} +""" +) + + +class _SplitContract: + def __init__(self, enforced): + self.enforced = enforced + + +class _SplitConfig: + """Minimal stand-in for dbt's `config` context var.""" + + def __init__(self, contract_enforced=False, as_columnstore=True): + self._values = { + "contract": _SplitContract(contract_enforced), + "as_columnstore": as_columnstore, + "full_refresh_build": "heap_then_index", + } + + def get(self, key, default=None): + return self._values.get(key, default) + + +class _SplitAdapter(_Adapter): + """Adds the render-time side effect the stage half performs.""" + + def __init__(self): + self.dropped = [] + + def drop_relation(self, relation): + self.dropped.append(str(relation)) + + +def _render_split(call, adapter=None, config=None, **context): + source = _SPLIT_STUBS + _STUBS + CREATE_SQL.read_text() + "\n" + call + env = jinja2.Environment( + undefined=jinja2.StrictUndefined, + extensions=["jinja2.ext.do"], + ) + return " ".join( + env.from_string(source) + .render( + adapter=adapter or _SplitAdapter(), + config=config or _SplitConfig(), + model={"columns": {"id": {}, "my col": {}}}, + **context, + ) + .split() + ) + + +def _stage(temporary=False): + return ( + "{{ sqlserver__get_create_table_stage_sql(" + f"{str(temporary).lower()}, target, 'select 1 as id') }}}}" + ).replace("}}}}", "}}") + + +def _load(temporary=False): + return ( + "{{ sqlserver__get_create_table_load_sql(" + f"{str(temporary).lower()}, target, 'select 1 as id') }}}}" + ).replace("}}}}", "}}") + + +def _whole(temporary=False): + return ( + f"{{{{ sqlserver__create_table_as({str(temporary).lower()}, target, 'select 1 as id') }}}}" + ) + + +@pytest.mark.parametrize("contract_enforced", [True, False]) +def test_create_table_as_is_exactly_stage_then_load(target, contract_enforced): + """The invariant that keeps every existing caller safe. + + Snapshots and the incremental temp build call create_table_as and run the + result as one batch. Whatever the split does, their SQL must stay the + concatenation of the two halves - no statement added, dropped or reordered. + """ + config = _SplitConfig(contract_enforced=contract_enforced) + stage = _render_split(_stage(), config=config, target=target) + load = _render_split(_load(), config=config, target=target) + whole = _render_split(_whole(), config=config, target=target) + assert whole == " ".join(f"{stage} {load}".split()) + + +def test_stage_creates_the_view_and_the_empty_table_only(target): + sql = _render_split(_stage(), target=target) + assert "CREATE OR ALTER VIEW" in sql + assert "SELECT TOP 0 * INTO" in sql + # The load's work belongs to the other half. + assert "INSERT INTO" not in sql + assert "CCI" not in sql + + +def test_load_inserts_drops_the_view_and_builds_the_cci(target): + sql = _render_split(_load(), target=target) + assert "INSERT INTO" in sql + assert "WITH (TABLOCK)" in sql + assert "DROP VIEW IF EXISTS" in sql + assert "CCI" in sql + # Creating the object is the other half's job. + assert "SELECT TOP 0 * INTO" not in sql + assert "CREATE OR ALTER VIEW" not in sql + + +def test_view_drop_follows_the_insert_not_the_create(target): + """The load reads the view, so the drop cannot stay with the create.""" + sql = _render_split(_load(), target=target) + assert sql.index("INSERT INTO") < sql.index("DROP VIEW IF EXISTS") + + +def test_tmp_view_is_dropped_once_by_the_stage_half(target): + """adapter.drop_relation is a render-time side effect, not emitted SQL. + + It must fire exactly once per build - in the stage half - so rendering the + halves separately does not drop the view twice, and rendering the whole + macro does not skip it. + """ + stage_adapter = _SplitAdapter() + _render_split(_stage(), adapter=stage_adapter, target=target) + assert len(stage_adapter.dropped) == 1 + + load_adapter = _SplitAdapter() + _render_split(_load(), adapter=load_adapter, target=target) + assert load_adapter.dropped == [] + + whole_adapter = _SplitAdapter() + _render_split(_whole(), adapter=whole_adapter, target=target) + assert len(whole_adapter.dropped) == 1 + + +def test_temporary_build_skips_the_cci(target): + """as_columnstore never applied to temp builds; that must survive the split.""" + sql = _render_split(_load(temporary=True), target=target) + assert "CCI" not in sql + assert "INSERT INTO" in sql