From ab0cc6250e5f7b22cc8f1dc65b62acefa95f6923 Mon Sep 17 00:00:00 2001 From: Dave Page Date: Wed, 19 Aug 2026 12:39:43 +0100 Subject: [PATCH 1/3] 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 e70c6982076e19d8ae0f6b25d3cc1dcf3dd45b56 Mon Sep 17 00:00:00 2001 From: Dave Page Date: Tue, 25 Aug 2026 10:07:48 +0100 Subject: [PATCH 2/3] 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 251eb4363d77cfd362c882b8d0c766bcde107bcc Mon Sep 17 00:00:00 2001 From: Dave Page Date: Thu, 3 Sep 2026 11:17:23 +0100 Subject: [PATCH 3/3] 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(';'))