Re: Conflict Detection and Resolution - Mailing list pgsql-hackers
From | shveta malik |
---|---|
Subject | Re: Conflict Detection and Resolution |
Date | |
Msg-id | CAJpy0uBS1_wKzmhP9uHeFYY6m67Tq16rTR9XAyT=w6db5AyTXQ@mail.gmail.com Whole thread Raw |
In response to | Conflict Detection and Resolution (shveta malik <shveta.malik@gmail.com>) |
Responses |
Re: Conflict Detection and Resolution
|
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
pgsql-hackers by date: