On Mon, May 3, 2021 at 10:48 AM Dilip Kumar <dilipbal...@gmail.com> wrote: > > On Sun, May 2, 2021 at 10:04 PM vignesh C <vignes...@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
