RE: Conflict detection for update_deleted in logical replication

On Sat, May 24, 2025 at 6:28 PM Dilip Kumar wrote:

> 
> On Sat, May 24, 2025 at 11:00 AM Amit Kapila <amit.kapila16@gmail.com>
> wrote:
> >
> > On Sat, May 24, 2025 at 10:29 AM Dilip Kumar <dilipbalaut@gmail.com>
> wrote:
> > >
> > > On Sat, May 24, 2025 at 10:04 AM Dilip Kumar <dilipbalaut@gmail.com>
> wrote:
> > > >
> > > > On Fri, May 23, 2025 at 9:21 PM Xuneng Zhou
> <xunengzhou@gmail.com> wrote:
> > > > >
> > > > Looking at v31-0001 again, most of it looks fine except this logic
> > > > of getting the commit_ts after marking the transaction in commit.
> > > > I see in RecordTransactionCommit(), we are setting this flag
> > > > (DELAY_CHKPT_IN_COMMIT) to put the transaction in commit state[1],
> > > > and after that we insert the commit log[2], but I noticed that
> > > > there we call GetCurrentTransactionStopTimestamp() for acquiring
> > > > the commit-ts and IIUC we want to ensure that commit-ts timestamp
> > > > should be after we set the transaction in commit with
> > > > (DELAY_CHKPT_IN_COMMIT), but question is, is it guaranteed that
> > > > the place we are calling
> > > > GetCurrentTransactionStopTimestamp() will always give us the
> > > > current timestamp? Because if you see this function, it may return
> > > > 'xactStopTimestamp' as well if that is already set.  I am still
> > > > digging a bit more. Is there a possibility that
> > > > 'xactStopTimestamp' is already set during some interrupt handling
> > > > when
> > > > GetCurrentTransactionStopTimestamp() is already called by
> > > > pgstat_report_stat(), or is it guaranteed that during
> > > > RecordTransactionCommit we will call this first time?
> > > >
> > > > If we have already ensured this then I think adding a comment to
> > > > explain the same will be really useful.
> > > >
> > ...
> > >
> > > IMHO, this should not be an issue as the only case where
> > > 'xactStopTimestamp' is set for the current process is from
> > > ProcessInterrupts->pgstat_report_stat->
> > > GetCurrentTransactionStopTimestamp, and this call sequence is only
> > > possible when transaction blockState is TBLOCK_DEFAULT.  And that is
> > > only set after RecordTransactionCommit() is called, so logically,
> > > RecordTransactionCommit() should always be the first one to set the
> > > 'xactStopTimestamp'.  But I still think this is a candidate for
> > > comments, or even better,r if somehow it can be ensured by some
> > > assertion, maybe by passing a parameter in
> > > GetCurrentTransactionStopTimestamp() that if this is called from
> > > RecordTransactionCommit() then 'xactStopTimestamp' must not already
> > > be set.
> > >
> >
> > We can add an assertion as you are suggesting, but I feel that adding
> > a parameter for this purpose looks slightly odd.
> 
> 
> Yeah, that's true. Another option is to add an assert as
> Assert(xactStopTimestamp == 0) right before calling
> XactLogCommitRecord()?  With that, we don't need to pass an extra
> parameter, and since we are in a critical section, this process can not be
> interrupted, so it's fine even if we have ensured that 'xactStopTimestamp' is 0
> before calling the API, as this can not be changed.  And we can add a
> comment atop this assertion.

Thanks for the suggestion !

I think adding an Assert as suggested is OK. I am not adding more comments
atop of the assert because we already have comments in a very close place that
explains the importance of setting the flag first.

Best Regards,
Hou zj