On Thu, Aug 26, 2021 at 9:11 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
>
> On Thu, Aug 26, 2021 at 4:42 PM Masahiko Sawada <sawada.mshk@gmail.com> wrote:
> >
> > On Thu, Aug 26, 2021 at 3:09 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
> > > 1.
> > > + if (errarg->rel)
> > > + appendStringInfo(&buf, _(" for replication target relation \"%s.%s\""),
> > > + errarg->rel->remoterel.nspname,
> > > + errarg->rel->remoterel.relname);
> > > +
> > > + if (errarg->remote_attnum >= 0)
> > > + appendStringInfo(&buf, _(" column \"%s\""),
> > > + errarg->rel->remoterel.attnames[errarg->remote_attnum]);
> > >
> > > Isn't it better if 'remote_attnum' check is inside if (errargrel)
> > > check? It will be weird to print column information without rel
> > > information and in the current code, we don't set remote_attnum
> > > without rel. The other possibility could be to have an Assert for rel
> > > in 'remote_attnum' check.
> >
> > Agreed to check 'remote_attnum' inside "if(errargrel)".
> >
>
> Okay, changed accordingly. Additionally, I have changed the code which
> sets timestamp to (unset) when it is 0 so that it won't display the
> timestamp in that case. I have made few other cosmetic changes in the
> attached patch. See and let me know what you think of it?
Thank you for the patch!
Agreed with these changes. The patch looks good to me.
Regards,
--
Masahiko Sawada
EDB: https://www.enterprisedb.com/
