Added missing tab completion for alter subscription set option

On Fri, May 14, 2021 at 12:00 PM vignesh C <vignesh21@gmail.com> wrote:

Hi,

While I was reviewing one of the logical decoding features, I found
Streaming and binary options were missing in tab completion for the
alter subscription set option, the attached patch has the changes for
the same.
Thoughts?

+1.

Without patch:
postgres=# alter subscription testsub set (S
SLOT_NAME SYNCHRONOUS_COMMIT

With patch:
postgres=# alter subscription testsub set (
BINARY SLOT_NAME STREAMING SYNCHRONOUS_COMMIT

How about ordering the options alphabetically as the tab complete
output anyways shows that way? I'm not sure if that's the practice,
but just a thought.
Change:
+        COMPLETE_WITH("binary", "slot_name", "synchronous_commit",
"streaming");
To:
+        COMPLETE_WITH("binary", "slot_name", "streaming",
"synchronous_commit");

With Regards,
Bharath Rupireddy.
EnterpriseDB: http://www.enterprisedb.com

On Fri, May 14, 2021 at 12:25 PM Bharath Rupireddy
<bharath.rupireddyforpostgres@gmail.com> wrote:

On Fri, May 14, 2021 at 12:00 PM vignesh C <vignesh21@gmail.com> wrote:

Hi,

While I was reviewing one of the logical decoding features, I found
Streaming and binary options were missing in tab completion for the
alter subscription set option, the attached patch has the changes for
the same.
Thoughts?

+1.

Without patch:
postgres=# alter subscription testsub set (S
SLOT_NAME SYNCHRONOUS_COMMIT

With patch:
postgres=# alter subscription testsub set (
BINARY SLOT_NAME STREAMING SYNCHRONOUS_COMMIT

How about ordering the options alphabetically as the tab complete
output anyways shows that way? I'm not sure if that's the practice,
but just a thought.

I did not see any rule for this, but also did not see any harm in
keeping it in alphabetical order, so changed it in the attached patch.

Regards,
Vignesh

On Fri, May 14, 2021 at 6:51 PM vignesh C <vignesh21@gmail.com> wrote:

On Fri, May 14, 2021 at 12:25 PM Bharath Rupireddy
<bharath.rupireddyforpostgres@gmail.com> wrote:

On Fri, May 14, 2021 at 12:00 PM vignesh C <vignesh21@gmail.com> wrote:

Hi,

While I was reviewing one of the logical decoding features, I found
Streaming and binary options were missing in tab completion for the
alter subscription set option, the attached patch has the changes for
the same.
Thoughts?

+1.

Without patch:
postgres=# alter subscription testsub set (S
SLOT_NAME SYNCHRONOUS_COMMIT

With patch:
postgres=# alter subscription testsub set (
BINARY SLOT_NAME STREAMING SYNCHRONOUS_COMMIT

How about ordering the options alphabetically as the tab complete
output anyways shows that way? I'm not sure if that's the practice,
but just a thought.

I did not see any rule for this, but also did not see any harm in
keeping it in alphabetical order, so changed it in the attached patch.

Thanks. Just a few nitpicks:
1) How about patch name: "Add tab completion for ALTER SUBSCRIPTION
SET options streaming and binary"?
2) How about a detailed message: "Tab completion for the options
streaming and binary were missing in case of ALTER SUBSCRIPTION SET
command. This patch adds them."?

You may want to add this in commitfest so that we don't lose track of it.

With Regards,
Bharath Rupireddy.
EnterpriseDB: http://www.enterprisedb.com

On Fri, May 14, 2021 at 7:10 PM Bharath Rupireddy <
bharath.rupireddyforpostgres@gmail.com> wrote:

On Fri, May 14, 2021 at 6:51 PM vignesh C <vignesh21@gmail.com> wrote:

On Fri, May 14, 2021 at 12:25 PM Bharath Rupireddy
<bharath.rupireddyforpostgres@gmail.com> wrote:

On Fri, May 14, 2021 at 12:00 PM vignesh C <vignesh21@gmail.com>

wrote:

Hi,

While I was reviewing one of the logical decoding features, I found
Streaming and binary options were missing in tab completion for the
alter subscription set option, the attached patch has the changes

for

the same.
Thoughts?

+1.

Without patch:
postgres=# alter subscription testsub set (S
SLOT_NAME SYNCHRONOUS_COMMIT

