Fix server-cursor queries silently failing with 'keyword not supported: prepare' - #10343
Open
dpage wants to merge 1 commit into
Open
Fix server-cursor queries silently failing with 'keyword not supported: prepare'#10343dpage wants to merge 1 commit into
dpage wants to merge 1 commit into
Conversation
psycopg's AsyncServerCursor.execute() has never accepted a prepare kwarg (a server-side DECLARE CURSOR can't be a prepared statement) and raises TypeError on any unexpected keyword, even one whose value is None. PR pgadmin-org#10030 widened AsyncDictCursor.execute()/_execute() to forward prepare/binary to whatever cursor it holds, fixing a real psycopg_pool.ConnectionPool.check_connection breakage. AsyncDictServerCursor inherits that same execute()/_execute() without overriding it, so every server-cursor query now unconditionally forwards prepare=None straight into AsyncServerCursor.execute() and fails with "TypeError: keyword not supported: prepare". That TypeError isn't a psycopg.Error, so execute_async()'s "except psycopg.Error" doesn't catch it. It propagates into the background QueryThread's generic exception handler, which logs it and builds an internal_server_error response that is discarded (the thread's return value goes nowhere) - so the query silently never runs, invisible to the user, and the async cursor is left with no query having actually executed on it. Give AsyncDictServerCursor its own _execute() that drops prepare before delegating, leaving DictCursor/AsyncDictCursor's forwarding for the pool-checkout case it was written for untouched. Strengthen test_server_cursor.py's existing scenario to assert the poll response's actual status/result instead of only the HTTP status code and the echoed server_cursor flag, and add a fast, DB-less regression test asserting AsyncDictServerCursor._execute never forwards prepare.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
Included review availability: Your plan provides up to 8 included reviews per hour; 5 remain after this review. WalkthroughThe change updates ChangesServer cursor execution
Estimated code review effort: 2 (Simple) | ~10 minutes Merge Risk: ⚪ Minimal · up to The change narrowly prevents server-cursor queries from failing when an unsupported prepare argument is forwarded, with targeted tests covering the regression; no actionable merge-blocking risk remains. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
Every query run under "Use server cursor?" currently fails silently.
AsyncServerCursor.execute()(psycopg) has never accepted apreparekeyword — a server-sideDECLARE CURSORcan't be a prepared statement — and raisesTypeErroron any unexpected keyword, even one whose value isNone.#10030 widened
AsyncDictCursor.execute()/_execute()to forwardprepare/binaryto whatever cursor it holds, correctly fixing apsycopg_pool.ConnectionPool.check_connectionbreakage.AsyncDictServerCursorinherits that sameexecute()/_execute()without overriding it, so every server-cursor query now unconditionally forwardsprepare=NoneintoAsyncServerCursor.execute()and fails with:That
TypeErrorisn't apsycopg.Error, soexecute_async()'sexcept psycopg.Errordoesn't catch it. It propagates into the backgroundQueryThread's generic exception handler, which logs it and builds aninternal_server_errorresponse that's discarded (the thread's return value goes nowhere) — so the query silently never runs. The user sees no error; the async cursor is left with no query ever actually executed on it, which is also the precondition behind the crash in #10321 (get_explain_query_lengthbeing handed a cursor whose_queryisNone).Fix
Give
AsyncDictServerCursorits own_execute()that dropspreparebefore delegating, leavingDictCursor/AsyncDictCursor's forwarding (the actual #10030 fix) untouched for the pool-checkout case it was written for.Tests
test_server_cursor.py's existing scenario to assert the poll response's actualstatus/result, rather than only the HTTP status code and the echoedserver_cursorflag (which is just config, not proof the query ran) — this is what let the regression through un-caught.AsyncDictServerCursor._executenever forwardsprepareto the underlying cursor.Both fail without the fix and pass with it; ran locally against PostgreSQL 18.
Summary by CodeRabbit
Bug Fixes
Tests