Re: Conflict detection for update_deleted in logical replication

On Sat, May 24, 2025 at 11:00 AM Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
>
> On Sat, May 24, 2025 at 10:29 AM Dilip Kumar <dilipbalaut(at)gmail(dot)com> wrote:
> >
> > On Sat, May 24, 2025 at 10:04 AM Dilip Kumar <dilipbalaut(at)gmail(dot)com> wrote:
> > >
> > > On Fri, May 23, 2025 at 9:21 PM Xuneng Zhou <xunengzhou(at)gmail(dot)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.

--
Regards,
Dilip Kumar
EnterpriseDB: http://www.enterprisedb.com