From: | Ajin Cherian <itsajin(at)gmail(dot)com> |
---|---|
To: | shveta malik <shveta(dot)malik(at)gmail(dot)com> |
Cc: | Nisha Moond <nisha(dot)moond412(at)gmail(dot)com>, Amit Kapila <amit(dot)kapila16(at)gmail(dot)com>, Ashutosh Bapat <ashutosh(dot)bapat(dot)oss(at)gmail(dot)com>, Dilip Kumar <dilipbalaut(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> |
Subject: | Re: Conflict Detection and Resolution |
Date: | 2024-07-31 11:24:06 |
Message-ID: | CAFPTHDZvACoqRJzrdd8BtexuTD8y3xaSo__y5Ep4u6CnNBspkg@mail.gmail.com |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | pgsql-hackers |
On Tue, Jul 30, 2024 at 2:19 PM shveta malik <shveta(dot)malik(at)gmail(dot)com> wrote:
> On Fri, Jul 26, 2024 at 9:50 AM Ajin Cherian <itsajin(at)gmail(dot)com> wrote:
> >>
> > Please find v7 patch-set, the changes are:
> >
>
> Thanks Ajin for working on this. Please find few comments:
>
> 1)
> parse_subscription_conflict_resolvers():
> Here we loop in this function to find the given conflict type in the
> supported list and error out if conflict-type is not valid. Also we
> call validate_conflict_type_and_resolver() which again validates
> conflict-type. I would recommend to loop 'stmtresolvers' in parse
> function and then read each type and resolver and pass that to
> validate_conflict_type_and_resolver(). Avoid double validation.
>
>
I have modified this as per comment.
> 2)
> SetSubConflictResolver():
> It works well, but it does not look apt that the 'resolvers' passed to
> this function by the caller is an array and this function knows the
> array range and traverse from CT_MIN to CT_MAX assuming this array
> maps directly to ConflictType. I think it would be better to have it
> passed as a list and then SetSubConflictResolver() traverse the list
> without knowing the range of it. Similar to what we do in
> alter-sub-flow in and around UpdateSubConflictResolvers().
>
>
I have kept the array as it requires that all conflict resolvers be set, if
not provided by the user then default needs to be used. However, I have
modified SetSubConflictResolver such that it takes in the size of the array
and does not assume it.
3)
> When I execute 'alter subscription ..(detect_conflict=on)' for a
> subscription which *already* has detect_conflict as ON, it tries to
> reset resolvers to default and ends up in error. It should actually be
> no-op in this particular situation and should not reset resolvers to
> default.
>
> postgres=# alter subscription sub1 set (detect_conflict=on);
> WARNING: Using default conflict resolvers
> ERROR: duplicate key value violates unique constraint
> "pg_subscription_conflict_sub_index"
>
>
fixed
> 4)
> Do we need SUBSCRIPTIONCONFLICTOID cache? We are not using it
> anywhere. Shall we remove this and the corresponding index?
>
>
We are using the index but not the cache, so removing the cache.
> 5)
> RemoveSubscriptionConflictBySubid().
> --We can remove extra blank line before table_open.
> --We can get rid of curly braces around CatalogTupleDelete() as it is
> a single line in loop.
>
>
fixed.
On Tue, Jul 30, 2024 at 8:34 PM Dilip Kumar <dilipbalaut(at)gmail(dot)com> wrote:
> On Fri, Jul 26, 2024 at 9:50 AM Ajin Cherian <itsajin(at)gmail(dot)com> wrote:
>
> Comment in 0002,
>
> 1) I do not see any test case that set a proper conflict type and
> conflict resolver, all tests either give incorrect conflict
> type/conflict resolver or the conflict resolver is ignored
>
fixed.
I've also fixed a cfbot error due to patch 0001. Rebase of table resolver
patch is still pending, will try and target that in the next patch-set.
regards,
Ajin Cherian
Fujitsu Australia
Attachment | Content-Type | Size |
---|---|---|
v8-0004-Manage-Clock-skew-and-implement-last_update_wins.patch | application/octet-stream | 51.3 KB |
v8-0002-Add-CONFLICT-RESOLVERS-into-the-syntax-for-CREATE.patch | application/octet-stream | 37.6 KB |
v8-0001-Detect-and-log-conflicts-in-logical-replication.patch | application/octet-stream | 98.6 KB |
v8-0003-Conflict-resolvers-for-insert-update-and-delete.patch | application/octet-stream | 55.8 KB |
From | Date | Subject | |
---|---|---|---|
Next Message | Ilia Evdokimov | 2024-07-31 13:02:37 | Re: refactor the CopyOneRowTo |
Previous Message | Zhijie Hou (Fujitsu) | 2024-07-31 11:21:23 | RE: Remove duplicate table scan in logical apply worker and code refactoring |