On Mon, May 3, 2021 at 1:46 PM Dilip Kumar <dilipbalaut@gmail.com> wrote:
>
> On Mon, May 3, 2021 at 10:48 AM Dilip Kumar <dilipbalaut@gmail.com> wrote:
> >
> > On Sun, May 2, 2021 at 10:04 PM vignesh C <vignesh21@gmail.com> wrote:
> > >
> > > Thanks for the comments.
> > > The Attached patch has the fixes for the same.
> >
> > I was reviewing the documentation part, I think in the below paragraph
> > we should include validate_publication as well?
> >
> > <varlistentry>
> > <term><literal>connect</literal> (<type>boolean</type>)</term>
> > <listitem>
> > <para>
> > Specifies whether the <command>CREATE SUBSCRIPTION</command>
> > should connect to the publisher at all. Setting this to
> > <literal>false</literal> will change default values of
> > <literal>enabled</literal>, <literal>create_slot</literal> and
> > <literal>copy_data</literal> to <literal>false</literal>.
> > </para>
> >
Modified.
> > I will review/test the other parts of the patch and let you know.
>
> I have reviewed it and it mostly looks good to me. I have some minor
> suggestions though.
>
> 1.
> +/*
> + * Check the specified publication(s) is(are) present in the publisher.
> + */
>
> vs
>
> +
> +/*
> + * Connect to the publisher and check if the publications exist.
> + */
>
> I think the formatting of the comments are not uniform. Some places
> we are using "publication(s) is(are)" whereas other places are just
> "publications".
>
Modified.
> 2. Add a error case for connect=false and VALIDATE_PUBLICATION = true
Added.
Thanks for the comments, attached v7 patch has the fixes for the same.
Thoughts?
Regards,
Vignesh