Re: Conflict detection and logging in logical replication

On Fri, Jul 19, 2024 at 2:06 PM shveta malik <shveta(dot)malik(at)gmail(dot)com> wrote:
>
> On Thu, Jul 18, 2024 at 7:52 AM Zhijie Hou (Fujitsu)
> <houzj(dot)fnst(at)fujitsu(dot)com> wrote:
> >
> > Attach the V5 patch set which changed the following.
>

Please find last batch of comments on v5:

patch001:
1)
create subscription sub1 ... (detect_conflict=true);
I think it will be good to give WARNING here indicating that
detect_conflict is enabled but track_commit_timestamp is disabled and
thus few conflicts detection may not work. (Rephrase as apt)

2)
013_partition.pl: Since we have added update_differ testcase here, we
shall add delete_differ as well. And in some file where appropriate,
we shall add update_exists test as well.

3)
013_partition.pl (#799,802):
For update_missing and delete_missing, we have log verification format
as 'qr/conflict delete_missing/update_missing detected on relation '.
But for update_differ, we do not capture "conflict update_differ
detected on relation ...". We capture only the DETAIL.
I think we should be consistent and capture conflict name here as well.

patch002:

4)
pg_stat_get_subscription_stats():

---------
/* conflict count */
for (int nconflict = 0; nconflict < CONFLICT_NUM_TYPES; nconflict++)
values[i + nconflict] = Int64GetDatum(subentry->conflict_count[nconflict]);

i += CONFLICT_NUM_TYPES;
---------

Can't we do values[i++] here as well (instead of values[i +
nconflict])? Then we don't need to do 'i += CONFLICT_NUM_TYPES'.

5)
026_stats.pl:
Wherever we are checking this: 'Apply and Sync errors are > 0 and
reset timestamp is NULL', we need to check update_exssts_count as well
along with other counts.

thanks
Shveta