With patch:
postgres=# alter subscription testsub set (
BINARY SLOT_NAME STREAMING

SYNCHRONOUS_COMMIT

How about ordering the options alphabetically as the tab complete
output anyways shows that way? I'm not sure if that's the practice,
but just a thought.

I did not see any rule for this, but also did not see any harm in
keeping it in alphabetical order, so changed it in the attached patch.

Thanks. Just a few nitpicks:
1) How about patch name: "Add tab completion for ALTER SUBSCRIPTION
SET options streaming and binary"?

Modified.

2) How about a detailed message: "Tab completion for the options
streaming and binary were missing in case of ALTER SUBSCRIPTION SET
command. This patch adds them."?

Modified.

You may want to add this in commitfest so that we don't lose track of it.

I have added a commitfest entry at [1]https://commitfest.postgresql.org/33/3116/.
Thanks for the comments, the attached patch has the changes for the same.

[1]: https://commitfest.postgresql.org/33/3116/

Regards,
Vignesh

On Sat, May 15, 2021 at 10:44 AM vignesh C <vignesh21@gmail.com> wrote:

I have added a commitfest entry at [1].
Thanks for the comments, the attached patch has the changes for the same.

[1] - https://commitfest.postgresql.org/33/3116/

Thanks Vignesh. The v3 patch looks good to me. It applies and compiles
well, works as expected i.e. the streaming and binary options are
shown in the tab-complete of the ALTER SUBSCRIPTION SET command. I
have no further comments, hence moving it to "ready for committer"
state.

With Regards,
Bharath Rupireddy.
EnterpriseDB: http://www.enterprisedb.com

On 2021-May-14, vignesh C wrote:

While I was reviewing one of the logical decoding features, I found
Streaming and binary options were missing in tab completion for the
alter subscription set option, the attached patch has the changes for
the same.
Thoughts?

I wish we didn't have to keep knowledge in the psql source on which
option names are to be used for each command. If we had some function
SELECT pg_completion_options('alter subscription set');
that returned the list of options usable for each command, we wouldn't
have to ... psql would just retrieve the list of options for the current
command.

Maintaining such a list does not seem hard -- for example we could just
have a function alongside parse_subscription_option() that returns the
names that are recognized by that one. If we drive the implementation
of both off a single struct, it would never be outdated.

--
�lvaro Herrera 39�49'30"S 73�17'W
"[PostgreSQL] is a great group; in my opinion it is THE best open source
development communities in existence anywhere." (Lamar Owen)

On Tue, May 18, 2021 at 9:21 PM Alvaro Herrera <alvherre@alvh.no-ip.org> wrote:

On 2021-May-14, vignesh C wrote:

While I was reviewing one of the logical decoding features, I found
Streaming and binary options were missing in tab completion for the
alter subscription set option, the attached patch has the changes for
the same.
Thoughts?

I wish we didn't have to keep knowledge in the psql source on which
option names are to be used for each command. If we had some function
SELECT pg_completion_options('alter subscription set');
that returned the list of options usable for each command, we wouldn't
have to ... psql would just retrieve the list of options for the current
command.

Maintaining such a list does not seem hard -- for example we could just
have a function alongside parse_subscription_option() that returns the
names that are recognized by that one. If we drive the implementation
of both off a single struct, it would never be outdated.

Yeah, having something similar to table_storage_parameters works better.

While on this, I found that all the options are not listed for CREATE
SUBSCRIPTION command in tab-complete.c, missing ones are binary and
streaming:
else if (HeadMatches("CREATE", "SUBSCRIPTION") && TailMatches("WITH", "("))
COMPLETE_WITH("copy_data", "connect", "create_slot", "enabled",
"slot_name", "synchronous_commit");

Similarly, CREATE and ALTER PUBLICATION don't have
publish_via_partition_root option:
else if (HeadMatches("CREATE", "PUBLICATION") && TailMatches("WITH", "("))
COMPLETE_WITH("publish");

I think having some structures like below in subscriptioncmds.h,
publicationcmds.h and using them in tab-complete.c would make more
sense.

static const char *const create_subscription_params[] = {
"copy_data",
"create_slot",
"enabled",
"slot_name",
"synchronous_commit",
"binary",
"connect",
"streaming",
NULL
}

