I3eka commented on code in PR #45729:
URL: https://github.com/apache/superset/pull/45729#discussion_r4385180556
##########
superset/db_engine_specs/mssql.py:
##########
@@ -165,6 +168,31 @@ class MssqlEngineSpec(BaseEngineSpec):
),
}
+ @classmethod
+ def get_cancel_query_id(cls, cursor: Any, query: Query) -> str | None:
+ """Capture the SQL Server session before executing its query."""
+ if not cls.has_query_id_before_execute:
+ return None
+ cursor.execute("SELECT @@SPID")
+ row = cursor.fetchone()
+ return str(row[0]) if row else None
+
+ @classmethod
+ def cancel_query(cls, cursor: Any, query: Query, cancel_query_id: str) ->
bool:
+ """Terminate the captured session using a separate cancellation
connection."""
+ if not cls.has_query_id_before_execute or not
cls.validate_cancel_query_id(
+ cancel_query_id, r"^[0-9]+$"
+ ):
+ return False
+ try:
+ # KILL cannot run inside the fresh cancellation connection's
transaction.
+ cursor.execute("IF @@TRANCOUNT > 0 ROLLBACK TRANSACTION")
+ cursor.execute(f"KILL {cancel_query_id}")
Review Comment:
Confirmed. SQL Server releases session IDs when a connection ends and may
assign them to another connection ([Microsoft KILL
documentation](https://learn.microsoft.com/en-us/sql/t-sql/language-elements/kill-transact-sql?view=sql-server-ver17)).
A metadata check followed by KILL would still race.
Fixed in 4f2596aa2c by removing both @@SPID capture and KILL, including the
path for previously stored numeric handles. The MCP execution owner instead
uses the original driver's cursor.cancel() where supported (pyodbc documents
this cross-thread API in its [cursor
contract](https://github.com/mkleehammer/pyodbc/blob/master/src/pyodbc.pyi)). A
cursor lifetime lock waits for an in-flight native cancel before releasing the
cursor; a queued callback after release is a no-op.
I narrowed the title and documentation accordingly: this does not add
cross-process SQL Lab/chart-task cancellation, pymssql support, or support for
the Azure Synapse/Sybase subclasses. It needs no KILL privilege. Native
cancellation remains best effort if the driver is unresponsive.
Validation: 11 new regression cases failed before the fix and pass after it,
including stale IDs, a driver without cursor.cancel(), delayed callbacks, and
concurrent cancellation/close. All 132 engine/worker/cancellation tests and all
changed-file pre-commit checks pass. No live SQL Server cancellation or
deployment was performed.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
