Обсуждение: Miscellaneous tab completion issue fixes

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

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

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

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

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

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

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