static const char *const alter_subscription_set_params[] = {
"binary",
"slot_name",
"streaming",
"synchronous_commit",
NULL
}

static const char *const create_or_alter_publication_params[] = {
"publish",
"publish_via_partition_root"
NULL
}

With Regards,
Bharath Rupireddy.
EnterpriseDB: http://www.enterprisedb.com

On Tue, May 18, 2021 at 9:20 PM Alvaro Herrera <alvherre@alvh.no-ip.org> wrote:

On 2021-May-14, vignesh C wrote:

While I was reviewing one of the logical decoding features, I found
Streaming and binary options were missing in tab completion for the
alter subscription set option, the attached patch has the changes for
the same.
Thoughts?

I wish we didn't have to keep knowledge in the psql source on which
option names are to be used for each command. If we had some function
SELECT pg_completion_options('alter subscription set');
that returned the list of options usable for each command, we wouldn't
have to ... psql would just retrieve the list of options for the current
command.

Maintaining such a list does not seem hard -- for example we could just
have a function alongside parse_subscription_option() that returns the
names that are recognized by that one. If we drive the implementation
of both off a single struct, it would never be outdated.

I like the idea of maintaining a common list, that will also prevent
options getting missed in the future. I will work on this and provide
a patch for it.

Regards,
Vignesh

On Tue, May 18, 2021 at 9:20 PM Alvaro Herrera <alvherre@alvh.no-ip.org> wrote:

On 2021-May-14, vignesh C wrote:

While I was reviewing one of the logical decoding features, I found
Streaming and binary options were missing in tab completion for the
alter subscription set option, the attached patch has the changes for
the same.
Thoughts?

I wish we didn't have to keep knowledge in the psql source on which
option names are to be used for each command. If we had some function
SELECT pg_completion_options('alter subscription set');
that returned the list of options usable for each command, we wouldn't
have to ... psql would just retrieve the list of options for the current
command.

Maintaining such a list does not seem hard -- for example we could just
have a function alongside parse_subscription_option() that returns the
names that are recognized by that one. If we drive the implementation
of both off a single struct, it would never be outdated.

On further analysis, I felt that as psql is a front end client, we
should not put any dependency on backend code. I felt that might be
the reason it has been coded to mention the options directly in
tab-complete instead of having any dependency on backend code. we
could have the common module to maintain the options and have both
frontend and backend access it or Should we retain the changes like
the earlier patch.
Thoughts?

Regards,
Vignesh

vignesh C <vignesh21@gmail.com> writes:

On Tue, May 18, 2021 at 9:20 PM Alvaro Herrera <alvherre@alvh.no-ip.org> wrote:

I wish we didn't have to keep knowledge in the psql source on which
option names are to be used for each command. If we had some function
SELECT pg_completion_options('alter subscription set');
that returned the list of options usable for each command, we wouldn't
have to ... psql would just retrieve the list of options for the current
command.

On further analysis, I felt that as psql is a front end client, we
should not put any dependency on backend code. I felt that might be
the reason it has been coded to mention the options directly in
tab-complete instead of having any dependency on backend code.

Well, the problem with Alvaro's proposal is how do you square it
with psql's need to support back versions of the server. Maybe
you could code tab-complete.c like "if server >= v15 then do X
else do Y", but since Y would be largely duplicative of the
server-side knowledge accessed by X, you haven't really gotten
rid of the two-places-that-know-this issue. And I'm afraid that
tab-complete.c would become even more of a mess than it is now;
although maybe somebody can see a cute way to avoid that.

regards, tom lane

On Thu, May 20, 2021 at 9:40 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:

vignesh C <vignesh21@gmail.com> writes:

On Tue, May 18, 2021 at 9:20 PM Alvaro Herrera <alvherre@alvh.no-ip.org> wrote:

I wish we didn't have to keep knowledge in the psql source on which
option names are to be used for each command. If we had some function
SELECT pg_completion_options('alter subscription set');
that returned the list of options usable for each command, we wouldn't
have to ... psql would just retrieve the list of options for the current
command.

On further analysis, I felt that as psql is a front end client, we
should not put any dependency on backend code. I felt that might be
the reason it has been coded to mention the options directly in
tab-complete instead of having any dependency on backend code.

