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

On Fri, Mar 4, 2022 at 2:55 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
>
> On Fri, Mar 4, 2022 at 6:40 AM Masahiko Sawada <sawada.mshk@gmail.com> wrote:
> >
> > Attached updated version patches.
> >
>
> The patch looks mostly good to me. Few minor comments:

Thank you for the comments!

> 1. I think we can have an Assert for errarg->origin_name in
> apply_error_callback after checking the command as this function
> assumes that it will always be set.

Added.

> 2. I suggest minor changes in the documentation change:
> When a conflict produces an error, the replication won't proceed, and
> the apply worker will emit the following kind of message to the
> subscriber's server log:
> +<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 from replication origin "pg_16395"
> +</screen>
>
> The LSN of the transaction that contains the change violating the
> constraint and the replication origin name can be found from the
> server log (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">
> <function>pg_replication_origin_advance()</function></link> function
> with the <parameter>node_name</parameter> (i.e.,
> <literal>pg_16395</literal>) and the next LSN of the commit LSN (i.e.,
> LSN 0/14C0379).

Fixed.

On Fri, Mar 4, 2022 at 3:21 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
>
> On Fri, Mar 4, 2022 at 11:45 AM osumi.takamichi@fujitsu.com
> <osumi.takamichi@fujitsu.com> wrote:
> >
> > On Friday, March 4, 2022 2:23 PM Masahiko Sawada <sawada.mshk@gmail.com> wrote:
> > > I've attached updated patches.
> > Hi, thank you for updating the patch.
> >
> > One comment on v4.
> >
> > In v4-0002, we introduce 'commit_lsn' in the ApplyErrorCallbackArg.
> > This member is set for prepare, rollback prepared and stream_abort as well.
> > The new log message format is useful when we have a prepare transaction
> > that keeps failing on the subscriber and want to know the target transaction
> > for the pg_replication_origin_advance(), right ?
> > If this is true, I wasn't sure if the name 'commit_lsn' is the
> > most accurate name for this variable. Should we adjust the name a bit ?
> >
>
> If we want to change this variable name, the other options could be
> 'end_lsn', or 'finish_lsn'.
>

Agreed with 'finish_lsn'.

I've attached updated patches. Please review them.

Regards,

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