Re: [PATCH] psql: tab completion for ALTER ROLE ... IN DATABASE ... - Mailing list pgsql-hackers
| From | Dharin Shah |
|---|---|
| Subject | Re: [PATCH] psql: tab completion for ALTER ROLE ... IN DATABASE ... |
| Date | |
| Msg-id | CAOj6k6fdP=1XwK_6=SeoHPBEO-5cmGznqSaiwzwuwUMjOS-W0g@mail.gmail.com Whole thread Raw |
| In response to | Re: [PATCH] psql: tab completion for ALTER ROLE ... IN DATABASE ... (VASUKI M <vasukianand0119@gmail.com>) |
| Responses |
Re: [PATCH] psql: tab completion for ALTER ROLE ... IN DATABASE ...
Re: [PATCH] psql: tab completion for ALTER ROLE ... IN DATABASE ... |
| List | pgsql-hackers |
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.
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.
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
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 <vasukianand0119@gmail.com> 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
pgsql-hackers by date: