Re: Conflict Detection and Resolution - Mailing list pgsql-hackers

On Fri, Sep 20, 2024 at 8:40 AM Nisha Moond <nisha.moond412@gmail.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:

1)
Please see these 2 errors:

postgres=# create subscription sub2 connection '....' publication pub1
CONFLICT RESOLVER(insert_exists = 'error') WITH (two_phase=true,
streaming=ON, streaming=OFF);
ERROR:  conflicting or redundant options
LINE 1: ...ists='error') WITH (two_phase=true, streaming=ON, streaming=...

                               ^
postgres=# create subscription sub2 connection '....' publication pub1
CONFLICT RESOLVER(insert_exists = 'error', insert_exists = 'error')
WITH (two_phase=true);
ERROR:  duplicate conflict type "insert_exists" found

When we give duplicate options in 'WITH', we get an error as
'conflicting or redundant options' with 'position' pointed out, while
in case of CONFLICT RESOLVER, it is different. Can we review to see if
we can have similar error in CONFLICT RESOLVER as that of WITH?
Perhaps we need to call 'errorConflictingDefElem' from resolver flow.

2)
+static void
+parse_subscription_conflict_resolvers(List *stmtresolvers,
+   ConflictTypeResolver *resolvers)
+{
+ ListCell   *lc;
+ List    *SeenTypes = NIL;
+
+

Remove redundant blank line

3)
parse_subscription_conflict_resolvers():

+ if (stmtresolvers)
+ conf_detection_check_prerequisites();
+
+}

Remove redundant blank line

4)
parse_subscription_conflict_resolvers():
+ resolver = defGetString(defel);
+ type = validate_conflict_type_and_resolver(defel->defname,
+    defGetString(defel));

Shall we use 'resolver' as arg to validate function instead of doing
defGetStringagain?

5)
parse_subscription_conflict_resolvers():

+ /* Update the corresponding resolver for the given conflict type. */
+ resolvers[type].resolver = downcase_truncate_identifier(resolver,
strlen(resolver), false);

Shouldn't we do this before validate_conflict_type_and_resolver()
itself like we do it in GetAndValidateSubsConflictResolverList()? And
do we need downcase_truncate_identifier on defel->defname as well
before we do validate_conflict_type_and_resolver()?

6)
GetAndValidateSubsConflictResolverList() and
parse_subscription_conflict_resolvers() are similar but yet have so
many differences which I  pointed out above. Not a good idea to
maintain 2 such functions. We should have a common parsing function
for both Create and Alter Sub. Can you please review the possibility
of that?

~~

conflict.c:

7)
+
+
+/*
+ * Set default values for CONFLICT RESOLVERS for each conflict type
+ */
+void
+SetDefaultResolvers(ConflictTypeResolver * conflictResolvers)

Remove redundant blank line

8)
 * Set default values for CONFLICT RESOLVERS for each conflict type

Is it better to change to:  Set default resolver for each conflict type

9)
 validate_conflict_type_and_resolver(): Since it is called from other
file as well, shall we rename to ValidateConflictTypeAndResolver()

10)
+ return type;
+
+}
Remove redundant blank line after 'return'

thanks
Shveta