Well, the problem with Alvaro's proposal is how do you square it
with psql's need to support back versions of the server. Maybe
you could code tab-complete.c like "if server >= v15 then do X
else do Y", but since Y would be largely duplicative of the
server-side knowledge accessed by X, you haven't really gotten
rid of the two-places-that-know-this issue. And I'm afraid that
tab-complete.c would become even more of a mess than it is now;
although maybe somebody can see a cute way to avoid that.

In my opinion let's not make that change as part of this fix. I think
we can fix the existing problem with the existing way of just
including the options directly in the tab-complete client code because
the new design has an impact on the older versions and also could end
up in duplication like Tom Lane had pointed out. We can start a new
thread for this and try to get others' opinions on it.

Regards,
Vignesh

On Wed, May 19, 2021 at 2:03 PM Bharath Rupireddy <
bharath.rupireddyforpostgres@gmail.com> wrote:

On Tue, May 18, 2021 at 9:21 PM Alvaro Herrera <alvherre@alvh.no-ip.org>

wrote:

On 2021-May-14, vignesh C wrote:

While I was reviewing one of the logical decoding features, I found
Streaming and binary options were missing in tab completion for the
alter subscription set option, the attached patch has the changes for
the same.
Thoughts?

I wish we didn't have to keep knowledge in the psql source on which
option names are to be used for each command. If we had some function
SELECT pg_completion_options('alter subscription set');
that returned the list of options usable for each command, we wouldn't
have to ... psql would just retrieve the list of options for the current
command.

Maintaining such a list does not seem hard -- for example we could just
have a function alongside parse_subscription_option() that returns the
names that are recognized by that one. If we drive the implementation
of both off a single struct, it would never be outdated.

Yeah, having something similar to table_storage_parameters works better.

While on this, I found that all the options are not listed for CREATE
SUBSCRIPTION command in tab-complete.c, missing ones are binary and
streaming:
else if (HeadMatches("CREATE", "SUBSCRIPTION") && TailMatches("WITH",

"("))

COMPLETE_WITH("copy_data", "connect", "create_slot", "enabled",
"slot_name", "synchronous_commit");

Modified.

Similarly, CREATE and ALTER PUBLICATION don't have
publish_via_partition_root option:
else if (HeadMatches("CREATE", "PUBLICATION") && TailMatches("WITH",

"("))

COMPLETE_WITH("publish");

Modified.

I think having some structures like below in subscriptioncmds.h,
publicationcmds.h and using them in tab-complete.c would make more
sense.

This approach has few disadvantages that Tom Lane has pointed out in [1]/messages/by-id/3690759.1621527026@sss.pgh.pa.us,
Let's use the existing way of adding options directly for tab completion.

Thanks for the comments, Attached v4 patch has the fixes for the same.
[1]: /messages/by-id/3690759.1621527026@sss.pgh.pa.us
/messages/by-id/3690759.1621527026@sss.pgh.pa.us

Regards,
Vignesh

On Sun, May 23, 2021 at 04:24:59PM +0530, vignesh C wrote:

/* Complete "CREATE SUBSCRIPTION <name> ...  WITH ( <opt>" */
else if (HeadMatches("CREATE", "SUBSCRIPTION") && TailMatches("WITH", "("))
-		COMPLETE_WITH("copy_data", "connect", "create_slot", "enabled",
-					  "slot_name", "synchronous_commit");
+		COMPLETE_WITH("binary", "copy_data", "connect", "create_slot",
+					  "enabled", "slot_name", "streaming",
+					  "synchronous_commit");

"copy_data" and "connect" need to be reversed. Applied.
--
Michael

On Fri, Jun 11, 2021 at 12:27 PM Michael Paquier <michael@paquier.xyz> wrote:

On Sun, May 23, 2021 at 04:24:59PM +0530, vignesh C wrote:

/* Complete "CREATE SUBSCRIPTION <name> ...  WITH ( <opt>" */
else if (HeadMatches("CREATE", "SUBSCRIPTION") && TailMatches("WITH", "("))
-             COMPLETE_WITH("copy_data", "connect", "create_slot", "enabled",
-                                       "slot_name", "synchronous_commit");
+             COMPLETE_WITH("binary", "copy_data", "connect", "create_slot",
+                                       "enabled", "slot_name", "streaming",
+                                       "synchronous_commit");

"copy_data" and "connect" need to be reversed. Applied.

Thanks for committing this.

Regards,
Vignesh