FIX: Multi-Statement SQL Enhancement for mssql-python - #229
FIX: Multi-Statement SQL Enhancement for mssql-python#229Arvis Šmats (arvis108) wants to merge 5 commits into
Conversation
|
@microsoft-github-policy-service agree |
gargsaumya
changed the title
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
gargsaumya
left a comment
gargsaumya
left a comment
There was a problem hiding this comment.
Hi Arvis Šmats (@arvis108) ,
Thank you for your contribution and for working on this enhancement! We appreciate the effort you’ve put into addressing this.
I noticed that the PR includes the file PR_SUMMARY.md, which isn’t required for this change. While the PR is currently passing all pipeline checks and everything else looks good, it would be great if we could remove this extra file to keep the PR clean and aligned with our project guidelines.
Let me know if you’d like any help or have questions, we’d be happy to assist!
Thanks again for your work on this!
| @@ -0,0 +1,299 @@ | |||
| # PR Summary: PyODBC-Style Multi-Statement SQL Enhancement for mssql-python | |||
There was a problem hiding this comment.
Can we please remove this file.
There was a problem hiding this comment.
Removed
There was a problem hiding this comment.
Arvis Šmats (@arvis108) - thanks a lot for your contribution to the project, this is indeed a detailed implementation of the fix.
I have added some comments requesting a few more testcases and a minor logger change, also I have identified a expected behaviour deviation of pyodbc with this.
Please fel free to add your inputs in the discussion
| drop_table_if_exists(cursor, "dbo.money_test") | ||
| db_connection.commit() | ||
|
|
||
| def test_multi_statement_query(cursor, db_connection): |
There was a problem hiding this comment.
great testcase, can we add a few more tests targetting the below areas
- multiple result sets with multiple select statements on temp tables with
nextset() - semicolons in b/w string literals to ensure no false positives in buffering logic
- multi-statement batch where the final statement is not a SELECT
There was a problem hiding this comment.
Added the tests you mentioned, feel free to add more
| def test_multi_statement_query(cursor, db_connection): | ||
| """Test multi-statement query with temp tables""" | ||
| try: | ||
| # Single SQL with multiple statements - tests pyODBC-style buffering |
There was a problem hiding this comment.
actually - pyodbc strangely gives out below error with this testcase (technically it shouldn't)
pyodbc.ProgrammingError: No results. Previous SQL was not a query.
just curious for discussion, gargsaumya - if possible, could you please take a look at what pyodbc internally might be doing for this case?
There was a problem hiding this comment.
the simplest repro would be
cursor.execute("""
SELECT 1 as col1, 'test' as col2 INTO #temp1;
SELECT * FROM #temp1;
""")
rows = cursor.fetchall()
print(rows)
pyodbc gives out the error as mentioned in above comment
whereas mssql-python in this PR branch gives out
[(1, 'test')]
There was a problem hiding this comment.
pymssql also gives out [(1, 'test')] with your query
There was a problem hiding this comment.
You are right about pyodbc, I am getting same error now. Looks like when I was testing, I had "SET NOCOUNT ON" set for the session. I am sorry.
There was a problem hiding this comment.
No worries! Since we have a few higher-priority items in progress, we’ll circle back to testing and reviewing this fix a bit later so we can get it merged. Please let us know if this PR is blocking you in any way. Really appreciate your continued efforts to improve the driver.
There was a problem hiding this comment.
PR is not blocking me, no worries
8ee1a36 to
2fbdab4
Compare
…uccessful batch (cherry picked in spirit from 9161fce on fix/814-mssql-python-trailing-drain-transaction-error; not a literal cherry-pick since v1.11's execute() predates _try_drain_nextset existing here at all -- it drains with a raw `while cursor.nextset(): pass` loop instead, which propagates a failure exactly the same way.) Reported against dbt-sqlserver 1.12.0rc2 / mssql-python 1.12.0: an incremental delete+insert model failed 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. with dbt_sqlserver_use_dbt_transactions: false and threads > 1 on the mssql-python backend. The batch itself succeeded -- the failure traced to execute()'s trailing nextset() drain, i.e. after execute() had already read the batch's response via get_response(cursor). Forcing threads=1 avoided it. The same delete+insert strategy and the same connection-manager shape exist unchanged on this branch, so the bug applies here too. Could not reproduce the exact driver-side condition locally (see the fuller investigation notes on the master-branch fix); microsoft/mssql-python#229 (open upstream) describes the mechanism: a multi-statement batch that turns SET NOCOUNT back off before its final statement -- exactly what sqlserver__get_delete_insert_merge_sql does, so that statement's rowcount is reported -- leaves a DONE_IN_PROC token the driver's nextset() handling does not always walk cleanly. execute() already read cursor.rowcount into its response before any of this runs, so nothing about a build's outcome depends on successfully walking the rest of the DONE chain. _drain_trailing_results wraps that walk the same way _discard_pending_results already treats a probe cursor's own trailing results -- logged and swallowed, not propagated. Narrower than that: it only wraps the walk that runs after the response has been captured, not the fetch=True loop still looking for the first real result set. Verified with a fake cursor reproducing the exact reported message -- test_execute_survives_a_spurious_mssql_python_error_during_trailing_drain.
…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.
…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. (cherry picked from commit ee30264)
3 participants
Work Item / Issue Reference
Summary
FEAT: Multi-statement SQL support with temp tables
This PR fixes an issue where multi-statement SQL queries (especially those using temporary tables) executed successfully but returned empty result sets in
mssql-python, unlike SSMS and pyodbc.The solution follows pyodbc’s proven approach by automatically applying
SET NOCOUNT ONto multi-statement queries. This preventsDONE_IN_PROCinterference, ensuring correct results without breaking existing functionality.Implementation Highlights
SET NOCOUNT ONinjection incursor.pyBenefits