On Fri, Jul 19, 2024 at 2:06 PM shveta malik <shveta.malik@gmail.com> wrote:
>
> On Thu, Jul 18, 2024 at 7:52 AM Zhijie Hou (Fujitsu)
> <houzj.fnst@fujitsu.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
