Fix BEGIN/COMMIT/ROLLBACK silently failing under server cursor mode

web/pgadmin/tools/sqleditor/__init__.py

@@ -1150,7 +1150,8 @@ def poll(trans_id):
                 'transaction_status': transaction_status,
                 'explain_query_length':
                 get_explain_query_length(conn._Connection__async_cursor._query)
-                if conn._Connection__async_cursor else 0
+                if conn._Connection__async_cursor and
+                conn._Connection__async_cursor._query else 0
             }
             return internal_server_error(result, query_len_data)
         elif status == ASYNC_OK:

web/pgadmin/tools/sqleditor/tests/test_poll_explain_query_length_guard.py

@@ -0,0 +1,90 @@
+##########################################################################
+#
+# pgAdmin 4 - PostgreSQL Tools
+#
+# Copyright (C) 2013 - 2026, The pgAdmin Development Team
+# This software is released under the PostgreSQL Licence
+#
+##########################################################################
+
+"""Regression test for a review comment on PR #10321 (pgAdmin issue
+#8991): poll()'s error-handling branch built the 'explain_query_length'
+value with::
+
+    get_explain_query_length(conn._Connection__async_cursor._query)
+    if conn._Connection__async_cursor else 0
+
+which only guarded against the cached async cursor itself being falsy,
+not against its ``_query`` attribute being ``None``. PR #10321's own fix
+runs BEGIN/COMMIT/ROLLBACK through a throwaway plain cursor under
+"server cursor" mode; once that has happened the cached async cursor
+that poll() sees next can be a cursor that has not yet executed a real
+statement, so ``_query`` is still ``None``. get_explain_query_length()
+then immediately does ``query_obj.query.decode()``, and with
+``query_obj`` being ``None`` that crashes with::
+
+    AttributeError: 'NoneType' object has no attribute 'query'
+
+turning any query error that follows a commit under "server cursor"
+mode into an unhandled 500 and leaving the Query Tool unusable, instead
+of the normal JSON error response."""
+
+import json
+import secrets
+from unittest.mock import MagicMock, patch
+
+from pgadmin.utils.route import BaseTestGenerator
+
+
+class TestPollExplainQueryLengthGuard(BaseTestGenerator):
+    """poll() must not crash while building 'explain_query_length' when
+    the cached async cursor has not yet executed any statement."""
+
+    scenarios = [
+        ('Cached async cursor has not executed a statement yet '
+         '(_query is None) - poll() must not crash', dict())
+    ]
+
+    def runTest(self):
+        trans_id = secrets.choice(range(1, 9999999))
+
+        # A cursor left over from execute_void()'s throwaway plain
+        # cursor (or a freshly (re)created server-side cursor) that has
+        # not executed a real statement yet - exactly the state PR
+        # #10321's own fix can leave behind after a commit under
+        # "server cursor" mode.
+        async_cursor = MagicMock()
+        async_cursor._query = None
+
+        conn = MagicMock()
+        conn.poll.return_value = (False, 'some query error')
+        conn.connected.return_value = True
+        conn.messages.return_value = []
+        conn.transaction_status.return_value = 0
+        conn._Connection__async_cursor = async_cursor
+
+        trans_obj = MagicMock()
+        trans_obj.get_thread_native_id.return_value = None
+
+        session_obj = {}
+
+        with patch(
+            'pgadmin.tools.sqleditor.check_transaction_status',
+            return_value=(True, None, conn, trans_obj, session_obj)
+        ):
+            response = self.tester.get(
+                '/sqleditor/poll/{0}'.format(trans_id))
+
+        # Before the fix this either raised AttributeError outright, or
+        # (via the app's generic exception handler) came back as a 500
+        # whose errormsg was the raw AttributeError text instead of the
+        # intended query-error response.
+        response_text = response.data.decode('utf-8')
+        self.assertNotIn(
+            "'NoneType' object has no attribute 'query'", response_text)
+
+        response_data = json.loads(response_text)
+        self.assertEqual(response.status_code, 500)
+        self.assertEqual(response_data['errormsg'], 'some query error')
+        self.assertEqual(
+            response_data['data']['explain_query_length'], 0)

web/pgadmin/utils/driver/psycopg3/connection.py

