Re: Conflict detection for update_deleted in logical replication

On Sat, May 24, 2025 at 3:58 PM Dilip Kumar <dilipbalaut@gmail.com> 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.
>

This sounds reasonable to me. Let us see what others think.

-- 
With Regards,
Amit Kapila.