Fix/undrained probe cursor rollback - #810
Merged
axellpadilla merged 4 commits intoAug 6, 2026
Merged
Conversation
…he build get_column_schema_from_query wants a query's column shape and gets it by executing the query, reading cursor.description, then returning 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 the cancel arrives as an attention. Every connection runs SET XACT_ABORT ON (dbt-msft#718), under which SQL Server answers an attention by rolling back the open transaction. None of that raises, so the damage surfaces later and elsewhere: a snapshot loses the staging table it built moments earlier in the same transaction and fails with `Invalid object name '..._dbt_tmp'`, and a contract-enforced model silently loses its in-transaction pre_hook writes. Only queries opening with a CTE were exposed. Anything else is wrapped as `select * from (...) where 1 = 0` by sqlserver__get_empty_subquery_sql and returns nothing, 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, while everything else stayed quiet. The size of the result set is not the trigger; the timing is. The cancel only raises an attention while the request is still in flight, so a few hundred rows at zero client delay is enough, while ~10ms of client work before the close makes even 20MB safe. Existing coverage exercised these paths with tens of narrow rows and so never tripped it -- the snapshot fixtures in test_transactions.py are now sized past the boundary, and the previously untested contract + CTE combination is covered too. _discard_pending_results lives next to _try_drain_nextset rather than introducing a second draining idiom, and is applied to _get_row_count as well, which leaked its cursor on every expand_column_types call. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
sqlserver__get_empty_subquery_sql neuters a probe query as `select * from (...) where 1 = 0`, but a query opening with a CTE cannot be wrapped that way and is passed through untouched, so it ran in full purely to have its column names read. A snapshot executed its whole staging query once for the probe and again to build the staging table; a contract-enforced model ran twice per build. Draining that result set (previous commit) makes it safe but not cheap, and the cost tracks data volume. sp_describe_first_result_set compiles the query and reports its shape without scanning, which is all this method ever wanted. It is already how sqlserver__get_columns_in_query handles CTEs (dbt-msft#698). The macro cannot be the fix point: its output is also embedded in a CREATE VIEW body by dbt-core's unit-test materialization, where only a bare SELECT is legal, and reading cursor.description of an `exec sp_describe...` batch would describe the procedure's own result shape rather than the query's. Reported names and types are unchanged. Reading cursor.description reports Python classes, which collapse whole type families -- every integer width arrives as int, every string type as varchar -- so the describe path is mapped back onto exactly those names and fed through Column.create as before, leaving the dbt_sqlserver_use_native_string_types flag working without the mapping knowing it exists. Rather than guess, the describe path declines and lets the caller execute whenever it cannot guarantee agreement: the adbc backend (whose type names derive from Arrow codes and would disagree), a type outside the mapping, a query sp_describe_first_result_set refuses to describe such as one reading a #temp table, or a describe that returns nothing. Every gap degrades to slower, never to silently different. TestCteProbeAvoidsExecution pins both halves: that a CTE-headed probe no longer executes its query, and that the two branches agree across 22 types. The second is a guard rather than a symptom and passes either way -- it is what makes the mapping safe to maintain. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Two ways the column probe reported a type the driver would not have. The describe path (previous commit) maps sp_describe_first_result_set's system_type_name back onto the name reading cursor.description yields, but that name is the *driver's* choice of Python class, not SQL Server's, and the two ODBC backends do not agree on three of them. mssql-python decodes uniqueidentifier as uuid.UUID, datetimeoffset as datetime and sql_variant as str where pyodbc hands back str, bytearray and bytearray, so one fixed table could not serve both: a CTE-headed model with a uniqueidentifier column described as varchar and executed as uniqueidentifier, and its contract comparison shifted under it. The lookup now follows the backend in use. Two entries resist tabulation and say so rather than guess. datetimeoffset on pyodbc depends on whether add_query has registered the -155 output converter yet -- bytearray on a connection's first query, str on every one after -- so it is absent and the query is executed, which agrees with itself by construction. uniqueidentifier on pyodbc depends on the process-global pyodbc.native_uuid flag, which is readable, so it is read at probe time. The adbc backend, which skips the describe path entirely, had gaps of its own in the Arrow maps. tinyint is SQL Server's only unsigned integer and arrives as uint8, uniqueidentifier as the arrow.uuid extension type; neither was mapped, so the probe raised "Unsupported SQL Server type code" and the model failed outright. real arrives as Arrow "float", which is the 4-byte float -- it was mapped to SQL Server's 8-byte float, so a contract declaring real compared against FLOAT and failed. The integer-keyed map is keyed on Arrow type ids but held values matching no Arrow enum: id 3 (int8) said varchar, 7 (int32) said float, 12 (double) said decimal. Nothing reaches it today -- ADBC reports pyarrow DataType objects, which the name-keyed map handles -- so it was silently wrong rather than broken. Both maps are now derived from the same set and pinned against pyarrow itself instead of transcribed ids. Types the ADBC driver cannot represent at all (datetimeoffset, and sql_variant, which arrives as an Arrow struct) fail before the probe is reached, so the functional matrix covers those two on the ODBC backends only.
# Conflicts: # CHANGELOG.md # dbt/adapters/sqlserver/sqlserver_adapter.py
Collaborator
Author
|
@axellpadilla In fixing the bug I ended up coming across more bugs with the type mappings when I tried to unroll the underlying cause that triggered the bug, which was a performance issue. As we maintain 3 drivers now the type mapping code ended up being quite complex. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #809.
get_column_schema_from_queryexecuted a query to read its column shape, then returned without fetching the rows or closing the cursor. Closing that cursor while the server was still streaming made the driver cancel the request, and SQL Server answers that cancel by rolling back the open transaction, because every connection runsSET XACT_ABORT ON(#718). Nothing raised, so snapshots failed a statement later withInvalid object name '..._dbt_tmp'and contract-enforced models silently lost their in-transactionpre_hookwrites.Only CTE-headed queries were exposed — anything else is wrapped as
select * from (...) where 1 = 0and returns no rows.Changes
fix:drain and close probe cursors. Adds_discard_pending_resultsnext to the existing_try_drain_nextset, applied toget_column_schema_from_queryand to the row-count probe inexpand_column_types, which leaked its cursor on every call.perf:describe CTE-headed probes instead of executing them. A CTE-headed query can't be wrapped inwhere 1 = 0, so it ran in full just to have its column names read — a snapshot executed its whole staging query twice per build.sp_describe_first_result_setreports the shape without scanning, which is already howsqlserver__get_columns_in_queryhandles CTEs (#698). Reported names and types are unchanged: the describe path is mapped back onto the coarser namescursor.descriptionproduces, and falls back to executing whenever it can't guarantee that (theadbcbackend, a type outside the mapping, or a query the procedure declines to describe, such as one reading a#temptable). Every gap degrades to slower, never to different.Tests
test_transactions.py— snapshot fixtures sized past the timing boundary, covering bothtimestampandcheckstrategies. The old seed-sized fixtures were why this went unnoticed.test_constraints.py— the previously untested contract + CTE combination.get_column_schema_from_queryandexpand_column_types, plusTestCteProbeAvoidsExecution, which pins that a CTE-headed probe no longer executes its query and that the describe and execute paths agree across 22 types.