RE: Conflict detection and logging in logical replication

> On Monday, July 29, 2024 6:59 PM Dilip Kumar <dilipbalaut@gmail.com>
> wrote:
> >
> > On Mon, Jul 29, 2024 at 11:44 AM Zhijie Hou (Fujitsu)
> > <houzj.fnst@fujitsu.com> wrote:
> > >
> >
> > I was going through v7-0001, and I have some initial comments.
> 
> Thanks for the comments !
> 
> >
> > @@ -536,11 +542,9 @@ ExecCheckIndexConstraints(ResultRelInfo
> > *resultRelInfo, TupleTableSlot *slot,
> >   ExprContext *econtext;
> >   Datum values[INDEX_MAX_KEYS];
> >   bool isnull[INDEX_MAX_KEYS];
> > - ItemPointerData invalidItemPtr;
> >   bool checkedIndex = false;
> >
> >   ItemPointerSetInvalid(conflictTid);
> > - ItemPointerSetInvalid(&invalidItemPtr);
> >
> >   /*
> >   * Get information from the result relation info structure.
> > @@ -629,7 +633,7 @@ ExecCheckIndexConstraints(ResultRelInfo
> > *resultRelInfo, TupleTableSlot *slot,
> >
> >   satisfiesConstraint =
> >   check_exclusion_or_unique_constraint(heapRelation, indexRelation,
> > - indexInfo, &invalidItemPtr,
> > + indexInfo, &slot->tts_tid,
> >   values, isnull, estate, false,
> >   CEOUC_WAIT, true,
> >   conflictTid);
> >
> > What is the purpose of this change?  I mean
> > 'check_exclusion_or_unique_constraint' says that 'tupleid'
> > should be invalidItemPtr if the tuple is not yet inserted and
> > ExecCheckIndexConstraints is called by ExecInsert before inserting the
> tuple.
> > So what is this change?
> 
> Because the function ExecCheckIndexConstraints() is now invoked after
> inserting a tuple (in the patch). So, we need to ignore the newly inserted tuple
> when checking conflict in check_exclusion_or_unique_constraint().
> 
> > Would this change ExecInsert's behavior as well?
> 
> Thanks for pointing it out, I will check and reply.

After checking, I think it may affect ExecInsert's behavior if the slot passed
to ExecCheckIndexConstraints() comes from other tables (e.g. when executing
INSERT INTO SELECT FROM othertbl), because the slot->tts_tid points to a valid
position from another table in this case, which can cause the
check_exclusion_or_unique_constraint to skip a tuple unexpectedly).

I thought about two ideas to fix this: One is to reset the slot->tts_tid before
calling ExecCheckIndexConstraints() in ExecInsert(), but I feel a bit
uncomfortable to this since it is touching existing logic. So, another idea is to
just add a new parameter 'tupletid' in ExecCheckIndexConstraints(), then pass
tupletid=InvalidOffsetNumber in when invoke the function in ExecInsert() and
pass a valid tupletid in the new code paths in the patch.  The new
'tupletid' will be passed to check_exclusion_or_unique_constraint to
skip the target tuple. I feel the second one maybe better.

What do you think ?

Best Regards,
Hou zj