From ab0cc6250e5f7b22cc8f1dc65b62acefa95f6923 Mon Sep 17 00:00:00 2001 From: Dave Page Date: Wed, 19 Aug 2026 12:39:43 +0100 Subject: [PATCH 1/2] 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/2] 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)