On Fri, May 7, 2021 at 6:44 PM vignesh C <vignesh21@gmail.com> wrote:
>
> 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.
The patch was not applying on the head, attached patch which is rebased on HEAD.
Regards,
Vignesh
