RE: Skipping logical replication transactions on subscriber side

On Thursday, August 12, 2021 1:53 PM Masahiko Sawada <sawada(dot)mshk(at)gmail(dot)com> wrote:
>
> I've attached the updated patches. FYI I've included the patch
> (v8-0005) that fixes the assertion failure during shared fileset
> cleanup to make cfbot tests happy.

Hi

Thanks for your patch. I met a problem when using it. The log is not what I expected in some cases, but in streaming mode, they work well.

For example:
------publisher------
create table test (a int primary key, b varchar);
create publication pub for table test;

------subscriber------
create table test (a int primary key, b varchar);
insert into test values (10000);
create subscription sub connection 'dbname=postgres port=5432' publication pub with(streaming=on);

------publisher------
insert into test values (10000);

Subscriber log:
2021-08-17 14:24:43.415 CST [3630341] ERROR: duplicate key value violates unique constraint "test_pkey"
2021-08-17 14:24:43.415 CST [3630341] DETAIL: Key (a)=(10000) already exists.

It didn't give more context info generated by apply_error_callback function.

In streaming mode(which worked as I expected):
------publisher------
INSERT INTO test SELECT i, md5(i::text) FROM generate_series(1, 10000) s(i);

Subscriber log:
2021-08-17 14:26:26.521 CST [3630510] ERROR: duplicate key value violates unique constraint "test_pkey"
2021-08-17 14:26:26.521 CST [3630510] DETAIL: Key (a)=(10000) already exists.
2021-08-17 14:26:26.521 CST [3630510] CONTEXT: processing remote data during "INSERT" for replication target relation "public.test" in transaction id 710 with commit timestamp 2021-08-17 14:26:26.403214+08

I looked into it briefly and thought it was related to some code in
apply_dispatch function. It set callback when apply_error_callback_arg.command
is 0, and reset the callback back at the end of the function. But
apply_error_callback_arg.command was not reset to 0, so it won't set callback
when calling apply_dispatch function next time.

I tried to fix it with the following change, thoughts?

@@ -2455,7 +2455,10 @@ apply_dispatch(StringInfo s)

/* Pop the error context stack */
if (set_callback)
+ {
error_context_stack = errcallback.previous;
+ apply_error_callback_arg.command = 0;
+ }
}

Besides, if we make the changes like this, do we still need to reset
apply_error_callback_arg.command in reset_apply_error_context_info function?

Regards
Tang