Re: Conflict Detection and Resolution

On Fri, Sep 6, 2024 at 2:05 PM Ajin Cherian <itsajin(at)gmail(dot)com> wrote:
>
>
> Thank you for your feedback, Shveta. I've addressed both sets of comments you provided.

Thanks for the patches. I am reviewing v12-patch001, it is WIP. But
please find first set of comments:

1)
src/sgml/logical-replication.sgml:
+ Users have the option to configure a conflict_resolver

Full stop for previous line is missing.

2)
+ For more information on the conflict_types detected and the
supported conflict_resolvers, refer to section CONFLICT RESOLVERS.

We may change to :
For more information on the supported conflict_types and
conflict_resolvers, refer to section CONFLICT RESOLVERS.

3)
src/backend/commands/subscriptioncmds.c:
Line removed. This change is not needed.

static void CheckAlterSubOption(Subscription *sub, const char *option,
bool slot_needs_update, bool isTopLevel);
-

4)

Let's stick to the same comments format as the rest of the file i.e.
first letter in caps.

+ /* first initialise the resolvers with default values */

first --> First
initialise --> initialize

Same for below comments:
+ /* validate the conflict type and resolver */
+ /* update the corresponding resolver for the given conflict type */

Please verify the rest of the file for the same.

5)
Please add below in header of parse_subscription_conflict_resolvers
(similar to parse_subscription_options):

* This function will report an error if mutually exclusive options
are specified.

6)
+ * Warn users if prerequisites are not met.
+ * Initialize with default values.
+ */
+ if (stmt->resolvers)
+ conf_detection_check_prerequisites();
+

Would it be better to move the above call inside
parse_subscription_conflict_resolvers(), then we will have all
resolver related stuff at one place?
Irrespective of whether we move it or not, please remove 'Initialize
with default values.' from above as that is now not done here.

thanks
Shveta