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

I have tried the below statement to check existing behavior:
create subscription sub1 connection 'dbname=postgres' publication pub1
with (streaming = on, streaming=off);
ERROR: conflicting or redundant options
LINE 1: ...=postgres' publication pub1 with (streaming = on, streaming=...

So duplicate options are not allowed. If we see any challenges to
follow same for resolvers then we can discuss but it seems better to
follow the existing behavior of other subscription options.

Also, the behavior for CREATE/ALTER should be the same.

--
With Regards,
Amit Kapila.