Re: Conflict Detection and Resolution

On Tue, Jul 30, 2024 at 2:19 PM shveta malik <shveta(dot)malik(at)gmail(dot)com> wrote:

> On Fri, Jul 26, 2024 at 9:50 AM Ajin Cherian <itsajin(at)gmail(dot)com> wrote:
> >>
> > Please find v7 patch-set, the changes are:
> >
>
> Thanks Ajin for working on this. Please find few comments:
>
> 1)
> parse_subscription_conflict_resolvers():
> Here we loop in this function to find the given conflict type in the
> supported list and error out if conflict-type is not valid. Also we
> call validate_conflict_type_and_resolver() which again validates
> conflict-type. I would recommend to loop 'stmtresolvers' in parse
> function and then read each type and resolver and pass that to
> validate_conflict_type_and_resolver(). Avoid double validation.
>
>
I have modified this as per comment.

> 2)
> SetSubConflictResolver():
> It works well, but it does not look apt that the 'resolvers' passed to
> this function by the caller is an array and this function knows the
> array range and traverse from CT_MIN to CT_MAX assuming this array
> maps directly to ConflictType. I think it would be better to have it
> passed as a list and then SetSubConflictResolver() traverse the list
> without knowing the range of it. Similar to what we do in
> alter-sub-flow in and around UpdateSubConflictResolvers().
>
>
I have kept the array as it requires that all conflict resolvers be set, if
not provided by the user then default needs to be used. However, I have
modified SetSubConflictResolver such that it takes in the size of the array
and does not assume it.

3)
> When I execute 'alter subscription ..(detect_conflict=on)' for a
> subscription which *already* has detect_conflict as ON, it tries to
> reset resolvers to default and ends up in error. It should actually be
> no-op in this particular situation and should not reset resolvers to
> default.
>
> postgres=# alter subscription sub1 set (detect_conflict=on);
> WARNING: Using default conflict resolvers
> ERROR: duplicate key value violates unique constraint
> "pg_subscription_conflict_sub_index"
>
>
fixed

> 4)
> Do we need SUBSCRIPTIONCONFLICTOID cache? We are not using it
> anywhere. Shall we remove this and the corresponding index?
>
>
We are using the index but not the cache, so removing the cache.

> 5)
> RemoveSubscriptionConflictBySubid().
> --We can remove extra blank line before table_open.
> --We can get rid of curly braces around CatalogTupleDelete() as it is
> a single line in loop.
>
>
fixed.

On Tue, Jul 30, 2024 at 8:34 PM Dilip Kumar <dilipbalaut(at)gmail(dot)com> wrote:

> On Fri, Jul 26, 2024 at 9:50 AM Ajin Cherian <itsajin(at)gmail(dot)com> wrote:
>
> Comment in 0002,
>
> 1) I do not see any test case that set a proper conflict type and
> conflict resolver, all tests either give incorrect conflict
> type/conflict resolver or the conflict resolver is ignored
>

fixed.

I've also fixed a cfbot error due to patch 0001. Rebase of table resolver
patch is still pending, will try and target that in the next patch-set.

regards,
Ajin Cherian
Fujitsu Australia