From 95111ccdd8b6c5661c4baf42e67d139eed87426e Mon Sep 17 00:00:00 2001 From: Dave Page Date: Wed, 19 Aug 2026 12:39:43 +0100 Subject: [PATCH 1/6] fix: run BEGIN/COMMIT/ROLLBACK on a plain cursor under server cursor mode (#8991) execute_void() blindly reused whatever cursor was cached for the connection, which under "server cursor" mode is the named/server-side AsyncDictServerCursor left over from the last SELECT. A named cursor's execute() always wraps the statement as `DECLARE ... CURSOR FOR `, which cannot express a transaction-control statement, so BEGIN/COMMIT/ROLLBACK silently failed (failing one step earlier still, on a `prepare` keyword the server-side cursor's execute() doesn't accept at all) and the exception was swallowed by the background query thread. The transaction was therefore never actually committed or rolled back, and the next poll() picked up the previous query's leftover column info, which is what made the result grid appear instead of the Messages tab. Run the statement through a throwaway plain cursor instead, leaving the cached server-side cursor untouched, and clear the stale column info so poll() correctly reports no result set. --- .../utils/driver/psycopg3/connection.py | 13 +++ .../tests/test_execute_void_server_cursor.py | 85 +++++++++++++++++++ 2 files changed, 98 insertions(+) create mode 100644 web/pgadmin/utils/driver/psycopg3/tests/test_execute_void_server_cursor.py diff --git a/web/pgadmin/utils/driver/psycopg3/connection.py b/web/pgadmin/utils/driver/psycopg3/connection.py index d07a16cefcd..d8a6cd53172 100644 --- a/web/pgadmin/utils/driver/psycopg3/connection.py +++ b/web/pgadmin/utils/driver/psycopg3/connection.py @@ -1173,6 +1173,19 @@ 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 `, which cannot express a + # transaction-control statement such as BEGIN/COMMIT/ROLLBACK. + # Run this one statement through a throwaway plain cursor + # instead, leaving the cached server-side cursor untouched, and + # treat it as leaving no result set for whatever poll() call + # comes next. + cur = self.conn.cursor() + self.column_info = None + self.row_count = 0 + query_id = str(secrets.choice(range(1, 9999999))) current_app.logger.log( diff --git a/web/pgadmin/utils/driver/psycopg3/tests/test_execute_void_server_cursor.py b/web/pgadmin/utils/driver/psycopg3/tests/test_execute_void_server_cursor.py new file mode 100644 index 00000000000..c885f66df8e --- /dev/null +++ b/web/pgadmin/utils/driver/psycopg3/tests/test_execute_void_server_cursor.py @@ -0,0 +1,85 @@ +########################################################################## +# +# pgAdmin 4 - PostgreSQL Tools +# +# Copyright (C) 2013 - 2026, The pgAdmin Development Team +# This software is released under the PostgreSQL Licence +# +########################################################################## + +"""Regression test: ``execute_void()`` must not run a transaction-control +statement (BEGIN/COMMIT/ROLLBACK) through a cached named/server-side +cursor. + +A named cursor's ``execute()`` always wraps the statement as +``DECLARE ... CURSOR FOR ``, which cannot express BEGIN/COMMIT/ +ROLLBACK. Before the fix, the Commit/Rollback buttons under "server +cursor" mode silently did nothing: the DECLARE-wrapped call failed +(actually failing one step earlier, on a ``prepare`` keyword the +server-side cursor's ``execute()`` doesn't accept at all), the exception +was swallowed by the background query thread, and the next poll() then +reported the *previous* query's leftover column info, making the result +grid appear instead of the Messages tab (pgAdmin issue #8991).""" + +from unittest.mock import MagicMock, patch + +from pgadmin.utils.driver.psycopg3.connection import Connection +from pgadmin.utils.driver.psycopg3.cursor import AsyncDictServerCursor +from pgadmin.utils.route import BaseTestGenerator + + +class ExecuteVoidServerCursorTest(BaseTestGenerator): + + scenarios = [ + ('COMMIT with a cached server-side cursor runs on a throwaway ' + 'plain cursor and clears stale column info', dict(sql='COMMIT;')), + ('ROLLBACK with a cached server-side cursor runs on a throwaway ' + 'plain cursor and clears stale column info', + dict(sql='ROLLBACK;')), + ] + + def runTest(self): + manager = MagicMock(sid=1) + conn = Connection(manager, 'test-conn-id', 'testdb') + conn.python_encoding = 'utf-8' + + # Leftover state from a previous SELECT executed through the + # server-side cursor. + conn.column_info = [{'name': 'x'}] + conn.row_count = 1 + + server_cursor = MagicMock(spec=AsyncDictServerCursor) + server_cursor.closed = False + + plain_cursor = MagicMock() + plain_cursor.closed = False + + conn.conn = MagicMock() + conn.conn.cursor.return_value = plain_cursor + conn.conn.info.user = 'postgres' + conn.conn.info.host = 'localhost' + conn.conn.info.dbname = 'testdb' + + # current_user needs a real request context to resolve at all; + # patch it only once inside that context, to a stand-in with the + # attribute execute_void()'s log line reads. + with self.app.test_request_context(): + with patch( + 'pgadmin.utils.driver.psycopg3.connection.current_user', + MagicMock(email='test@example.com') + ), patch.object(Connection, '_Connection__cursor', + return_value=(True, server_cursor)): + status, result = conn.execute_void(self.sql) + + self.assertTrue(status) + self.assertIsNone(result) + + # The statement ran on the throwaway plain cursor, not the + # cached server-side one. + plain_cursor.execute.assert_called_once() + server_cursor.execute.assert_not_called() + + # Stale result-set state from the prior SELECT must not leak + # into whatever poll() call comes next. + self.assertIsNone(conn.column_info) + self.assertEqual(conn.row_count, 0) From 8072ca2bc15b4650f9c02aaec5db75ff85cb4b30 Mon Sep 17 00:00:00 2001 From: Dave Page Date: Tue, 25 Aug 2026 10:07:48 +0100 Subject: [PATCH 2/6] fix: guard explain_query_length against an async cursor with no query yet Under server cursor mode, execute_void() running BEGIN/COMMIT/ROLLBACK on a throwaway plain cursor can leave the cached async cursor pointing at a cursor that has not executed a real statement yet, so its _query attribute is still None. poll()'s error path called get_explain_query_length() on that None unconditionally, crashing with AttributeError: 'NoneType' object has no attribute 'query' on the next query error and leaving the Query Tool unusable, instead of returning the intended JSON error response. --- web/pgadmin/tools/sqleditor/__init__.py | 3 +- .../test_poll_explain_query_length_guard.py | 90 +++++++++++++++++++ 2 files changed, 92 insertions(+), 1 deletion(-) create mode 100644 web/pgadmin/tools/sqleditor/tests/test_poll_explain_query_length_guard.py diff --git a/web/pgadmin/tools/sqleditor/__init__.py b/web/pgadmin/tools/sqleditor/__init__.py index 8080d220a54..8cdc44bea7d 100644 --- a/web/pgadmin/tools/sqleditor/__init__.py +++ b/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: diff --git a/web/pgadmin/tools/sqleditor/tests/test_poll_explain_query_length_guard.py b/web/pgadmin/tools/sqleditor/tests/test_poll_explain_query_length_guard.py new file mode 100644 index 00000000000..7286af05a7b --- /dev/null +++ b/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) From 5f84ba8495d258f67d9658de73cb41006577023c Mon Sep 17 00:00:00 2001 From: Dave Page Date: Thu, 3 Sep 2026 11:17:23 +0100 Subject: [PATCH 3/6] fix: point the async cursor at the throwaway transaction-control cursor Clearing column_info and row_count when execute_void() diverts BEGIN/COMMIT/ROLLBACK onto a throwaway plain cursor was not enough on its own, because poll() rebuilds both from self.__async_cursor, and that was still the cached server-side cursor from the previous SELECT. It reports itself open, so poll() walked past its "not cur or cur.closed" guard and restored the previous query's column metadata and row count over the "no result set" the transaction-control statement had just left behind, which is the same stale state that made the result grid appear in place of the Messages tab. Make the throwaway cursor the async cursor as well. The connection's cursor_factory is AsyncDictCursor, so it carries ordered_description(), get_rowcount() and the rest of the API poll() calls, and it describes the statement that actually ran: poll() therefore reports no columns and no rows, and status_message() reports COMMIT or ROLLBACK rather than the previous query's message. The cursor cached for the connection is left alone, so the next query still reuses it. --- .../utils/driver/psycopg3/connection.py | 17 ++++- .../tests/test_execute_void_server_cursor.py | 63 +++++++++++++++++-- 2 files changed, 73 insertions(+), 7 deletions(-) diff --git a/web/pgadmin/utils/driver/psycopg3/connection.py b/web/pgadmin/utils/driver/psycopg3/connection.py index d8a6cd53172..1a5b66f1974 100644 --- a/web/pgadmin/utils/driver/psycopg3/connection.py +++ b/web/pgadmin/utils/driver/psycopg3/connection.py @@ -1179,10 +1179,21 @@ def execute_void(self, query, params=None, formatted_exception_msg=False): # as `DECLARE ... CURSOR FOR `, which cannot express a # transaction-control statement such as BEGIN/COMMIT/ROLLBACK. # Run this one statement through a throwaway plain cursor - # instead, leaving the cached server-side cursor untouched, and - # treat it as leaving no result set for whatever poll() call - # comes next. + # instead, leaving the cursor cached for the connection in + # place for the next query to reuse. cur = self.conn.cursor() + # 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" a transaction + # control statement leaves behind. The connection's + # cursor_factory is AsyncDictCursor, so the throwaway carries + # ordered_description(), get_rowcount() and the rest of the + # API poll() calls. + self.__async_cursor = cur self.column_info = None self.row_count = 0 diff --git a/web/pgadmin/utils/driver/psycopg3/tests/test_execute_void_server_cursor.py b/web/pgadmin/utils/driver/psycopg3/tests/test_execute_void_server_cursor.py index c885f66df8e..99d6151087a 100644 --- a/web/pgadmin/utils/driver/psycopg3/tests/test_execute_void_server_cursor.py +++ b/web/pgadmin/utils/driver/psycopg3/tests/test_execute_void_server_cursor.py @@ -19,7 +19,16 @@ server-side cursor's ``execute()`` doesn't accept at all), the exception was swallowed by the background query thread, and the next poll() then reported the *previous* query's leftover column info, making the result -grid appear instead of the Messages tab (pgAdmin issue #8991).""" +grid appear instead of the Messages tab (pgAdmin issue #8991). + +Clearing ``column_info``/``row_count`` in ``execute_void()`` is not enough +on its own, because ``poll()`` rebuilds both from whatever +``self.__async_cursor`` points at, and that is still the cached +server-side cursor: it reports itself open, so the ``not cur or +cur.closed`` guard lets it through and the previous query's metadata comes +straight back. The throwaway cursor therefore has to become the async +cursor as well, which also makes ``status_message()`` report the +transaction-control statement rather than the previous query.""" from unittest.mock import MagicMock, patch @@ -32,9 +41,10 @@ class ExecuteVoidServerCursorTest(BaseTestGenerator): scenarios = [ ('COMMIT with a cached server-side cursor runs on a throwaway ' - 'plain cursor and clears stale column info', dict(sql='COMMIT;')), + 'plain cursor, and a following poll() reports no result set', + dict(sql='COMMIT;')), ('ROLLBACK with a cached server-side cursor runs on a throwaway ' - 'plain cursor and clears stale column info', + 'plain cursor, and a following poll() reports no result set', dict(sql='ROLLBACK;')), ] @@ -48,17 +58,43 @@ def runTest(self): conn.column_info = [{'name': 'x'}] conn.row_count = 1 + # The cursor the previous SELECT ran on, which is both cached for + # the connection and still referenced as the async cursor. It + # reports itself open, and still describes that SELECT's result. + stale_column = MagicMock() + stale_column.to_dict.return_value = {'name': 'x'} server_cursor = MagicMock(spec=AsyncDictServerCursor) server_cursor.closed = False - + server_cursor.description = [stale_column] + server_cursor.ordered_description.return_value = [stale_column] + # AsyncDictServerCursor.get_rowcount() answers 1 unconditionally. + server_cursor.get_rowcount.return_value = 1 + server_cursor.nextset.return_value = None + server_cursor.statusmessage = 'SELECT 1' + conn._Connection__async_cursor = server_cursor + + # The throwaway cursor execute_void() should use instead. A + # transaction-control statement leaves no result set behind, so it + # has no description and no rows. plain_cursor = MagicMock() plain_cursor.closed = False + # Values taken from what psycopg actually leaves on the cursor + # after a COMMIT/ROLLBACK: no description, and a result with no + # tuples in it, which AsyncDictCursor.get_rowcount() reports as 0. + plain_cursor.description = None + plain_cursor.get_rowcount.return_value = 0 + plain_cursor.nextset.return_value = None + plain_cursor.statusmessage = self.sql.rstrip(';') conn.conn = MagicMock() conn.conn.cursor.return_value = plain_cursor conn.conn.info.user = 'postgres' conn.conn.info.host = 'localhost' conn.conn.info.dbname = 'testdb' + # Not ACTIVE, and no connection level error, so poll() gets as far + # as reading the cursor rather than answering from either of those. + conn.conn.info.transaction_status = 2 + conn.conn.pgconn.error_message = None # current_user needs a real request context to resolve at all; # patch it only once inside that context, to a stand-in with the @@ -83,3 +119,22 @@ def runTest(self): # into whatever poll() call comes next. self.assertIsNone(conn.column_info) self.assertEqual(conn.row_count, 0) + + # ... and the poll() that the Query Tool makes next must not put it + # back. This is the call that made the result grid appear instead + # of the Messages tab, because it rebuilds column_info and + # row_count from the async cursor, which was still the server-side + # one describing the previous SELECT. + with self.app.test_request_context(): + status, result = conn.poll(no_result=True) + status_message = conn.status_message() + + self.assertEqual(status, 1) + self.assertIsNone(result) + self.assertIsNone(conn.column_info) + self.assertEqual(conn.row_count, 0) + server_cursor.ordered_description.assert_not_called() + + # The status message belongs to the statement just run, not to the + # previous query. + self.assertEqual(status_message, self.sql.rstrip(';')) From 9019962ed90efa8bb6bb5ba43675261ae8f4c5d5 Mon Sep 17 00:00:00 2001 From: Dave Page Date: Mon, 14 Sep 2026 10:24:17 +0100 Subject: [PATCH 4/6] Only detach the async cursor for transaction-control statements execute_void() diverts onto a throwaway plain cursor whenever the cursor cached for the connection is a server-side one, since a named cursor can only execute through DECLARE ... CURSOR FOR, and it then made that throwaway the async cursor so that the following poll() and status_message() report on the statement that actually ran. The second half of that was applied to every statement rather than only to the ones that need it, so a statement such as the SELECT pg_cancel_backend() that cancel_transaction() issues could detach the cursor a result set was still being paged or downloaded from, leaving the pagination and download calls that follow reading a throwaway with no rows in it. The throwaway is still used for any statement, because a server-side cursor cannot run one directly, but it now becomes the async cursor only for a transaction-control statement, judged by the leading keyword. Those are the statements that leave no result set behind and whose result the Query Tool polls for immediately afterwards. Raised by CodeRabbit on #10321. --- .../utils/driver/psycopg3/connection.py | 58 ++++++++--- .../tests/test_execute_void_server_cursor.py | 98 ++++++++++++++++++- 2 files changed, 141 insertions(+), 15 deletions(-) diff --git a/web/pgadmin/utils/driver/psycopg3/connection.py b/web/pgadmin/utils/driver/psycopg3/connection.py index 1a5b66f1974..14319ab150b 100644 --- a/web/pgadmin/utils/driver/psycopg3/connection.py +++ b/web/pgadmin/utils/driver/psycopg3/connection.py @@ -59,6 +59,29 @@ 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' +}) + + +def _is_transaction_control(query): + """ + Report whether the given statement is a transaction-control statement, + judged by its leading keyword. + + Args: + query: SQL statement, as passed to execute_void() + """ + keyword = query.strip().split(None, 1)[0] if query.strip() else '' + + return keyword.rstrip(';').lower() in TRANSACTION_CONTROL_KEYWORDS + + class Connection(BaseConnection): """ class Connection(object) @@ -1180,22 +1203,31 @@ def execute_void(self, query, params=None, formatted_exception_msg=False): # 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. - cur = self.conn.cursor() - # 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" a transaction - # control statement leaves behind. The connection's + # 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. - self.__async_cursor = cur - self.column_info = None - self.row_count = 0 + 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))) diff --git a/web/pgadmin/utils/driver/psycopg3/tests/test_execute_void_server_cursor.py b/web/pgadmin/utils/driver/psycopg3/tests/test_execute_void_server_cursor.py index 99d6151087a..8da1e73d3c5 100644 --- a/web/pgadmin/utils/driver/psycopg3/tests/test_execute_void_server_cursor.py +++ b/web/pgadmin/utils/driver/psycopg3/tests/test_execute_void_server_cursor.py @@ -28,11 +28,15 @@ cur.closed`` guard lets it through and the previous query's metadata comes straight back. The throwaway cursor therefore has to become the async cursor as well, which also makes ``status_message()`` report the -transaction-control statement rather than the previous query.""" +transaction-control statement rather than the previous query. That +promotion is limited to transaction-control statements, for the reason +given in ``ExecuteVoidNonTransactionServerCursorTest`` below.""" from unittest.mock import MagicMock, patch -from pgadmin.utils.driver.psycopg3.connection import Connection +from pgadmin.utils.driver.psycopg3.connection import ( + Connection, _is_transaction_control +) from pgadmin.utils.driver.psycopg3.cursor import AsyncDictServerCursor from pgadmin.utils.route import BaseTestGenerator @@ -138,3 +142,93 @@ def runTest(self): # The status message belongs to the statement just run, not to the # previous query. self.assertEqual(status_message, self.sql.rstrip(';')) + + +class ExecuteVoidNonTransactionServerCursorTest(BaseTestGenerator): + """A statement that is not transaction control must leave the cached + server-side cursor in place as the async cursor. + + ``execute_void()`` runs on a throwaway plain cursor whenever the cached + cursor is a server-side one, but only a transaction-control statement + has that throwaway become the async cursor. Promoting it for every + statement would let something like the ``SELECT pg_cancel_backend(...)`` + that ``cancel_transaction()`` issues detach the cursor a result set is + still being paged or downloaded from, so that the pagination and + download calls that follow read the throwaway and find no rows. + """ + + scenarios = [ + ('a non-transaction statement keeps the cached server-side cursor ' + 'as the async cursor', + dict(sql='SELECT pg_cancel_backend(1234);')), + ] + + def runTest(self): + manager = MagicMock(sid=1) + conn = Connection(manager, 'test-conn-id', 'testdb') + conn.python_encoding = 'utf-8' + + # State from the query whose result set is still being read. + conn.column_info = [{'name': 'x'}] + conn.row_count = 1 + + server_cursor = MagicMock(spec=AsyncDictServerCursor) + server_cursor.closed = False + conn._Connection__async_cursor = server_cursor + + plain_cursor = MagicMock() + plain_cursor.closed = False + + conn.conn = MagicMock() + conn.conn.cursor.return_value = plain_cursor + conn.conn.info.user = 'postgres' + conn.conn.info.host = 'localhost' + conn.conn.info.dbname = 'testdb' + + with self.app.test_request_context(): + with patch( + 'pgadmin.utils.driver.psycopg3.connection.current_user', + MagicMock(email='test@example.com') + ), patch.object(Connection, '_Connection__cursor', + return_value=(True, server_cursor)): + status, result = conn.execute_void(self.sql) + + self.assertTrue(status) + self.assertIsNone(result) + + # It still runs on the throwaway, since a server-side cursor cannot + # execute anything except through DECLARE ... CURSOR FOR. + plain_cursor.execute.assert_called_once() + server_cursor.execute.assert_not_called() + + # ... but the result set being read is left alone. + self.assertIs(conn._Connection__async_cursor, server_cursor) + self.assertEqual(conn.column_info, [{'name': 'x'}]) + self.assertEqual(conn.row_count, 1) + + +class IsTransactionControlTest(BaseTestGenerator): + """Unit tests for the leading-keyword check that decides whether a + statement is transaction control.""" + + scenarios = [ + ('BEGIN', dict(sql='BEGIN;', expected=True)), + ('COMMIT', dict(sql='COMMIT;', expected=True)), + ('ROLLBACK', dict(sql='ROLLBACK;', expected=True)), + ('lower case, no semicolon', dict(sql='commit', expected=True)), + ('leading whitespace', dict(sql=' \n\tROLLBACK;', expected=True)), + ('START TRANSACTION', dict(sql='START TRANSACTION;', expected=True)), + ('ROLLBACK TO SAVEPOINT', + dict(sql='ROLLBACK TO SAVEPOINT sp1;', expected=True)), + ('SAVEPOINT', dict(sql='SAVEPOINT sp1;', expected=True)), + ('RELEASE', dict(sql='RELEASE sp1;', expected=True)), + ('a SELECT', dict(sql='SELECT pg_cancel_backend(1234);', + expected=False)), + ('an INSERT', dict(sql='INSERT INTO t VALUES (1);', expected=False)), + # "beginx" is not "begin". + ('a keyword prefix', dict(sql='BEGINNING;', expected=False)), + ('an empty statement', dict(sql=' ', expected=False)), + ] + + def runTest(self): + self.assertEqual(_is_transaction_control(self.sql), self.expected) From a94372c8c6bbc914b47a96a6dc71c7a2c4f7b970 Mon Sep 17 00:00:00 2001 From: Dave Page Date: Wed, 23 Sep 2026 13:30:03 +0100 Subject: [PATCH 5/6] Skip leading SQL comments when classifying transaction control _is_transaction_control() judged a statement by its first word, so one opening with a -- line comment or a /* */ block comment was classified as something other than transaction control, and execute_void() would then leave the cached server-side cursor as the async cursor after a COMMIT or ROLLBACK, putting the stale result metadata back in front of poll(). Skip leading whitespace and comments first, handling nested block comments as PostgreSQL does, before taking the leading keyword. Raised by CodeRabbit on #10321. --- .../utils/driver/psycopg3/connection.py | 41 ++++++++++++++++++- .../tests/test_execute_void_server_cursor.py | 14 +++++++ 2 files changed, 53 insertions(+), 2 deletions(-) diff --git a/web/pgadmin/utils/driver/psycopg3/connection.py b/web/pgadmin/utils/driver/psycopg3/connection.py index 14319ab150b..5f7f69c8e2e 100644 --- a/web/pgadmin/utils/driver/psycopg3/connection.py +++ b/web/pgadmin/utils/driver/psycopg3/connection.py @@ -69,15 +69,52 @@ }) +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. + judged by its leading keyword once any leading comments are skipped. Args: query: SQL statement, as passed to execute_void() """ - keyword = query.strip().split(None, 1)[0] if query.strip() else '' + words = _skip_leading_comments(query).split(None, 1) + keyword = words[0] if words else '' return keyword.rstrip(';').lower() in TRANSACTION_CONTROL_KEYWORDS diff --git a/web/pgadmin/utils/driver/psycopg3/tests/test_execute_void_server_cursor.py b/web/pgadmin/utils/driver/psycopg3/tests/test_execute_void_server_cursor.py index 8da1e73d3c5..eca66d0625f 100644 --- a/web/pgadmin/utils/driver/psycopg3/tests/test_execute_void_server_cursor.py +++ b/web/pgadmin/utils/driver/psycopg3/tests/test_execute_void_server_cursor.py @@ -228,6 +228,20 @@ class IsTransactionControlTest(BaseTestGenerator): # "beginx" is not "begin". ('a keyword prefix', dict(sql='BEGINNING;', expected=False)), ('an empty statement', dict(sql=' ', expected=False)), + ('a leading line comment', + dict(sql='-- finish up\nCOMMIT;', expected=True)), + ('a leading block comment', + dict(sql='/* finish up */ COMMIT;', expected=True)), + ('a nested block comment', + dict(sql='/* outer /* inner */ still outer */ROLLBACK;', + expected=True)), + ('several leading comments', + dict(sql=' -- one\n/* two */\n\t-- three\nBEGIN;', expected=True)), + ('a comment ahead of a SELECT', + dict(sql='/* COMMIT */ SELECT 1;', expected=False)), + ('only a comment', dict(sql='-- COMMIT', expected=False)), + ('an unterminated block comment', + dict(sql='/* COMMIT;', expected=False)), ] def runTest(self): From 9556df09efd733eefbf126dbf2a0397634e1ae1a Mon Sep 17 00:00:00 2001 From: Dave Page Date: Thu, 24 Sep 2026 12:32:13 +0100 Subject: [PATCH 6/6] Read the transaction-control keyword as a whole word Splitting on whitespace left a comment or semicolon written straight after the keyword attached to it, so COMMIT/* note */; and COMMIT;-- note were not recognised as transaction control, and execute_void() left the previous result's server-side cursor as the async cursor. Match the leading identifier instead, and add scenarios for both forms. --- web/pgadmin/utils/driver/psycopg3/connection.py | 14 +++++++++++--- .../tests/test_execute_void_server_cursor.py | 6 ++++++ 2 files changed, 17 insertions(+), 3 deletions(-) diff --git a/web/pgadmin/utils/driver/psycopg3/connection.py b/web/pgadmin/utils/driver/psycopg3/connection.py index 5f7f69c8e2e..0229b417c75 100644 --- a/web/pgadmin/utils/driver/psycopg3/connection.py +++ b/web/pgadmin/utils/driver/psycopg3/connection.py @@ -14,6 +14,7 @@ """ import os +import re import secrets import datetime import asyncio @@ -69,6 +70,10 @@ }) +# 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 @@ -113,10 +118,13 @@ def _is_transaction_control(query): Args: query: SQL statement, as passed to execute_void() """ - words = _skip_leading_comments(query).split(None, 1) - keyword = words[0] if words else '' + # 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 keyword.rstrip(';').lower() in TRANSACTION_CONTROL_KEYWORDS + return bool(match) and \ + match.group().lower() in TRANSACTION_CONTROL_KEYWORDS class Connection(BaseConnection): diff --git a/web/pgadmin/utils/driver/psycopg3/tests/test_execute_void_server_cursor.py b/web/pgadmin/utils/driver/psycopg3/tests/test_execute_void_server_cursor.py index eca66d0625f..4475c3c4e01 100644 --- a/web/pgadmin/utils/driver/psycopg3/tests/test_execute_void_server_cursor.py +++ b/web/pgadmin/utils/driver/psycopg3/tests/test_execute_void_server_cursor.py @@ -242,6 +242,12 @@ class IsTransactionControlTest(BaseTestGenerator): ('only a comment', dict(sql='-- COMMIT', expected=False)), ('an unterminated block comment', dict(sql='/* COMMIT;', expected=False)), + ('a block comment straight after the keyword', + dict(sql='COMMIT/* note */;', expected=True)), + ('a line comment straight after the semicolon', + dict(sql='COMMIT;-- note', expected=True)), + ('a keyword followed by digits', + dict(sql='BEGIN1;', expected=False)), ] def runTest(self):