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?
Note - I have just attached the first patch here, once this is
committed we can focus on others.
--
With Regards,
Amit Kapila.
