On Fri, Sep 6, 2024 at 2:05 PM Ajin Cherian <itsa...@gmail.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
