On Mon, Jul 4, 2022 at 9:23 AM Amit Kapila <amit.kapila16@gmail.com> wrote:
>
> On Mon, Jul 4, 2022 at 3:59 AM Peter Smith <smithpb2250@gmail.com> wrote:
> >
> > On Mon, Jul 4, 2022 at 12:59 AM vignesh C <vignesh21@gmail.com> wrote:
> > ...
> > > > 2.
> > > > /* ALTER SUBSCRIPTION <name> SET ( */
> > > > else if (HeadMatches("ALTER", "SUBSCRIPTION", MatchAny) &&
> > > > TailMatches("SET", "("))
> > > > - COMPLETE_WITH("binary", "slot_name", "streaming",
> > > > "synchronous_commit", "disable_on_error");
> > > > + COMPLETE_WITH("binary", "origin", "slot_name", "streaming",
> > > > "synchronous_commit", "disable_on_error");
> > > > /* ALTER SUBSCRIPTION <name> SKIP ( */
> > > > else if (HeadMatches("ALTER", "SUBSCRIPTION", MatchAny) &&
> > > > TailMatches("SKIP", "("))
> > > > COMPLETE_WITH("lsn");
> > > > @@ -3152,7 +3152,7 @@ psql_completion(const char *text, int start, int end)
> > > > /* Complete "CREATE SUBSCRIPTION <name> ... WITH ( <opt>" */
> > > > else if (HeadMatches("CREATE", "SUBSCRIPTION") && TailMatches("WITH", "("))
> > > > COMPLETE_WITH("binary", "connect", "copy_data", "create_slot",
> > > > - "enabled", "slot_name", "streaming",
> > > > + "enabled", "origin", "slot_name", "streaming",
> > > > "synchronous_commit", "two_phase", "disable_on_error");
> > > >
> > > > Why do you choose to add a new option in-between other parameters
> > > > instead of at the end which we normally do? The one possible reason I
> > > > can think of is that all the parameters at the end are boolean so you
> > > > want to add this before those but then why before slot_name, and again
> > > > I don't see such a rule being followed for other parameters.
> > >
> > > I was not sure if it should be maintained in alphabetical order,
> > > anyway since the last option "disable_on_error" is at the end, I have
> > > changed it to the end.
> > >
> >
> > Although it seems it is not a hard rule, mostly the COMPLETE_WITH are
> > coded using alphabetical order. Anyway, I think that was a clear
> > intention here too since 13 of 14 parameters were already in
> > alphabetical order; it is actually only that "disable_on_error"
> > parameter that was misplaced; not the new "origin" parameter.
> >
>
> Agreed, but let's not change disable_on_error as part of this patch.
I have changed this in the v28 patch attached at [1].
[1] - https://www.postgresql.org/message-id/CALDaNm3MNK2hMYroTiHGS9HkSxiA-az1QC1mpa0YwDZ8nGmmZg%40mail.gmail.com
Regards,
Vignesh