Обсуждение: [PATCH] psql: tab completion for ALTER ROLE ... IN DATABASE ...

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


On Fri, Nov 21, 2025 at 8:44 AM 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 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,

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,

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

--
Regards,
Man Zeng
www.openhalo.org

> Added a safety guard before calling PQescapeLiteral() to ensure pset.db is non-NULL and in CONNECTION_OK state,
fallingback 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
conditionalcheck. 
 
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

> That guard was actually my suggestion in review, and the reason is that it’s protecting the PQescapeLiteral() calls,
notexec_query().
 
> In the RESET completion branch we call PQescapeLiteral(pset.db, ...) to safely quote the role/dbname before we even
buildthe SQL string. That happens before COMPLETE_WITH_QUERY_PLUS ever reaches complete_from_query() / exec_query(). So
whileexec_query() does guard query execution when pset.db is null or not CONNECTION_OK, it doesn’t p> revent us from
passinga 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
staticcompletion 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

> The only thing I’m cautious about is treating “pset.db is NULL/invalid” as just another “quoting failure” case. In
thiscompletion branch we call PQescapeLiteral(pset.db, ...) before we ever reach exec_query(), so an explicit guard is
aboutavoiding passing an unusable handle into libpq in the first place. Even if libpq were to return NULL in that
situation,it’s > not something I’d want to rely on implicitly.
 
> That’s why I suggested the explicit guard: it matches the general psql style of checking !pset.db before calling
libpqAPIs (e.g. psql_get_variable() in src/bin/psql/common.c checks !pset.db before calling PQescapeLiteral()), and it
makesthe intent obviously safe. Behavior-wise it’s the same (fall back to ALL), just more defensive/clear & explicit. 
 
Hi,

Okay, I understand what you mean, thank you.

--
Regards,
Man Zeng
www.openhalo.org