Re: Conflict detection for update_deleted in logical replication

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. I suggest adding
comments and probably a test case, if possible, so that if we acquire
commit_ts before setting DELAY_CHKPT_IN_COMMIT flag, then the test
should fail. I haven't checked the feasibility of such a test, so it
may be that it is not feasible or may require some odd injection
points, but even then, it seems better to add comments for this case.

--
With Regards,
Amit Kapila.