Re: Conflict Detection and Resolution

> 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.

1)
logical-repliction.sgmlL

+ Additional logging is triggered for specific conflict_resolvers.
Users can also configure conflict_types while creating the
subscription. Refer to section CONFLICT RESOLVERS for details on
conflict_types and conflict_resolvers.

Can we please change it to:

Additional logging is triggered in various conflict scenarios, each
identified as a conflict type. Users have the option to configure a
conflict resolver for each conflict type when creating a subscription.
For more information on the conflict types detected and the supported
conflict resolvers, refer to the section <CONFLICT RESOLVERS>

2)
SetSubConflictResolver

+ for (type = 0; type < resolvers_cnt; type++)

'type' does not look like the correct name here. The variable does not
state conflict_type, it is instead a resolver-array-index, so please
rename accordingly. Maybe idx or res_idx?

3)
CreateSubscription():

+ if (stmt->resolvers)
+ check_conflict_detection();

3a) We can have a comment saying warn users if prerequisites are not met.

3b) Also, I do not find the name 'check_conflict_detection'
appropriate. One suggestion could be
'conf_detection_check_prerequisites' (similar to
replorigin_check_prerequisites)

3c) We can move the below comment after check_conflict_detection() as
it makes more sense there.
/*
* Parse and check conflict resolvers. Initialize with default values
*/

4)
Should we allow repetition/duplicates of 'conflict_type=..' in CREATE
and ALTER SUB? As an example:
ALTER SUBSCRIPTION sub1 CONFLICT RESOLVER (insert_exists =
'apply_remote', insert_exists = 'error');

Such a repetition works for Create-Sub but gives some internal error
for alter-sub. (ERROR: tuple already updated by self). Behaviour
should be the same for both. And if we give an error, it should be
some user understandable one. But I would like to know the opinions of
others. Shall it give an error or the last one should be accepted as
valid configuration in case of repetition?

5)
GetAndValidateSubsConflictResolverList():
+ ConflictTypeResolver *CTR = NULL;

We can change the name to a more appropriate one similar to other
variables. It need not be in all capital.

thanks
Shveta