From: | vignesh C <vignesh21(at)gmail(dot)com> |
---|---|
To: | Bharath Rupireddy <bharath(dot)rupireddyforpostgres(at)gmail(dot)com> |
Cc: | Alvaro Herrera <alvherre(at)alvh(dot)no-ip(dot)org>, PostgreSQL Hackers <pgsql-hackers(at)lists(dot)postgresql(dot)org> |
Subject: | Re: Added missing tab completion for alter subscription set option |
Date: | 2021-05-23 10:54:59 |
Message-ID: | CALDaNm2J5dL30eH6kUjhMmeWpU+NUAg4B9sGuB+DAf0oBVk66g@mail.gmail.com |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | pgsql-hackers |
On Wed, May 19, 2021 at 2:03 PM Bharath Rupireddy <
bharath(dot)rupireddyforpostgres(at)gmail(dot)com> wrote:
>
> On Tue, May 18, 2021 at 9:21 PM Alvaro Herrera <alvherre(at)alvh(dot)no-ip(dot)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],
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] -
https://www.postgresql.org/message-id/3690759.1621527026%40sss.pgh.pa.us
Regards,
Vignesh
Attachment | Content-Type | Size |
---|---|---|
v4-0001-Add-missing-tab-completion-for-missing-options-in.patch | text/x-patch | 2.8 KB |
From | Date | Subject | |
---|---|---|---|
Next Message | Mark Dilger | 2021-05-23 14:17:17 | Re: Fixing the docs for ALTER SUBSCRIPTION ... ADD/DROP PUBLICATION |
Previous Message | vignesh C | 2021-05-23 10:47:03 | Re: Added missing tab completion for alter subscription set option |