Miscellaneous tab completion issue fixes

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