On Friday, August 16, 2024 5:25 PM shveta malik <shveta.ma...@gmail.com> wrote: > > On Fri, Aug 16, 2024 at 12:19 PM Amit Kapila <amit.kapil...@gmail.com> > wrote: > > > > On Fri, Aug 16, 2024 at 11:48 AM shveta malik <shveta.ma...@gmail.com> > wrote: > > > > > > On Fri, Aug 16, 2024 at 10:46 AM shveta malik <shveta.ma...@gmail.com> > wrote: > > > > > > > > 3) > > > > For update_exists(), we dump: > > > > Key (a, b)=(2, 1) > > > > > > > > For delete_missing, update_missing, update_differ, we dump: > > > > Replica identity (a, b)=(2, 1). > > > > > > > > For update_exists as well, shouldn't we dump 'Replica identity'? > > > > Only for insert case, it should be referred as 'Key'. > > > > > > > > > > On rethinking, is it because for update_exists case 'Key' dumped is > > > not the one used to search the row to be updated? Instead it is the > > > one used to search the conflicting row. Unlike update_differ, the > > > row to be updated and the row currently conflicting will be > > > different for update_exists case. I earlier thought that 'KEY' and > > > 'Existing local tuple' dumped always belong to the row currently > > > being updated/deleted/inserted. But for 'update_eixsts', that is not > > > the case. We are dumping 'Existing local tuple' and 'Key' for the > > > row which is conflicting and not the one being updated. Example: > > > > > > ERROR: conflict detected on relation "public.tab_1": > > > conflict=update_exists Key (a, b)=(2, 1); existing local tuple (2, 1); > > > remote > tuple (2, 1). > > > > > > Operations performed were: > > > Pub: insert into tab values (1,1); > > > Sub: insert into tab values (2,1); > > > Pub: update tab set a=2 where a=1; > > > > > > Here Key and local tuple are both 2,1 instead of 1,1. While replica > > > identity value (used to search original row) will be 1,1 only. > > > > > > It may be slightly confusing or say tricky to understand when > > > compared to other conflicts' LOGs. But not sure what better we can do > here. > > > > > > > The update_exists behaves more like insert_exists as we detect that > > only while inserting into index. It is also not clear to me if we can > > do better than to clarify this in docs. > > > > Instead of 'existing local tuple', will it be slightly better to have > 'conflicting local > tuple'?
I am slightly not sure about adding one more variety to describe the "existing local tuple". I think we’d better use a consistent word. But if others feel otherwise, I can change it in next version. > > Few trivial comments: > > 1) > errdetail_apply_conflict() header says: > > * 2. Display of conflicting key, existing local tuple, remote new tuple, and > * replica identity columns, if any. > > We may mention that existing *conflicting* local tuple. Like above, I think that would duplicate the "existing local tuple" word. > > Looking at build_tuple_value_details(), the cases where we display 'KEY 'and > the ones where we display 'replica identity' are mutually exclusives (we have > ASSERTs like that). Shall we add this info in > header that either Key or 'replica identity' is displayed. Or if we > don't want to make it mutually exclusive then update_exists is one such casw > where we can have both Key and 'Replica Identity cols'. I think it’s fine to display replica identity for update_exists, so added. > > > 2) > BuildIndexValueDescription() header comment says: > > * This is currently used > * for building unique-constraint, exclusion-constraint and logical > replication > * tuple missing conflict error messages > > Is it being used only for 'tuple missing conflict' flow? I thought, it will > be hit for > other flows as well. Removed the "tuple missing". Attach the V16 patch which addressed the comments we agreed on. I will add a doc patch to explain the log format after the 0001 is RFC. Best Regards, Hou zj
v16-0001-Detect-and-log-conflicts-in-logical-replication.patch
Description: v16-0001-Detect-and-log-conflicts-in-logical-replication.patch
