On Fri, May 7, 2021 at 5:44 PM Dilip Kumar <dilipbalaut@gmail.com> wrote:
>
> On Fri, May 7, 2021 at 5:38 PM Bharath Rupireddy
> <bharath.rupireddyforpostgres@gmail.com> wrote:
> >
> > On Fri, May 7, 2021 at 11:50 AM Dilip Kumar <dilipbalaut@gmail.com> wrote:
> > >
> > > On Thu, May 6, 2021 at 7:22 PM vignesh C <vignesh21@gmail.com> wrote:
> > > >
> > >
> > > Some comments:
> > > 1.
> > > I don't see any change in pg_dump.c, don't we need to dump this option?
> >
> > I don't think it is necessary there as the default value of the
> > validate_publication is false, so even if the pg_dump has no mention
> > of the option, then it is assumed to be false while restoring. Note
> > that the validate_publication option is transient (like with other
> > options such as create_slot, copy_data) which means it can't be stored
> > in pg_subscritpion catalogue. Therefore, user specified value can't be
> > fetched once the CREATE/ALTER subscription command is finished. If we
> > were to dump the option, we should be storing it in the catalogue,
> > which I don't think is necessary. Thoughts?
>
> If we are not storing it in the catalog then it does not need to be dumped.
I intentionally did not store this value, I felt we need not persist
this option's value. This value will be false while dumping similar to
other non stored parameters.
> > > 2.
> > > + /* Try to connect to the publisher. */
> > > + wrconn = walrcv_connect(sub->conninfo, true, sub->name, &err);
> > > + if (!wrconn)
> > > + ereport(ERROR,
> > > + (errmsg("could not connect to the publisher: %s", err)));
> > >
> > > Instead of using global wrconn, I think you should use a local variable?
> >
> > Yeah, we should be using local wrconn, otherwise there can be
> > consequences, see the patches at [1]. Thanks for pointing out this.
Modified.
Thanks for the comments, the attached patch has the fix for the same.
Regards,
Vignesh
