Re: Skipping logical replication transactions on subscriber side

On Tue, Aug 10, 2021 at 7:18 PM Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
>
> On Tue, Aug 10, 2021 at 11:59 AM Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
> >
> > On Tue, Aug 10, 2021 at 10:37 AM Masahiko Sawada <sawada(dot)mshk(at)gmail(dot)com> wrote:
> > >
> > > I've attached the latest patches that incorporated all comments I got
> > > so far. Please review them.
> > >
> >
> > I am not able to apply the latest patch
> > (v6-0001-Add-errcontext-to-errors-happening-during-applyin) on HEAD,
> > getting the below error:
> >
>
> Few comments on v6-0001-Add-errcontext-to-errors-happening-during-applyin

Thank you for the comments!

> ==============================================================
>
> 1. While applying DML operations, we are setting up the error context
> multiple times due to which the context information is not
> appropriate. The first is set in apply_dispatch and then during
> processing, we set another error callback slot_store_error_callback in
> slot_store_data and slot_modify_data. When I forced one of the errors
> in slot_store_data(), it displays the below information in CONTEXT
> which doesn't make much sense.
>
> 2021-08-10 15:16:39.887 IST [6784] ERROR: incorrect binary data
> format in logical replication column 1
> 2021-08-10 15:16:39.887 IST [6784] CONTEXT: processing remote data
> for replication target relation "public.test1" column "id"
> during apply of "INSERT" for relation "public.test1" in
> transaction with xid 740 committs 2021-08-10 14:44:38.058174+05:30

Yes, but we cannot change the error context message depending on other
error context messages. So it seems hard to construct a complete
sentence in the context message that is okay in terms of English
grammar. Is the following message better?

CONTEXT: processing remote data for replication target relation
"public.test1" column “id"
applying "INSERT" for relation "public.test1” in transaction
with xid 740 committs 2021-08-10 14:44:38.058174+05:30

>
> 2.
> I think we can slightly change the new context information as below:
> Before
> during apply of "INSERT" for relation "public.test1" in transaction
> with xid 740 committs 2021-08-10 14:44:38.058174+05:30
> After
> during apply of "INSERT" for relation "public.test1" in transaction id
> 740 with commit timestamp 2021-08-10 14:44:38.058174+05:30

Fixed.

>
> 3.
> +/* Struct for saving and restoring apply information */
> +typedef struct ApplyErrCallbackArg
> +{
> + LogicalRepMsgType command; /* 0 if invalid */
> +
> + /* Local relation information */
> + char *nspname;
> + char *relname;
>
> ...
> ...
>
> +
> +static ApplyErrCallbackArg apply_error_callback_arg =
> +{
> + .command = 0,
> + .relname = NULL,
> + .nspname = NULL,
>
> Let's initialize the struct members in the order they are declared.
> The order of relname and nspname should be another way.

Fixed.

> 4.
> +
> + TransactionId remote_xid;
> + TimestampTz committs;
> +} ApplyErrCallbackArg;
>
> It might be better to add a comment like "remote xact information"
> above these structure members.

Fixed.

>
> 5.
> +static void
> +apply_error_callback(void *arg)
> +{
> + StringInfoData buf;
> +
> + if (apply_error_callback_arg.command == 0)
> + return;
> +
> + initStringInfo(&buf);
>
> At the end of this call, it is better to free this (pfree(buf.data))

Fixed.

>
> 6. In the commit message, you might want to indicate that this
> additional information can be used by the future patch to skip the
> conflicting transaction.

Fixed.

I've attached the new patches. Please review them.

Regards,

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