backport(1.11): drop the model's outbound foreign keys too (#632)

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)
 

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('')

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