Re: Skipping logical replication transactions on subscriber side - Mailing list pgsql-hackers

On Tue, Aug 10, 2021 at 11:59 AM Amit Kapila <amit.kapila16@gmail.com> wrote:
>
> On Tue, Aug 10, 2021 at 10:37 AM Masahiko Sawada <sawada.mshk@gmail.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
==============================================================

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

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


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.

4.
+
+ TransactionId remote_xid;
+ TimestampTz committs;
+} ApplyErrCallbackArg;

It might be better to add a comment like "remote xact information"
above these structure members.

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))

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.


-- 
With Regards,
Amit Kapila.