Re: Conflict Detection and Resolution

On Thu, Sep 26, 2024 at 2:57 PM shveta malik <shveta(dot)malik(at)gmail(dot)com> wrote:
>
> On Fri, Sep 20, 2024 at 8:40 AM Nisha Moond <nisha(dot)moond412(at)gmail(dot)com> wrote:
> >
> > Thanks for the review.
> > Here is the v14 patch-set fixing review comments in [1] and [2].
> >
>
> Thanks for the patches. I am reviewing patch001, it is WIP, but please
> find initial set of comments:
>

Please find next set of comments on patch001:

11)
conflict.c
#include "access/tableam.h" (existing)
#include "replication/logicalproto.h" (added by patch002)

Above 2 are not needed. The code compiles without these. I think the
first one has become redundant due to inclusion of other header files
which indirectly include this.

12)
create_subscription.sgml:
+ apply_remote (enum)
+ This resolver applies the remote change. It can be used for
insert_exists, update_exists, update_origin_differs and
delete_origin_differs. It is the default resolver for insert_exists
and update_exists.

Wrong info, it is default for update_origin_differs and delete_origin_differs

13)
alter_subscription.sgml:
Synopsis:
+ ALTER SUBSCRIPTION name RESET CONFLICT RESOLVER FOR (conflict_type)

we don't support parenthesis in the syntax. So please correct the doc.
postgres=# ALTER SUBSCRIPTION sub1 RESET CONFLICT RESOLVER FOR
('insert_exists');
ERROR: syntax error at or near "("

14)
alter_subscription.sgml:
+ CONFLICT RESOLVER ( conflict_type [= conflict_resolver] [, ... ] )
+ This clause alters either the default conflict resolvers or those
set by CREATE SUBSCRIPTION. Refer to section CONFLICT RESOLVERS for
the details on supported conflict_types and conflict_resolvers.

+ conflict_type
+ The conflict type being reset to its default resolver setting. For
details on conflict types and their default resolvers, refer to
section CONFLICT RESOLVERS

a) These details seem problematic. Shouldn't we have RESET as heading
similar to SKIP and then try explaining both ALL and conflict_type
under that. Above seems we are trying to explain conflict_type of
'CONFLICT RESOLVER ( conflict_type [= conflict_resolver]' subcommand
while
giving details of RESET subcommand.

b) OTOH, 'CONFLICT RESOLVER ( conflict_type [= conflict_resolver]'
should have its own explanation of conflict_type and conflict_resolver
parameters.

15)
logical-replication.sgml:

Existing:
+ Additional logging is triggered in various conflict scenarios, each
identified as a conflict type, and the conflict statistics are
collected (displayed in the pg_stat_subscription_stats view). Users
have the option to configure a conflict_resolver for each
conflict_type when creating a subscription. For more information on
the supported conflict_types detected and conflict_resolvers, refer to
section CONFLICT RESOLVERS.

Suggestion:
Additional logging is triggered for various conflict scenarios, each
categorized by a specific conflict type, with conflict statistics
being gathered and displayed in the pg_stat_subscription_stats view.
Users can configure a conflict_resolver for each conflict_type when
creating a subscription.
For more details on the supported conflict types and corresponding
conflict resolvers, refer to the section on <CONFLICT RESOLVERS>.

thanks
Shveta