> On Wed, Aug 28, 2024 at 10:30 AM Ajin Cherian <itsajin@gmail.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
