psql: Add tab completion for ALTER USER RESET

Hi,

Please find attached a patch to help tab completion show only currently set
vars
during ALTER USER RESET.

Currently tab completion provides a list of all vars which is only
partially helpful.
This patch allows tab completion to see which vars are currently set for
the role
and only show those options during tab completion.

For example:
postgres=# \drds
List of settings
Role | Database | Settings
----------+----------+-------------------------------------
postgres | | max_parallel_workers_per_gather=100+
| | max_parallel_workers=100
(1 row)

postgres=# alter user postgres reset
ALL max_parallel_workers
max_parallel_workers_per_gather

postgres=# alter user postgres reset max_parallel_workers;
ALTER ROLE

postgres=# \drds
List of settings
Role | Database | Settings
----------+----------+-------------------------------------
postgres | | max_parallel_workers_per_gather=100
(1 row)

postgres=# alter user postgres reset
ALL max_parallel_workers_per_gather

postgres=# alter user postgres reset all;
ALTER ROLE

postgres=# \drds
Did not find any settings.

Applies on master (b6612aedc53a6) / make check successful.
-
Robins Tharakan
Amazon Web Services

Hi,

On 11/29/24 09:42, Robins Tharakan wrote:
> Hi,
>
> Please find attached a patch to help tab completion show only currently
> set vars
> during ALTER USER RESET.
>
> Currently tab completion provides a list of all vars which is only
> partially helpful.
> This patch allows tab completion to see which vars are currently set for
> the role
> and only show those options during tab completion.
>

Yeah, this looks convenient. Without the patch we'd get

...
Display all 200 possibilities? (y or n)

and then a wall of text with all the options. With the patch we get only
options that are actually set for the user, which is much better.

Two comments:

1) Does it make sense to still show "ALL" when the query returns
nothing? Not sure if we already have a way to handle this.

2) Should we do the same thing for ALTER DATABASE? That also allows
setting options.

thanks

--
Tomas Vondra

Hi Tomas,

Thanks for taking a look - apologies for the delay here.

On Tue, 10 Dec 2024 at 09:09, Tomas Vondra <tomas(at)vondra(dot)me> wrote:
>
> 1) Does it make sense to still show "ALL" when the query returns
> nothing? Not sure if we already have a way to handle this.
>

+1 - "ALL" is pointless when there is nothing to RESET, but I didn't
find an easy way to make it conditional (which was partly the
cause of delay here). For this, I tried (and not yet successful) to
create a new set of macros - something along the lines of
COMPLETE_WITH_QUERY_IFNOTEMPTY_PLUS() - where
the second part is conditional on whether the query list returns
a non-empty set.

If this is a blocker, I'll continue working on a macro that allows that
more easily (although it'd be great if you could point me to a better
way to implement that). For now, reattached v1 to this email for
convenience.

> 2) Should we do the same thing for ALTER DATABASE? That also allows
> setting options.
>
+1. Attached a separate patch for ALTER DATABASE RESET in case
you think it is good to go alongside (albeit again, this still does show
ALL even if there's no vars to RESET).

-
robins

On 2/15/25 12:14, Robins Tharakan wrote:
> Hi Tomas,
>
> Thanks for taking a look - apologies for the delay here.
>
> On Tue, 10 Dec 2024 at 09:09, Tomas Vondra <tomas(at)vondra(dot)me
> <mailto:tomas(at)vondra(dot)me>> wrote:
>>
>> 1) Does it make sense to still show "ALL" when the query returns
>> nothing? Not sure if we already have a way to handle this.
>>
>
> +1 - "ALL" is pointless when there is nothing to RESET, but I didn't
> find an easy way to make it conditional (which was partly the
> cause of delay here). For this, I tried (and not yet successful) to
> create a new set of macros - something along the lines of
> COMPLETE_WITH_QUERY_IFNOTEMPTY_PLUS() - where
> the second part is conditional on whether the query list returns
> a non-empty set.
>
> If this is a blocker, I'll continue working on a macro that allows that
> more easily (although it'd be great if you could point me to a better
> way to implement that). For now, reattached v1 to this email for
> convenience.
>

Thanks. I don't think it's a blocker - in fact, after thinking about it
a bit more, I believe showing the "ALL" is consistent with what we do in
other places when the query returns nothing. Consider for example CLOSE:

/* CLOSE */
else if (Matches("CLOSE"))
COMPLETE_WITH_QUERY_PLUS(Query_for_list_of_cursors,
"ALL");

Other places do similar stuff. It's just a noop. So I don't think you
need to waste time inventing something for this one place.

>
>> 2) Should we do the same thing for ALTER DATABASE? That also allows
>> setting options.
>>
> +1. Attached a separate patch for ALTER DATABASE RESET in case
> you think it is good to go alongside (albeit again, this still does show
> ALL even if there's no vars to RESET).
>

Thanks. These patches look fine to me. I'll get them committed.

regards

--
Tomas Vondra

On 2/16/25 17:56, Tomas Vondra wrote:
>...
>
> Thanks. These patches look fine to me. I'll get them committed.
>

I've pushed both patches. Thanks!

--
Tomas Vondra