On Thu, Aug 26, 2021 at 4:42 PM Masahiko Sawada <sawada.m...@gmail.com> wrote:
>
> On Thu, Aug 26, 2021 at 3:09 PM Amit Kapila <amit.kapil...@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.
v12-0001-Add-logical-change-details-to-logical-replicatio.patch
Description: Binary data
