Hello, That guard was actually my suggestion in review, and the reason is that it’s protecting the PQescapeLiteral() calls, not exec_query().
In the RESET completion branch we call PQescapeLiteral(pset.db, ...) to safely quote the role/dbname before we even build the SQL string. That happens before COMPLETE_WITH_QUERY_PLUS ever reaches complete_from_query() / exec_query(). So while exec_query() does guard query execution when pset.db is null or not CONNECTION_OK, it doesn’t prevent us from passing a null conn into PQescapeLiteral(), which could crash or otherwise misbehave. If we don’t have a usable connection, we can’t reliably quote and run the catalog query anyway, so falling back to a static completion like ALL seems like the safest behavior. Hence I think it's valid. Regarding tests: I initially attempted to add TAP coverage via > src/bin/psql/t/010_tab_completion.pl, but based on Tom and Robert’s > comments[on discord], that file is intended to validate libreadline > mechanics rather than individual SQL completion cases. Since this change > does not introduce new readline behavior and SQL-based completion paths > during continuation prompts are not reliably exercised by the current test > harness, I’ve left the patch without TAP coverage. On testing: I did try adding TAP coverage in src/bin/psql/t/ 010_tab_completion.pl, and we can in fact cover most of this feature there today. In particular, 010_tab_completion.pl already contains a number of “individual completion case” checks beyond pure readline plumbing (e.g., query-driven completions like schema-qualified object references, timezone completion, COPY DEFAULT, etc.), so adding targeted cases for ALTER ROLE ... IN DATABASE seems consistent with existing practice. What we were able to cover reliably: - keyword progression (IN DATABASE) - database name completion after IN DATABASE postgres - offering SET/RESET after the database name (<TAB><TAB>) - SET keyword completion and GUC name completion (SET work_<TAB> -> work_mem) What I could not make reliable in TAP is the query-driven RESET variable listing itself. The new completion rule only triggers at “… RESET <TAB>” (cursor immediately after RESET), so prefix insertion tests like “RESET wo<TAB>” won’t exercise it. That leaves list-style output (“RESET <TAB><TAB>”), which is both highly variant across readline/libedit implementations and not reliably matchable with the current query_until()-based harness, leading to timeouts/flakiness even though manual interactive testing confirms it works. Given that, I think keeping the existing TAP coverage for the deterministic parts (as above) plus a short comment in 010 explaining why the RESET variable-listing output isn’t asserted is a reasonable compromise. If there’s a preferred pattern/harness improvement to make query-driven list output stable, I’m happy to follow that direction. Thanks, Dharin On Tue, Jan 6, 2026 at 11:54 AM VASUKI M <[email protected]> wrote: > Hi, > > Thanks for taking a closer look — that’s a fair question. > > The guard is not meant to protect exec_query(), but rather the calls to > PQescapeLiteral(), which are executed before COMPLETE_WITH_QUERY_PLUS > and therefore before exec_query() is reached. > > In this code path we construct the query string by calling > PQescapeLiteral(pset.db, ...) directly, and that function assumes a > non-NULL, valid PGconn. If pset.db is NULL or the connection is not in > CONNECTION_OK state, calling PQescapeLiteral() would be unsafe and could > lead to a crash before exec_query() has a chance to handle anything. > > The fallback to COMPLETE_WITH("ALL") is intended as a safe default, > matching > existing RESET completion behavior when no catalog information can be > retrieved. > > That said, I agree it’s worth double-checking whether pset.db is always > guaranteed to be valid in this context, and I’m happy to refine or document > this further if you think a different approach would be better. > > Thanks for flagging this . > > Regards, > Vasuki M > C-DAC,Chennai >
