From 6d7d5aa754c0ab4f2e1042ef8e5cdc1f4fc68d71 Mon Sep 17 00:00:00 2001 From: Axell Padilla <68310020+axellpadilla@users.noreply.github.com> Date: Sat, 1 Aug 2026 06:15:51 +0000 Subject: [PATCH 1/2] fix: drop the model's outbound foreign keys too (#632) drop_fk_constraints() only matched foreign keys whose referenced table was the model, so the keys the table itself declares survived the macro and kept blocking a truncate or rebuild of the table they point at. Join on parent_object_id as well, still scoped to the model's own schema. Consolidate the module's tests into a single project and dbt invocation, adding fk_model to cover both directions. No CHANGELOG change, so this cherry-picks clean onto release/v1.11. --- .../sqlserver/macros/adapters/indexes.sql | 18 +- .../adapter/mssql/test_index_macros.py | 155 +++++++++++++----- 2 files changed, 124 insertions(+), 49 deletions(-) diff --git a/dbt/include/sqlserver/macros/adapters/indexes.sql b/dbt/include/sqlserver/macros/adapters/indexes.sql index 0eb3425e..32107c1c 100644 --- a/dbt/include/sqlserver/macros/adapters/indexes.sql +++ b/dbt/include/sqlserver/macros/adapters/indexes.sql @@ -58,15 +58,25 @@ {{ log("Running drop_fk_constraints() macro...") }} - {# The schema filter applies to the referenced table (this model). The - constraint itself is dropped from the referencing (parent) table, which - may legitimately live in another schema, so it is not filtered. #} + {# Both directions are dropped (issue #632): the inbound keys of other + tables that reference this model, and this model's own outbound keys. + An inbound key blocks dropping this table or its primary key; an + outbound one blocks a truncate/rebuild of the table it points at and + would otherwise survive a rebuild of this one. + + The schema filter applies to this model's table only, so same-named + tables in other schemas keep their constraints. The counterparty of + either key may legitimately live in another schema and is not filtered. + ALTER TABLE always targets the constraint's own schema and parent table, + which is correct in both directions. #} declare @drop_fk_constraints nvarchar(max); select @drop_fk_constraints = ( select 'IF OBJECT_ID(''' + REPLACE(QUOTENAME(SCHEMA_NAME(sys.foreign_keys.[schema_id])) + '.' + QUOTENAME(sys.foreign_keys.[name]), '''', '''''') + ''', ''F'') IS NOT NULL ALTER TABLE ' + QUOTENAME(SCHEMA_NAME(sys.foreign_keys.[schema_id])) + '.' + QUOTENAME(OBJECT_NAME(sys.foreign_keys.[parent_object_id])) + ' DROP CONSTRAINT ' + QUOTENAME(sys.foreign_keys.[name]) + ';' from sys.foreign_keys {{ information_schema_hints() }} - inner join sys.tables {{ information_schema_hints() }} on sys.foreign_keys.[referenced_object_id] = sys.tables.[object_id] + inner join sys.tables {{ information_schema_hints() }} + on sys.foreign_keys.[referenced_object_id] = sys.tables.[object_id] + or sys.foreign_keys.[parent_object_id] = sys.tables.[object_id] where SCHEMA_NAME(sys.tables.[schema_id]) = '{{ this.schema }}' and sys.tables.[name] = '{{ this.table }}' for xml path('') diff --git a/tests/functional/adapter/mssql/test_index_macros.py b/tests/functional/adapter/mssql/test_index_macros.py index 6a779634..80f47e59 100644 --- a/tests/functional/adapter/mssql/test_index_macros.py +++ b/tests/functional/adapter/mssql/test_index_macros.py @@ -1,7 +1,7 @@ import pytest from dbt.tests.util import get_connection, run_dbt -from tests.functional.adapter.mssql.test_index_config import index_count, indexes_def +from tests.functional.adapter.mssql.test_index_config import indexes_def # flake8: noqa: E501 @@ -85,51 +85,52 @@ and t.[name] = '{table_name}' """ +# Foreign keys touching a table in either direction: inbound (some other table +# references it) and outbound (it references some other table). +fk_count_both_directions = """ +select + sum(case when fk.referenced_object_id = t.object_id then 1 else 0 end) as inbound, + sum(case when fk.parent_object_id = t.object_id then 1 else 0 end) as outbound +from sys.tables t +left join sys.foreign_keys fk + on fk.referenced_object_id = t.object_id + or fk.parent_object_id = t.object_id +where schema_name(t.schema_id) = '{schema_name}' + and t.[name] = '{table_name}' +""" -class TestIndex: - @pytest.fixture(scope="class") - def project_config_update(self): - return {"name": "generic_tests"} - - @pytest.fixture(scope="class") - def seeds(self): - return { - "raw_data.csv": index_seed_csv, - "schema.yml": index_schema_base_yml, - } - - @pytest.fixture(scope="class") - def models(self): - return { - "index_model.sql": model_sql, - "index_ccs_model.sql": model_sql_ccs, - "schema.yml": model_yml, - } - - def drop_artifacts(self, project): - with get_connection(project.adapter): - project.adapter.execute("DROP TABLE IF EXISTS index_model", fetch=True) - project.adapter.execute("DROP TABLE IF EXISTS index_ccs_model") +# The model owns a foreign key in each direction by the time +# drop_all_indexes_on_table() runs: fk_child points at it, and it points at +# fk_target. Both must be gone afterwards (issue #632). +drop_both_fk_directions_model = """ +{{ + config({ + "materialized": 'table', + "as_columnstore": False, + "post-hook": [ + "alter table {{ this.schema }}.{{ this.identifier }} alter column id_col int not null", + "alter table {{ this.schema }}.{{ this.identifier }} add constraint pk_fk_model primary key (id_col)", + "if object_id('{{ this.schema }}.fk_target', 'U') is null create table {{ this.schema }}.fk_target (target_id int not null constraint pk_fk_target primary key)", + "if object_id('{{ this.schema }}.fk_child', 'U') is null create table {{ this.schema }}.fk_child (child_id int not null, constraint fk_child_to_model foreign key (child_id) references {{ this.schema }}.{{ this.identifier }} (id_col))", + "alter table {{ this.schema }}.{{ this.identifier }} with nocheck add constraint fk_model_to_target foreign key (secondary_data) references {{ this.schema }}.fk_target (target_id)", + "{{ drop_all_indexes_on_table() }}", + ] + }) +}} +select * from {{ ref('raw_data') }} +""" - def test_create_index(self, project): - run_dbt(["seed"]) - run_dbt(["run"]) - with get_connection(project.adapter): - result, table = project.adapter.execute( - index_count.format(schema_name=project.created_schemas[0]), fetch=True - ) - schema_dict = {_[0]: _[1] for _ in table.rows} - expected = { - "clustered columnstore": 1, - "clustered unique": 1, - "nonclustered": 4, - } - self.drop_artifacts(project) - assert schema_dict == expected +class TestIndexMacros: + """Every index-macro assertion in this module shares one project and one + dbt invocation. The models are independent of each other, so each test + reads only the tables it owns: + create_index_model / index_ccs_model index creation + index_model drops stay inside the model's schema + fk_model drops reach both foreign key directions + """ -class TestIndexDropsOnlySchema: @pytest.fixture(scope="class") def project_config_update(self): return {"name": "generic_tests"} @@ -143,12 +144,26 @@ def seeds(self): @pytest.fixture(scope="class") def models(self): + # index_model keeps its name deliberately: it has to collide with the + # decoy table created in the other schema. return { + "create_index_model.sql": model_sql, "index_model.sql": drop_schema_model, "index_ccs_model.sql": model_sql_ccs, + "fk_model.sql": drop_both_fk_directions_model, "schema.yml": model_yml, } + @pytest.fixture(scope="class", autouse=True) + def build_once(self, project): + # The decoy table in the other schema has to exist before the run; the + # single `build` then seeds and builds every model in this class. + self.create_table_and_index_other_schema(project) + run_dbt(["build"]) + yield + self.drop_fk_artifacts(project) + self.drop_schema_artifacts(project) + def create_table_and_index_other_schema(self, project): _schema = project.test_schema + "other" create_sql = f""" @@ -199,6 +214,16 @@ def drop_schema_artifacts(self, project): project.adapter.execute(drop_table) project.adapter.execute(drop_schema) + def drop_fk_artifacts(self, project): + # Ordered so teardown still works when the macro under test leaves a + # key behind: each table goes before the one it points at. + with get_connection(project.adapter): + project.adapter.execute( + f"DROP TABLE IF EXISTS {project.test_schema}.fk_child", fetch=True + ) + project.adapter.execute(f"DROP TABLE IF EXISTS {project.test_schema}.fk_model") + project.adapter.execute(f"DROP TABLE IF EXISTS {project.test_schema}.fk_target") + def validate_other_schema(self, project): _schema = project.test_schema + "other" with get_connection(project.adapter): @@ -223,8 +248,48 @@ def validate_other_schema(self, project): assert fk_table.rows[0][0] == 1 def test_create_index(self, project): - self.create_table_and_index_other_schema(project) - run_dbt(["seed"]) - run_dbt(["run"]) + # Counted over the two index-building models by name rather than over + # the whole schema: the drop models share this schema, so a schema-wide + # count would make this assertion fail whenever a drop macro breaks. + index_types = {} + with get_connection(project.adapter): + for table_name in ("create_index_model", "index_ccs_model"): + _, table = project.adapter.execute( + indexes_def.format(schema_name=project.test_schema, table_name=table_name), + fetch=True, + ) + for row in table.rows: + key = row["index_type"] + (" unique" if row["unique"] == "Unique" else "") + index_types[key] = index_types.get(key, 0) + 1 + + assert index_types == { + "clustered columnstore": 1, + "clustered unique": 1, + "nonclustered": 4, + } + + def test_leaves_other_schemas_alone(self, project): self.validate_other_schema(project) - self.drop_schema_artifacts(project) + + def test_drops_inbound_and_outbound_fks(self, project): + """Both directions, not just the keys pointing at the model (#632).""" + with get_connection(project.adapter): + _, model_fks = project.adapter.execute( + fk_count_both_directions.format( + schema_name=project.test_schema, table_name="fk_model" + ), + fetch=True, + ) + # The counterparties keep their own constraints: fk_target's + # primary key is untouched by a drop scoped to fk_model. + _, target_pk = project.adapter.execute( + other_schema_pk_count.format( + schema_name=project.test_schema, table_name="fk_target" + ), + fetch=True, + ) + + inbound, outbound = model_fks.rows[0][0], model_fks.rows[0][1] + assert inbound == 0, "inbound foreign key survived drop_fk_constraints()" + assert outbound == 0, "outbound foreign key survived drop_fk_constraints()" + assert target_pk.rows[0][0] == 1 From b3deecc998bd9c13c7d09d833ea6efc939d41960 Mon Sep 17 00:00:00 2001 From: Axell Padilla <68310020+axellpadilla@users.noreply.github.com> Date: Sat, 1 Aug 2026 06:16:34 +0000 Subject: [PATCH 2/2] chore: changelog the outbound foreign key fix under v1.11.0 --- CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 97f81685..1ed19b52 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -19,6 +19,7 @@ - Fix `dbt_sqlserver_use_dbt_transactions: True` (opt-in in 1.11, the default from 1.12) breaking two things that assumed the old autocommit-per-statement behavior: building an index with `build_options: {online: true}` or `{resumable: true}` on a fresh table or `table_refresh_method`'s rename-swap path (SQL Server rejects `RESUMABLE` inside a user transaction outright — `create_indexes` didn't split these out the way index reconciliation already did); and the `dbt_full_refresh_incomplete` crash marker on `full_refresh_build=prebuilt` and incremental full refreshes, which previously rode the same ambient transaction as the rebuild it's meant to survive, so a real failure mid-rebuild rolled the marker back along with everything else instead of leaving it in place to block the next normal run. - Fix concurrent incremental models deadlocking (error 1205) at `threads` > 1 with `dbt_sqlserver_use_dbt_transactions: True` (opt-in in 1.11, the default from 1.12). The temp-relation build — `CREATE OR ALTER VIEW …__dbt_tmp_vw`, `SELECT * INTO …__dbt_tmp`, `DROP VIEW` — is all system-catalog DDL, and it was running inside the materialization's ambient transaction, so its exclusive `sys.sysschobjs` key locks were held until commit while a second worker's name resolution asked for shared locks on the same rows. Nothing in that build needs a transaction (the relation is throwaway); it was being opened accidentally, by a read-only catalog lookup that used `statement()`'s default `auto_begin=True`. The temp build and the fresh-create/full-refresh `create_table_as` batch now autocommit statement by statement and release their catalog locks as they go, while the incremental strategy DML stays transactional through to `adapter.commit()` as before. Pre-hook rollback semantics are unchanged: nothing is committed early, dbt only declines to open a transaction for a read. Because the temp build now commits standalone, a crashed run can leave a `__dbt_tmp` table behind, so the `SELECT * INTO` path drops adapter-generated throwaways first instead of failing the next run with `Msg 2714`; a fresh create of a real target still surfaces `Msg 2714` rather than silently destroying an object dbt does not know about. - Fix `drop_all_indexes_on_table()` and its component macros (`drop_pk_constraints()`, `drop_fk_constraints()`, `drop_xml_indexes()`, `drop_spatial_indexes()`) matching tables by name only. A post-hook on a model dropped the primary key, inbound foreign keys, and XML/spatial indexes of every same-named table in *other* schemas of the same database. All four now filter on the model's schema as well. Identifiers in the generated dynamic SQL are also wrapped in `QUOTENAME()`. +- Fix `drop_fk_constraints()` (and therefore `drop_pk_constraints()` / `drop_all_indexes_on_table()`) only dropping the foreign keys *pointing at* the model's table, never the ones the table itself declares. An outbound key survived the macro and kept blocking a truncate or rebuild of the table it referenced. Both directions are now dropped, still scoped to the model's own schema. [#632](https://github.com/dbt-msft/dbt-sqlserver/issues/632) - Apply the existing `information_schema_hints()` (`with (nolock)`) to the read-only catalog lookups that were missing it: `get_relation_last_modified` (reached by `dbt source freshness` on a source with no `loaded_at_field`), the foreign-key and primary-key introspection in `drop_fk_constraints` / `drop_pk_constraints` / `drop_fk_references`, `list_nonclustered_rowstore_indexes`, and `find_references`. SQL Server holds transaction-scoped exclusive locks on the system catalog for the duration of any `CREATE`/`ALTER`/`DROP`, so an un-hinted catalog read from dbt waits on every other writer in the database — a second dbt project, an ETL tool, a DBA mid-deployment — and since the default `LOCK_TIMEOUT` is `-1` it waits indefinitely, with dbt appearing to hang and no error. Catalog reads that act as correctness guards rather than discovery (the schema-exists check before `CREATE SCHEMA`, the principal-exists check in `apply_grants`, and the `dbt_full_refresh_incomplete` extended-property marker) are deliberately left un-hinted, since a dirty read there would weaken the guard. - **Behavior change:** `SET XACT_ABORT ON` is now a session-level default on every connection the adapter opens, fixing silent, committed data loss on the DML table refresh path (and any other multi-statement batch): a mid-batch run-time error (e.g. a `NOT NULL`/constraint violation on the swap `INSERT`) previously aborted only the failing statement, not the batch, so a trailing `COMMIT` could still commit a partial result — most notably an emptied target after the `DELETE` succeeded and the `INSERT` failed. This is on by default starting in this release; opt out per profile with `xact_abort: false` if you rely on continue-on-error batch semantics in custom hooks. [#718](https://github.com/dbt-msft/dbt-sqlserver/issues/718)