Re: Conflict detection and logging in logical replication

On Wed, Aug 7, 2024 at 1:08 PM Zhijie Hou (Fujitsu)
<houzj(dot)fnst(at)fujitsu(dot)com> wrote:
>
> On Wednesday, August 7, 2024 3:00 PM Kuroda, Hayato/黒田 隼人 <kuroda(dot)hayato(at)fujitsu(dot)com> wrote:
> >
> > While playing with the 0003 patch (the patch may not be ready), I found that
> > when the insert_exists event occurred, both apply_error_count and
> > insert_exists_count was counted.
>
> Thanks for testing. 0003 is a separate feature which we might review
> after the 0001 is in a good shape or committed.
>
> >
> > ```
> > -- insert a tuple on the subscriber
> > subscriber =# INSERT INTO tab VALUES (1);
> >
> > -- insert the same tuple on the publisher, which causes insert_exists conflict
> > publisher =# INSERT INTO tab VALUES (1);
> >
> > -- after some time...
> > subscriber =# SELECT * FROM pg_stat_subscription_stats; -[ RECORD
> > 1 ]--------+------
> > subid | 16389
> > subname | sub
> > apply_error_count | 16
> > sync_error_count | 0
> > insert_exists_count | 16
> > update_differ_count | 0
> > update_exists_count | 0
> > update_missing_count | 0
> > delete_differ_count | 0
> > delete_missing_count | 0
> > stats_reset |
> > ```
> >
> > Not tested, but I think this could also happen for the update_exists_count case,
> > or sync_error_count may be counted when the tablesync worker detects the
> > conflict.
> >
> > IIUC, the reason is that pgstat_report_subscription_error() is called in the
> > PG_CATCH part in start_apply() even after ReportApplyConflict(ERROR) is
> > called.
> >
> > What do you think of the current behavior? I wouldn't say I like that the same
> > phenomenon is counted as several events. E.g., in the case of vacuum, the
> > entry seemed to be separated based on the process by backends or
> > autovacuum.
>
> I think this is as expected. When the insert conflicts, it will report an ERROR
> so both the conflict count and error out are incremented which looks reasonable
> to me. The default behavior for each conflict could be different and is
> documented, I think It's clear that insert_exists will cause an ERROR while
> delete_missing or .. will not.
>

I had also observed this behaviour during my testing of stats patch.
But I found this behaviour to be okay. IMO, apply_error_count should
account any error caused during applying and thus should incorporate
insert-exists error-count too.

> In addition, we might support a resolution called "error" which is to report an
> ERROR When facing the specified conflict, it would be a bit confusing to me if
> the apply_error_count Is not incremented on the specified conflict, when I set
> resolution to ERROR.
>
> > I feel the spec is unfamiliar in that only insert_exists and update_exists are
> > counted duplicated with the apply_error_count.
> >
> > An easy fix is to introduce a global variable which is turned on when the conflict
> > is found.
>
> I am not sure about the benefit of changing the current behavior in the patch.
> And it will change the existing behavior, because before the conflict detection
> patch, the apply_error_count is incremented on each unique key violation, while
> after the detection patch, it stops incrementing the apply_error and only
> conflict_count is incremented.
>

thanks
Shveta