RE: Add the replication origin name and commit-LSN to logical replication worker errcontext

On Friday, March 4, 2022 10:09 AM Masahiko Sawada <sawada.mshk@gmail.com> wrote:
> On Thu, Mar 3, 2022 at 10:02 PM Masahiko Sawada
> <sawada.mshk@gmail.com> wrote:
> >
> >
> > I'm updating the patches and will submit them.
> 
> Attached updated version patches.
Thank you for sharing the patch v3.

Few minor comments.


(1) v03-0001, apply_error_callback function

-       /* append transaction information */
-       if (TransactionIdIsNormal(errarg->remote_xid))
+       if (errarg->rel == NULL)
        {
-               appendStringInfo(&buf, _(" in transaction %u"), errarg->remote_xid);

Should write !errarg->rel ?

(2) v03-0002, doc/src/sgml/logical-replication.sgml


+   transaction that conflicts with the existing data.  When a conflict produces
+   an error, it is shown in the subscriber's server logs as follows:
+<screen>
+ERROR:  duplicate key value violates unique constraint "test_pkey"
+DETAIL:  Key (c)=(1) already exists.
+CONTEXT:  processing remote data during "INSERT" for replication target relation "public.test" in transaction 725 committed at LSN 0/14BFA88
+</screen>


We should update the CONTEXT message by using the v3-0001.

(3) v03-0002, doc/src/sgml/logical-replication.sgml


+   The LSN of the transaction that contains the change violating the constraint and
+   the replication origin name can be found from those outputs (LSN 0/14C0378 and
+   replication origin <literal>pg_16395</literal> in the above case).  To skip the
+   transaction, the subscription needs to be disabled temporarily by
+   <command>ALTER SUBSCRIPTION ... DISABLE</command> first. Then, the transaction
+   can be skipped by calling the <link linkend="pg-replication-origin-advance">

The LSN(0/14C0378) is not same as the one in the above error context.
It's recommended to check LSNs directly written in the documentation.

(4) one confirmation

We don't have a TAP test of pg_replication_origin_advance()
for v3, that utilizes this new log in a logical replication setup.
This is because existing tests for this function (in test_decoding) is only for permission check
and argument validation, and we're just changing error message itself.
Is this correct ?


Best Regards,
	Takamichi Osumi