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>
>
> 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".
2. Add a error case for connect=false and VALIDATE_PUBLICATION = true
--
Regards,
Dilip Kumar
EnterpriseDB: http://www.enterprisedb.com
