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