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 <[email protected]> wrote: > > 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, <[email protected]> wrote: > >> 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 > >
From 11fa95492f79c3104534cba5b616b2aeafbda6be Mon Sep 17 00:00:00 2001 From: BharatDBPG <[email protected]> Date: Thu, 27 Nov 2025 14:38:48 +0530 Subject: [PATCH] psql: Add tab-completion support for ALTER ROLE ... IN DATABASE ... SET/RESET forms MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This patch extends psql's tab-completion logic to recognize the "ALTER ROLE <role> IN DATABASE <dbname>" command structure, and provides appropriate completions for the SET and RESET subcommands. Specifically: • After "ALTER ROLE <role> IN DATABASE <dbname> SET", psql now completes with the list of configuration variables that may be set (Query_for_list_of_set_vars), matching the behavior of the plain ALTER ROLE ... SET form. • After "ALTER ROLE <role> IN DATABASE <dbname> RESET", psql now suggests configuration variables that are *actually set* for that specific (role,database) pair, as recorded in pg_db_role_setting, plus the keyword ALL. This mirrors the behavior of ALTER DATABASE ... RESET, where we complete only the variables currently set for the object being modified. The role name and database name are extracted from the already-parsed input tokens, and SQL literal quoting is performed via PQescapeLiteral() using the implicit PGconn (pset.db) available to the tab-completion code. This avoids any need to alter tab-completion APIs and keeps the patch self-contained. Due to the structure of tab-completion, this patch intentionally does not attempt to complete arbitrary GUC names for RESET, but rather only those that exist in pg_db_role_setting for the given role and database. When none are present, psql falls back to suggesting ALL, matching existing RESET behavior elsewhere. Vasuki M --- src/bin/psql/tab-complete.in.c | 66 ++++++++++++++++++++++++++++++++-- 1 file changed, 64 insertions(+), 2 deletions(-) diff --git a/src/bin/psql/tab-complete.in.c b/src/bin/psql/tab-complete.in.c index 75a101c6ab5..c4cb79c33f2 100644 --- a/src/bin/psql/tab-complete.in.c +++ b/src/bin/psql/tab-complete.in.c @@ -2536,12 +2536,74 @@ match_previous_words(int pattern_id, else if (Matches("ALTER", "USER|ROLE", MatchAny) && !TailMatches("USER", "MAPPING")) COMPLETE_WITH("BYPASSRLS", "CONNECTION LIMIT", "CREATEDB", "CREATEROLE", - "ENCRYPTED PASSWORD", "INHERIT", "LOGIN", "NOBYPASSRLS", - "NOCREATEDB", "NOCREATEROLE", "NOINHERIT", + "ENCRYPTED PASSWORD", "IN DATABASE", "INHERIT", "LOGIN", + "NOBYPASSRLS", "NOCREATEDB", "NOCREATEROLE", "NOINHERIT", "NOLOGIN", "NOREPLICATION", "NOSUPERUSER", "PASSWORD", "RENAME TO", "REPLICATION", "RESET", "SET", "SUPERUSER", "VALID UNTIL", "WITH"); + /* ALTER USER,ROLE <name> IN */ + else if (Matches("ALTER", "USER|ROLE", MatchAny, "IN")) + COMPLETE_WITH("DATABASE"); + /* ALTER USER/ROLE <name> IN DATABASE */ + else if (Matches("ALTER", "USER|ROLE", MatchAny, "IN", "DATABASE")) + { + /* ALTER ROLE bob IN DATABASE <TAB> → list databases */ + COMPLETE_WITH_QUERY(Query_for_list_of_databases); + } + /* ALTER USER/ROLE <name> IN DATABASE <dbname> */ + else if (Matches("ALTER", "USER|ROLE", MatchAny, "IN", "DATABASE", MatchAny)) + { + /* ALTER ROLE bob IN DATABASE mydb <TAB> → SET, RESET */ + COMPLETE_WITH("SET", "RESET"); + } + /* ALTER USER/ROLE <name> IN DATABASE <dbname> SET */ + else if (Matches("ALTER", "USER|ROLE", MatchAny, "IN", "DATABASE", MatchAny, "SET")) + { + /* ALTER ROLE bob IN DATABASE mydb SET <TAB> */ + COMPLETE_WITH_QUERY(Query_for_list_of_set_vars); + } + /* ALTER USER/ROLE <name> IN DATABASE <dbname> RESET */ + else if (Matches("ALTER", "USER|ROLE", MatchAny, "IN", "DATABASE", MatchAny, "RESET")) + { + /* + * Extract tokens: + * prev5 = role name + * prev2 = database name + */ + char *role = prev5_wd; + char *dbname = prev2_wd; + char *q_role; + char *q_dbname; + char *query; + /* Safe SQL literal quoting using libpq */ + 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"); + } + else + { + query = psprintf( + "SELECT unnest(setconfig) " + " FROM pg_db_role_setting " + " WHERE setdatabase = " + " (SELECT oid FROM pg_database WHERE datname = %s) " + " AND setrole = %s::regrole", + q_dbname, q_role); + COMPLETE_WITH_QUERY_PLUS(query, "ALL"); + PQfreemem(q_role); + PQfreemem(q_dbname); + pfree(query); + } + } + /* ALTER USER,ROLE <name> RESET */ else if (Matches("ALTER", "USER|ROLE", MatchAny, "RESET")) { -- 2.43.0