@@ -14,6 +14,7 @@
 """
 
 import os
+import re
 import secrets
 import datetime
 import asyncio
@@ -59,6 +60,73 @@
 configure_driver_encodings(encodings)
 
 
+# Statements that leave no result set behind for poll() to report on, and
+# which therefore need the cursor they ran on to become the async cursor.
+# ROLLBACK covers ROLLBACK TO SAVEPOINT as well, and START covers START
+# TRANSACTION, there being no other START in the grammar.
+TRANSACTION_CONTROL_KEYWORDS = frozenset({
+    'abort', 'begin', 'commit', 'end', 'release', 'rollback', 'savepoint',
+    'start'
+})
+
+
+# A leading identifier or keyword, as PostgreSQL's lexer reads one.
+_LEADING_WORD = re.compile(r'[^\W\d][\w$]*')
+
+
+def _skip_leading_comments(query):
+    """
+    Return the given statement with any leading whitespace and SQL comments
+    removed. Both -- line comments and /* */ block comments are skipped,
+    the latter nesting as they do in PostgreSQL.
+
+    Args:
+        query: SQL statement
+    """
+    pos = 0
+    length = len(query)
+
+    while pos < length:
+        if query[pos].isspace():
+            pos += 1
+        elif query.startswith('--', pos):
+            newline = query.find('\n', pos)
+            pos = length if newline == -1 else newline + 1
+        elif query.startswith('/*', pos):
+            depth = 1
+            pos += 2
+            while pos < length and depth:
+                if query.startswith('/*', pos):
+                    depth += 1
+                    pos += 2
+                elif query.startswith('*/', pos):
+                    depth -= 1
+                    pos += 2
+                else:
+                    pos += 1
+        else:
+            break
+
+    return query[pos:]
+
+
+def _is_transaction_control(query):
+    """
+    Report whether the given statement is a transaction-control statement,
+    judged by its leading keyword once any leading comments are skipped.
+
+    Args:
+        query: SQL statement, as passed to execute_void()
+    """
+    # Take the keyword as a whole word, so that a comment or semicolon
+    # written straight after it (COMMIT/* note */; or COMMIT;-- note) does
+    # not become part of it, whilst BEGINNING is still not BEGIN.
+    match = _LEADING_WORD.match(_skip_leading_comments(query))
+
+    return bool(match) and \
+        match.group().lower() in TRANSACTION_CONTROL_KEYWORDS
+
+
 class Connection(BaseConnection):
     """
     class Connection(object)
@@ -1173,6 +1241,39 @@ def execute_void(self, query, params=None, formatted_exception_msg=False):
 
         if not status:
             return False, str(cur)
+
+        if isinstance(cur, AsyncDictServerCursor):
+            # A named/server-side cursor's execute() always runs the query
+            # as `DECLARE ... CURSOR FOR <query>`, which cannot express a
+            # transaction-control statement such as BEGIN/COMMIT/ROLLBACK.
+            # Run this one statement through a throwaway plain cursor
+            # instead, leaving the cursor cached for the connection in
+            # place for the next query to reuse. The connection's
+            # cursor_factory is AsyncDictCursor, so the throwaway carries
+            # ordered_description(), get_rowcount() and the rest of the
+            # API poll() calls.
+            cur = self.conn.cursor()
+
+            if _is_transaction_control(query):
+                # For a transaction-control statement the throwaway also
+                # has to become the async cursor, because poll() and
+                # status_message() report on that rather than on whatever
+                # this call used: the cached server-side cursor still
+                # describes the previous query and reports itself open, so
+                # a following poll() would read straight past its "not cur
+                # or cur.closed" guard and put that query's column metadata
+                # and row count back over the "no result set" the
+                # statement leaves behind.
+                #
+                # Anything else keeps the cached cursor as the async
+                # cursor. A statement such as the SELECT pg_cancel_backend()
+                # issued by cancel_transaction() has no business detaching
+                # the cursor a result set is still being paged or
+                # downloaded from.
+                self.__async_cursor = cur
+                self.column_info = None
+                self.row_count = 0
+
         query_id = str(secrets.choice(range(1, 9999999)))
 
         current_app.logger.log(