Re: Conflict Detection and Resolution

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.

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

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"

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

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.

thanks
Shveta