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(dot)mshk(at)gmail(dot)com> wrote:
> On Thu, Mar 3, 2022 at 10:02 PM Masahiko Sawada
> <sawada(dot)mshk(at)gmail(dot)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