Thread: Added missing tab completion for alter subscription set option

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?

Regards,
Vignesh

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 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 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