Re: Conflict Detection and Resolution

On Wed, Aug 28, 2024 at 4:07 PM shveta malik <shveta(dot)malik(at)gmail(dot)com> wrote:
>
> > On Wed, Aug 28, 2024 at 10:30 AM Ajin Cherian <itsajin(at)gmail(dot)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