fix: don't let a spurious mssql-python driver error during trailing drain fail a successful batch - #815
Conversation
…l-python trailing-drain error Reported: on the mssql-python backend, with dbt_sqlserver_use_dbt_transactions: false and threads > 1, a successful multi-statement batch (e.g. the delete+insert incremental strategy, or a table_refresh_method: dml swap) could fail the run with: Driver Error: Syntax error or access violation; DDBC Error: [Microsoft][SQL Server]New transaction is not allowed because there are other threads running in the session. The failure traced to execute()'s trailing nextset() drain, which runs after get_response(cursor) has already read the batch's own response. This matches an open upstream mssql-python defect (microsoft/mssql-python#229): a multi-statement batch that turns SET NOCOUNT back off before its last statement -- which sqlserver__get_delete_insert_merge_sql does deliberately, so that statement's rowcount is reported -- leaves a DONE_IN_PROC token the driver's nextset() handling does not always walk cleanly. _drain_trailing_results swallows only that exact error (duck-typed on exception class and message, the same way _try_drain_nextset already duck-types ADBC's NotSupportedError). nextset() can also carry a genuine, deferred error from a later statement in the same batch -- SQL Server's deferred name resolution lets `CREATE VIEW ... AS SELECT bad_column FROM t` succeed at create time, so a later statement querying that view (e.g. sqlserver__create_table_as's SELECT * INTO) only fails once nextset() walks to it -- so every other error still fails the build exactly as before. Verified with a fake mssql-python cursor: the known-spurious message no longer fails execute(), while a different error (mirroring test_concurrency.py's deliberately-broken model) still raises.
e02f15f to
ee30264
Compare
|
Closing — investigated further and this was the wrong fix. The error text ("New transaction is not allowed because there are other threads running in the session") is SQL Server engine error 3988 ( My original root-cause citation here (microsoft/mssql-python#229) was wrong — that PR is about local temp-table visibility across separate The real guidance for this case is already in #790: use |
Report
An incremental
delete+insertmodel failed even though the SQL itself succeeded. The failure traced toexecute()'s trailingnextset()drain — which runs afterexecute()had already read the batch's response (cursor.rowcount). Forcing--threads 1avoided it.Root cause
microsoft/mssql-python#229 (open upstream): a multi-statement batch that turns
SET NOCOUNTback off before its last statement — whichsqlserver__get_delete_insert_merge_sqldoes deliberately, so that statement's rowcount is reported — leaves aDONE_IN_PROCtoken the driver'snextset()handling does not always walk cleanly. Could not force the exact driver-side race locally; reproduced deterministically instead with a fake cursor.Fix
_drain_trailing_resultsswallows only that exact error (duck-typed on exception class + message, same pattern_try_drain_nextsetalready uses for ADBC'sNotSupportedError). This is a hard requirement, not a stylistic choice:nextset()can also carry a genuine, deferred error from a later statement in the same batch — SQL Server's deferred name resolution letsCREATE VIEW ... AS SELECT bad_column FROM tsucceed at create time, so a later statement querying that view (e.g.sqlserver__create_table_as'sSELECT * INTO) only fails oncenextset()walks to it, not on the initialexecute(). An earlier version of this fix broadly swallowed everynextset()exception and was caught bytests/functional/adapter/dbt/test_concurrency.py::TestConcurrency::test_concurrency, which builds a model selecting a nonexistent column and asserts the build fails — it silently reported success instead.Testing
test_execute_survives_a_spurious_mssql_python_error_during_trailing_drain— the known-spurious message no longer failsexecute().test_execute_still_raises_a_genuine_deferred_error_during_trailing_drain— a different error still raises (regression guard for the mistake above).test_concurrency.py,test_transactions.py,test_concurrent_incremental.py,test_materialize_change.py,test_constraints.pyall pass locally.