Horton

Thread

Commits

  1. Add a few new patterns to the tab completion of psql

  1. Miscellaneous tab completion issue fixes

    vignesh C <vignesh21@gmail.com> — 2022-10-03T15:11:07Z

    Hi,

There were a couple of tab completion issues present:
a) \dRp and \dRs tab completion displays tables instead of displaying
publications and subscriptions.
b) "ALTER ... OWNER TO" does not include displaying of CURRENT_ROLE,
CURRENT_USER and SESSION_USER.

The attached patch has the changes to handle the same.

Regards,
Vignesh

  2. Re: Miscellaneous tab completion issue fixes

    Dagfinn Ilmari Mannsåker <ilmari@ilmari.org> — 2022-10-03T17:29:32Z

    vignesh C <vignesh21@gmail.com> writes:

> Hi,
>
> There were a couple of tab completion issues present:
> a) \dRp and \dRs tab completion displays tables instead of displaying
> publications and subscriptions.
> b) "ALTER ... OWNER TO" does not include displaying of CURRENT_ROLE,
> CURRENT_USER and SESSION_USER.
>
> The attached patch has the changes to handle the same.

Good catches! Just a few comments:

> +	else if (TailMatchesCS("\\dRp*"))
> +		COMPLETE_WITH_QUERY(Query_for_list_of_publications[0].query);
> +	else if (TailMatchesCS("\\dRs*"))
> +		COMPLETE_WITH_QUERY(Query_for_list_of_subscriptions[0].query);

These are version-specific queries, so should be passed in their
entirety to COMPLETE_WITH_VERSIONED_QUERY() so that psql can pick the
right version, and avoid sending the query at all if the server is too
old.

> +/* add these to Query_for_list_of_roles in OWNER TO contexts */
> +#define Keywords_for_list_of_owner_to_roles \
> +"CURRENT_ROLE", "CURRENT_USER", "SESSION_USER"

I think this would read better without the TO, both in the comment and
the constant name, similar to the below only having GRANT without TO:

>  /* add these to Query_for_list_of_roles in GRANT contexts */
>  #define Keywords_for_list_of_grant_roles \
>  "PUBLIC", "CURRENT_ROLE", "CURRENT_USER", "SESSION_USER"

- ilmari

  3. Re: Miscellaneous tab completion issue fixes

    Michael Paquier <michael@paquier.xyz> — 2022-10-04T03:43:45Z

    On Mon, Oct 03, 2022 at 06:29:32PM +0100, Dagfinn Ilmari Mannsåker wrote:
> vignesh C <vignesh21@gmail.com> writes:
>> +	else if (TailMatchesCS("\\dRp*"))
>> +		COMPLETE_WITH_QUERY(Query_for_list_of_publications[0].query);
>> +	else if (TailMatchesCS("\\dRs*"))
>> +		COMPLETE_WITH_QUERY(Query_for_list_of_subscriptions[0].query);
> 
> These are version-specific queries, so should be passed in their
> entirety to COMPLETE_WITH_VERSIONED_QUERY() so that psql can pick the
> right version, and avoid sending the query at all if the server is too
> old.

+1.

>> +/* add these to Query_for_list_of_roles in OWNER TO contexts */
>> +#define Keywords_for_list_of_owner_to_roles \
>> +"CURRENT_ROLE", "CURRENT_USER", "SESSION_USER"
> 
> I think this would read better without the TO, both in the comment and
> the constant name, similar to the below only having GRANT without TO:

Keywords_for_list_of_grant_roles is used in six code paths, so it
seems to me that there is little gain in having a separate #define
here.  Let's just specify the list of allowed roles (RoleSpec) where
OWNER TO is parsed.
--
Michael

  4. Re: Miscellaneous tab completion issue fixes

    vignesh C <vignesh21@gmail.com> — 2022-10-04T09:30:10Z

    On Tue, 4 Oct 2022 at 09:13, Michael Paquier <michael@paquier.xyz> wrote:
