On Wed, Aug 28, 2024 at 4:07 PM shveta malik <shveta.ma...@gmail.com> wrote: > > > On Wed, Aug 28, 2024 at 10:30 AM Ajin Cherian <itsa...@gmail.com> wrote: > > > > > The review is WIP. Please find a few comments on patch001. >
More comments on ptach001 in continuation of previous comments: 6) SetDefaultResolvers() can be called from parse_subscription_conflict_resolvers() itself. This will be similar to how parse_subscription_options() sets defaults internally. 7) parse_subscription_conflict_resolvers(): + if (!stmtresolvers) + return; I think we do not need the above, 'foreach' will take care of it. Since we do not have any logic after foreach, we should be good without the above check explicitly added. 8) I think SetSubConflictResolver() should be moved before replorigin_create(). We can insert resolver entries immediately after we insert subscription entries. 9) check_conflict_detection/conf_detection_check_prerequisites shall be moved to conflict.c file. 10) validate_conflict_type_and_resolver(): Please mention in header that: It returns an enum ConflictType corresponding to the conflict type string passed by the caller. 11) UpdateSubConflictResolvers(): 11a) Rename CTR similar to other variables. 11b) Please correct the header as we deal with multiple conflict-types in it instead of 1. Suggestion: Update the subscription's conflict resolvers in pg_subscription_conflict system catalog for the given conflict types. 12) SetSubConflictResolver(): 12a) I think we do not need 'replaces' during INSERT and thus this is not needed: + memset(replaces, false, sizeof(replaces)); 12b) Shouldn't below be outside of loop: + memset(nulls, false, sizeof(nulls)); 13) Shall we rename RemoveSubscriptionConflictBySubid with RemoveSubscriptionConflictResolvers()? 'BySubid' is not needed as we have Subscription in the name and we do not have any other variation of removal. 14) We shall rename pg_subscription_conflict_sub_index to pg_subscription_conflict_confsubid_confrtype_index to give more clarity that it is any index on subid and conftype And SubscriptionConflictSubIndexId to SubscriptionConflictSubidTypeIndexId And SUBSCRIPTIONCONFLICTSUBOID to SUBSCRIPTIONCONFLMAP 15) conflict.h: + See ConflictTypeResolverMap in conflcit.c to find out which all conflcit.c --> conflict.c 16) subscription.sql: 16a) add one more test case for 'fail' scenario where both conflict type and resolver are valid but resolver is not for that particular conflict type. 16b) --try setting resolvers for few types Change to below (similar to other comments) -- ok - valid conflict types and resolvers 16c) -- ok - valid conflict type and resolver maybe change to: -- ok - valid conflict types and resolvers thanks Shveta
