From: | shveta malik <shveta(dot)malik(at)gmail(dot)com> |
---|---|
To: | Ajin Cherian <itsajin(at)gmail(dot)com> |
Cc: | Nisha Moond <nisha(dot)moond412(at)gmail(dot)com>, Amit Kapila <amit(dot)kapila16(at)gmail(dot)com>, Dilip Kumar <dilipbalaut(at)gmail(dot)com>, Ashutosh Bapat <ashutosh(dot)bapat(dot)oss(at)gmail(dot)com>, Jan Wieck <jan(at)wi3ck(dot)info>, Tomas Vondra <tomas(dot)vondra(at)enterprisedb(dot)com>, pgsql-hackers <pgsql-hackers(at)postgresql(dot)org>, "Zhijie Hou (Fujitsu)" <houzj(dot)fnst(at)fujitsu(dot)com>, shveta malik <shveta(dot)malik(at)gmail(dot)com> |
Subject: | Re: Conflict Detection and Resolution |
Date: | 2024-08-29 04:49:50 |
Message-ID: | CAJpy0uAV-pd5=34XEyVb-vdmGhchOjGMzKcGE2Dm4PYQv2JJYw@mail.gmail.com |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | pgsql-hackers |
On Wed, Aug 28, 2024 at 4:07 PM shveta malik <shveta(dot)malik(at)gmail(dot)com> wrote:
>
> > On Wed, Aug 28, 2024 at 10:30 AM Ajin Cherian <itsajin(at)gmail(dot)com> wrote:
> > >
>
> The review is WIP. Please find a few comments on patch001.
>
More comments on ptach001 in continuation of previous comments:
6)
SetDefaultResolvers() can be called from
parse_subscription_conflict_resolvers() itself. This will be similar
to how parse_subscription_options() sets defaults internally.
7)
parse_subscription_conflict_resolvers():
+ if (!stmtresolvers)
+ return;
I think we do not need the above, 'foreach' will take care of it.
Since we do not have any logic after foreach, we should be good
without the above check explicitly added.
8)
I think SetSubConflictResolver() should be moved before
replorigin_create(). We can insert resolver entries immediately after
we insert subscription entries.
9)
check_conflict_detection/conf_detection_check_prerequisites shall be
moved to conflict.c file.
10)
validate_conflict_type_and_resolver():
Please mention in header that:
It returns an enum ConflictType corresponding to the conflict type
string passed by the caller.
11)
UpdateSubConflictResolvers():
11a) Rename CTR similar to other variables.
11b) Please correct the header as we deal with multiple conflict-types
in it instead of 1.
Suggestion: Update the subscription's conflict resolvers in
pg_subscription_conflict system catalog for the given conflict types.
12)
SetSubConflictResolver():
12a) I think we do not need 'replaces' during INSERT and thus this is
not needed:
+ memset(replaces, false, sizeof(replaces));
12b)
Shouldn't below be outside of loop:
+ memset(nulls, false, sizeof(nulls));
13)
Shall we rename RemoveSubscriptionConflictBySubid with
RemoveSubscriptionConflictResolvers()? 'BySubid' is not needed as we
have Subscription in the name and we do not have any other variation
of removal.
14)
We shall rename pg_subscription_conflict_sub_index to
pg_subscription_conflict_confsubid_confrtype_index to give more
clarity that it is any index on subid and conftype
And SubscriptionConflictSubIndexId to SubscriptionConflictSubidTypeIndexId
And SUBSCRIPTIONCONFLICTSUBOID to SUBSCRIPTIONCONFLMAP
15)
conflict.h:
+ See ConflictTypeResolverMap in conflcit.c to find out which all
conflcit.c --> conflict.c
16)
subscription.sql:
16a) add one more test case for 'fail' scenario where both conflict
type and resolver are valid but resolver is not for that particular
conflict type.
16b)
--try setting resolvers for few types
Change to below (similar to other comments)
-- ok - valid conflict types and resolvers
16c)
-- ok - valid conflict type and resolver
maybe change to: -- ok - valid conflict types and resolvers
thanks
Shveta
From | Date | Subject | |
---|---|---|---|
Next Message | Michael Paquier | 2024-08-29 05:11:22 | Re: Removing log_cnt from pg_sequence_read_tuple() |
Previous Message | Andy Fan | 2024-08-29 04:38:57 | Re: New function normal_rand_array function to contrib/tablefunc. |