>
> On Mon, Oct 03, 2022 at 06:29:32PM +0100, Dagfinn Ilmari Mannsåker wrote:
> > vignesh C <vignesh21@gmail.com> writes:
> >> +    else if (TailMatchesCS("\\dRp*"))
> >> +            COMPLETE_WITH_QUERY(Query_for_list_of_publications[0].query);
> >> +    else if (TailMatchesCS("\\dRs*"))
> >> +            COMPLETE_WITH_QUERY(Query_for_list_of_subscriptions[0].query);
> >
> > These are version-specific queries, so should be passed in their
> > entirety to COMPLETE_WITH_VERSIONED_QUERY() so that psql can pick the
> > right version, and avoid sending the query at all if the server is too
> > old.
>
> +1.
>

Modified

 >> +/* add these to Query_for_list_of_roles in OWNER TO contexts */
> >> +#define Keywords_for_list_of_owner_to_roles \
> >> +"CURRENT_ROLE", "CURRENT_USER", "SESSION_USER"
> >
> > I think this would read better without the TO, both in the comment and
> > the constant name, similar to the below only having GRANT without TO:
>
> Keywords_for_list_of_grant_roles is used in six code paths, so it
> seems to me that there is little gain in having a separate #define
> here.  Let's just specify the list of allowed roles (RoleSpec) where
> OWNER TO is parsed.

I have removed the macro and specified the allowed roles.

Thanks for the comments, the attached v2 patch has the changes for the same.

Regards,
Vignesh

  5. Re: Miscellaneous tab completion issue fixes

    Dagfinn Ilmari Mannsåker <ilmari@ilmari.org> — 2022-10-04T09:35:25Z

    vignesh C <vignesh21@gmail.com> writes:

> On Tue, 4 Oct 2022 at 09:13, Michael Paquier <michael@paquier.xyz> wrote:
>>
>> On Mon, Oct 03, 2022 at 06:29:32PM +0100, Dagfinn Ilmari Mannsåker wrote:
>> > vignesh C <vignesh21@gmail.com> writes:
>> >> +    else if (TailMatchesCS("\\dRp*"))
>> >> +            COMPLETE_WITH_QUERY(Query_for_list_of_publications[0].query);
>> >> +    else if (TailMatchesCS("\\dRs*"))
>> >> +            COMPLETE_WITH_QUERY(Query_for_list_of_subscriptions[0].query);
>> >
>> > These are version-specific queries, so should be passed in their
>> > entirety to COMPLETE_WITH_VERSIONED_QUERY() so that psql can pick the
>> > right version, and avoid sending the query at all if the server is too
>> > old.
>>
>> +1.
>
> Modified
>
>  >> +/* add these to Query_for_list_of_roles in OWNER TO contexts */
>> >> +#define Keywords_for_list_of_owner_to_roles \
>> >> +"CURRENT_ROLE", "CURRENT_USER", "SESSION_USER"
>> >
>> > I think this would read better without the TO, both in the comment and
>> > the constant name, similar to the below only having GRANT without TO:
>>
>> Keywords_for_list_of_grant_roles is used in six code paths, so it
>> seems to me that there is little gain in having a separate #define
>> here.  Let's just specify the list of allowed roles (RoleSpec) where
>> OWNER TO is parsed.
>
> I have removed the macro and specified the allowed roles.
>
> Thanks for the comments, the attached v2 patch has the changes for the same.

LGTM, +1 to commit.

- ilmari

  6. Re: Miscellaneous tab completion issue fixes

    Michael Paquier <michael@paquier.xyz> — 2022-10-05T02:47:17Z

    On Tue, Oct 04, 2022 at 10:35:25AM +0100, Dagfinn Ilmari Mannsåker wrote:
> LGTM, +1 to commit.

Fine by me, so done.
--
Michael

  7. Re: Miscellaneous tab completion issue fixes

    vignesh C <vignesh21@gmail.com> — 2022-10-05T12:05:38Z

    On Wed, 5 Oct 2022 at 08:17, Michael Paquier <michael@paquier.xyz> wrote:
>
> On Tue, Oct 04, 2022 at 10:35:25AM +0100, Dagfinn Ilmari Mannsåker wrote:
> > LGTM, +1 to commit.
>
> Fine by me, so done.

Thanks for pushing this.

Regards,
Vignesh