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

On Fri, Mar 4, 2022 at 11:27 AM osumi(dot)takamichi(at)fujitsu(dot)com
<osumi(dot)takamichi(at)fujitsu(dot)com> wrote:
>
> 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 ?

I think either should be fine.

>
> (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.

Right, I missed checking and updating it.

>
> (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 ?

Yeah, I think it’s a good idea to add test in general but I don't
think we should include the tests for skipping a transaction by using
pg_replication_origin() in this patch because it's an existing way and
upcoming ALTER SUBSCRIPTION SKIP patch includes the tests and it's
more appropriate way. But if others also think it should do that too
along with this update, I'm happy to add tests.

I've attached updated patches.

Regards,

--
Masahiko Sawada
EDB: https://www.enterprisedb.com/