[PATCH] psql: tab completion for ALTER ROLE ... IN DATABASE ...

On Fri, Nov 21, 2025 at 1:25 PM Ian Lawrence Barwick <barwick@gmail.com>
wrote:

Hi

I found myself needing to work with ALTER ROLE ... IN DATABASE ... recently
and was annoyed by the lack of tab completion for this, so patch attached.

Regards

Ian Barwick

HI, I've reviewed the patch and did simple tests — it works correctly.

Ian Lawrence Barwick <barwick@gmail.com> writes:

Hi

I found myself needing to work with ALTER ROLE ... IN DATABASE ... recently
and was annoyed by the lack of tab completion for this, so patch attached.

A noble goal, but unfortunately th RESET form can't work properly due to
limitations of the tab completion system.

+	/* ALTER USER,ROLE <name> IN DATABASE */
+	else if (HeadMatches("ALTER", "USER|ROLE", MatchAny, "IN"))
+	{

[...]

+		else if (TailMatches("DATABASE", MatchAny, "RESET"))
+		{
+			set_completion_reference(prev5_wd);
+			COMPLETE_WITH_QUERY_PLUS(Query_for_list_of_user_vars, "ALL");

This queries pg_roles.rolconfig, which only contains variables set for
the user in all databases, not in the specified database. Instead,
you'd need to query pg_db_role_setting WHERE setdatabase = (SELECT oid
FROM pg_database WHERE datname = '%s') AND setrole = '%s'::regrole, but
unfortunately the tab completion system doesn't let you more than one
previous word in the query. I guess you could query WHERE setdatabase
<> 0, to get variables set for the user across all databases, not just
the specified one.

Also, alter ALTER ROLE ALL RESET needs separate handling, filtering
where setrole = 0, which is actually possible in the current system.

- ilmari

Hi Ian,

+1 for the patch,LGTM

But after applying the patch i can be able to apply all the results
except the reset options user variables

postgres=#
postgres=# alter role bob
BYPASSRLS CREATEROLE INHERIT NOCREATEDB
NOLOGIN PASSWORD RESET
VALID UNTIL
CONNECTION LIMIT ENCRYPTED PASSWORD LOGIN
NOCREATEROLE NOREPLICATION RENAME TO SET
WITH
CREATEDB IN DATABASE NOBYPASSRLS NOINHERIT
NOSUPERUSER REPLICATION SUPERUSER
postgres=# alter role bob in DATABASE
postgres template0 template1
postgres=# alter role bob in DATABASE postgres
RESET SET
postgres=# alter role bob in DATABASE postgres reset ALL

postgres=# alter role bob in DATABASE postgres reset ALL

also i cross verified that my system doesn't have variables so it
returns 0 rows?but:

postgres=# SELECT name FROM pg_settings LIMIT 5;
name
----------------------------
allow_alter_system
allow_in_place_tablespaces
allow_system_table_mods
application_name
archive_cleanup_command
(5 rows)

Can you check this ?

-regards

Vasuki M

Hi BharatDB,

On Tue, Nov 25, 2025 at 5:15 PM BharatDB <bharatdbpg@gmail.com> wrote:

Hi Ian,

+1 for the patch,LGTM

But after applying the patch i can be able to apply all the results
except the reset options user variables

postgres=# alter role bob in DATABASE postgres reset ALL

also i cross verified that my system doesn't have variables so it
returns 0 rows?but:

postgres=# SELECT name FROM pg_settings LIMIT 5;
name
----------------------------
allow_alter_system
allow_in_place_tablespaces
allow_system_table_mods
application_name
archive_cleanup_command
(5 rows)

The tab-completion here queries the user-specific config (not the global
pg_settings). I believe the underlying code logic will help explain this
behavior:

+ else if (TailMatches("DATABASE", MatchAny, "RESET"))
+ {
+ set_completion_reference(prev5_wd);
+ COMPLETE_WITH_QUERY_PLUS(Query_for_list_of_user_vars, "ALL");
+ }

#define Query_for_list_of_user_vars \

Hello all,

Based on the discussion, I have updated the patch to handle the RESET form
correctly without modifying psql’s tab-completion APIs.

Both the current database connection and the parsed input tokens are
already available via pset.db and the word tokens.The new patch extracts:

-

the role name from the parsed tokens (prev5_wd), and
-

the database name from the parsed tokens (prev2_wd),

and uses these to query pg_db_role_setting for variables that are actually
set for the specific (role, database) pair.

Literal quoting is now done with PQescapeLiteral(pset.db, …), per libpq
conventions, so no new helper functions were needed.

SET
After

ALTER ROLE <role> IN DATABASE <dbname> SET <TAB>

psql completes from Query_for_list_of_set_vars (same behavior as plain
ALTER ROLE … SET).

RESET
After

ALTER ROLE <role> IN DATABASE <dbname> RESET <TAB>

psql now completes with the GUC names recorded in pg_db_role_setting for
that specific (role,database), plus ALL.
When no settings exist, only ALL is suggested.
This mirrors the existing behavior of ALTER DATABASE … RESET.

I have attached the patch.

Regards,

Vasuki

On Thu, Nov 27, 2025 at 2:27 PM Ian Lawrence Barwick <barwick@gmail.com>
wrote:

Kindly review the attached patch ,
As i saw the proposed patch by Ian failed with CI
https://commitfest.postgresql.org/patch/6244/
Guide me with the patch

Regards,
Vasuki M

On Thu, Nov 27, 2025 at 2:50 PM VASUKI M <vasukianand0119@gmail.com> wrote:

Hi All,

Thanks to Ian and Vasuki for working on tab-completion support for ALTER
ROLE ... IN DATABASE.
This is a really good improvement.

Regarding Ian’s patch:
1. The patch applies cleanly and works as expected.
2. Tab-completion for IN DATABASE behaves consistently with existing ALTER
ROLE forms.

Suggestion: consider adding regression tests in "src/bin/psql/t/" to cover
these new cases.

Regarding Vasuki’s patch:
1. Really nice idea to extend completion for RESET by querying
pg_db_role_setting.
2. Use of PQescapeLiteral() (safe quoting) and consistent fallback to ALL
is really nice, and if the user falls back to 'ALL' maybe it is a good idea
to log it or let the user know of it.

Suggestion: consider adding regression tests in "src/bin/psql/t/" to cover
these new cases for SET/RESET.

Overall both patches look great.

Regards,
Surya

Hi surya,

Thanks a lot for reviewing the patches and for the encouraging feedback
Sorry for the late reply — I missed the mail since I wasn’t CC’d and
noticed it only now.

Good point about the TAP tests. I'll be adding tab-completion tests in
src/bin/psql/t/010_tab_completion.pl for both Ian’s patch and the SET/RESET
cases from my patch shortly.

Thanks again!

Regards,

Vasuki M

C-DAC,Chennai

Hi

I noticed that in the code, the variables `q_role` and `q_dbname` are processed with the `PQescapeLiteral` function,
so `PQfreemem` – instead of `pfree` – should be used here to free the memory.

--
Regards,
Man Zeng
www.openhalo.org

Hi zeng,

Thanks for pointing this out. You’re absolutely right — PQescapeLiteral()
allocates memory using libpq’s allocator, so the returned buffers must be
released with PQfreemem() rather than pfree(). Using pfree() here would be
incorrect, since it expects memory allocated via PostgreSQL’s memory
context APIs (palloc/psprintf).

I’ll update the patch to replace pfree() with PQfreemem() for the buffers
returned by PQescapeLiteral(),while continuing to use pfree() for memory
allocated via psprintf().

Thanks again for catching this.

Best regards,
Vasuki M
C-DAC,Chennai

On Mon, 22 Dec 2025, 8:18 pm zengman, <zengman@halodbtech.com> wrote:

Hi all,

I tried adding TAP coverage for the new ALTER ROLE … IN
DATABASE tab-completion paths in src/bin/psql/t/010_tab_completion.pl.

The tests themselves work as expected, but I ran into a limitation of the
existing interactive completion harness. The new RESET completion
intentionally ends on incomplete SQL and leaves psql in
continuation/readline mode (postgres-#). As a result, the interactive psql
process does not terminate cleanly at the end of the test, causing
IPC::Run to time out and the test to abort, even though all completion
checks pass.

Earlier completion tests never exercised this state, so the harness
implicitly assumes that psql can always be exited cleanly after <TAB> using
\q / clear query(); / clear line(); . This change exposes that assumption
rather than introducing a regression.

Given this limitation, and to avoid relying on timeouts or fragile cleanup
logic, I’m omitting TAP tests for this change for now. If there’s interest
in extending or refactoring the completion test harness to better handle
continuation-mode cases, I’d be happy to look into that separately.

Attaching some lines from the logfile for the reference,

[11:19:45.497](0.000s) ok 79 - complete a psql variable name
[11:19:45.497](0.000s) ok 80 - complete a psql variable value
[11:19:45.498](0.000s) ok 81 - \r works
[11:19:45.498](0.000s) ok 82 - complete an interpolated psql variable name
[11:19:45.498](0.000s) ok 83 - \r works
[11:19:45.498](0.000s) ok 84 - complete a psql variable test
[11:19:45.498](0.000s) ok 85 - \r works
[11:19:45.498](0.000s) ok 86 - check completion failure path
[11:19:45.499](0.000s) ok 87 - \r works
[11:19:45.499](0.000s) ok 88 - COPY FROM with DEFAULT completion
[11:19:45.499](0.000s) ok 89 - control-U works
IPC::Run: timeout on timer #1 at /usr/share/perl5/IPC/Run.pm line 3007.
# Postmaster PID for node "main" is 16601
### Stopping node "main" using mode immediate
# Running: pg_ctl --pgdata
/home/cdac/postgres/src/bin/psql/tmp_check/t_010_tab_completion_main_data/pgdata
--mode immediate stop
waiting for server to shut down.... done
server stopped
# No postmaster PID for node "main"
[11:22:46.573](181.074s) # Tests were run but no plan was declared and
done_testing() was not seen.
[11:22:46.574](0.000s) # Looks like your test exited with 29 just after 89.

Regards,

Vasuki M
C-DAC,Chennai.

On Mon, Dec 22, 2025 at 9:10 PM VASUKI M <vasukianand0119@gmail.com> wrote:

Hi,

I got lots of indentation-related warnings when running git apply (see output below).
Also, I found an issue: the RESET command unexpectedly displays "work_mem=16MB",
which is not correct. I've made a minor fix by adding split_part and attached the v3 patch.

```
postgres@zxm-VMware-Virtual-Platform:~/code/postgres$ git apply v2-0001-psql-alter-role-in-database-tab-completion.patch
v2-0001-psql-alter-role-in-database-tab-completion.patch:67: indent with spaces.
/* ALTER ROLE bob IN DATABASE <TAB> → list databases */
v2-0001-psql-alter-role-in-database-tab-completion.patch:68: indent with spaces.
COMPLETE_WITH_QUERY(Query_for_list_of_databases);
v2-0001-psql-alter-role-in-database-tab-completion.patch:73: indent with spaces.
/* ALTER ROLE bob IN DATABASE mydb <TAB> → SET, RESET */
v2-0001-psql-alter-role-in-database-tab-completion.patch:74: indent with spaces.
COMPLETE_WITH("SET", "RESET");
v2-0001-psql-alter-role-in-database-tab-completion.patch:79: indent with spaces.
/* ALTER ROLE bob IN DATABASE mydb SET <TAB> */
warning: squelched 37 whitespace errors
warning: 42 lines add whitespace errors.
```

```
postgres=# ALTER ROLE postgres IN DATABASE postgres SET work_mem = '16MB';
ALTER ROLE
postgres=# ALTER ROLE postgres IN DATABASE postgres RESET
ALL "work_mem=16MB"
```

Could you please take a look and see if this modification is correct?

--
Regards,
Man Zeng
www.openhalo.org

Hi all,

Thanks everyone for the reviews and discussion — I appreciate the time
spent looking at this.

Based on the feedback, I’ve prepared v4 of the patch with the following
updates:

Added a safety guard before calling PQescapeLiteral() to ensure pset.db is
non-NULL and in CONNECTION_OK state, falling back to ALL otherwise.

Kept the RESET completion limited to parameters actually present in
pg_db_role_setting, matching the behavior of other object-specific RESET
forms.

No changes to tab-completion APIs or libreadline interactions.

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.

I’m happy to adjust if there is a preferred place or pattern for testing
this kind of completion in the future.

Thanks again for the guidance.

On Mon, Dec 29, 2025 at 1:18 PM zengman <zengman@halodbtech.com> wrote:

Hi,

I got lots of indentation-related warnings when running git apply (see
output below).
Also, I found an issue: the RESET command unexpectedly displays
"work_mem=16MB",
which is not correct. I've made a minor fix by adding split_part and
attached the v3 patch.

Sorry my system is unable to indent with the 2.1.2 version as it is not

yet released,I’ve aligned the indentation with the surrounding code, and CI
/ buildfarm should catch any remaining formatting issues.

Could you please take a look and see if this modification is correct?

Yeah,thanks.

Regards,
Vasuki M
C-DAC,Chennai

Hi,

This v4 patch appears incomplete; my `git apply` command failed.
CFbot: Patch needs rebase

--
Regards,
Man Zeng
www.openhalo.org

Sorry,it was an incremental patch.
The attached v5 is the fresh patch against Master.

Regards,
Vasuki M
C-DAC,Chennai

Added a safety guard before calling PQescapeLiteral() to ensure pset.db is non-NULL and in CONNECTION_OK state, falling back to ALL otherwise.

Hi, VASUKI M

I just looked at this patch and thought about it briefly, but I'm a little confused about the purpose of this conditional check.
If the condition is met, then returning anything at this point makes no sense, right?

```
if (!pset.db || PQstatus(pset.db) != CONNECTION_OK)
{
COMPLETE_WITH("ALL");
}
```

Furthermore, I believe this should be guaranteed by the function `exec_query`.

```
COMPLETE_WITH_QUERY_PLUS -> COMPLETE_WITH_QUERY_LIST -> complete_from_query -> exec_query
```

However, I haven't had time to debug it yet, so I might need your help to double-check it.

--
Regards,
Man Zeng
www.openhalo.org

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

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 <vasukianand0119@gmail.com> wrote:

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 p> revent 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.

Hi,

Thank you both for clarifying my confusion. I hadn't paid much attention to `PQescapeLiteral` earlier.
I checked the function, and it should return NULL on failure.
```
q_role = PQescapeLiteral(pset.db, role, strlen(role));
q_dbname = PQescapeLiteral(pset.db, dbname, strlen(dbname));
if (!q_role || !q_dbname)
{
/* If quoting fails, just fall back to ALL */
if (q_role)
PQfreemem(q_role);
if (q_dbname)
PQfreemem(q_dbname);
COMPLETE_WITH("ALL");
}
```
When `pset.db` is NULL or for other reasons (resulting in q_role/q_dbname being NULL), `!q_role || !q_dbname` will be hit — this already meets our needs.
I wonder if this understanding is correct — what do you think?

--
Regards,
Man Zeng
www.openhalo.org