Re: Conflict detection for update_deleted in logical replication

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.
>
> [1]
> @@ -1537,7 +1541,7 @@ RecordTransactionCommit(void)
> */
> if (markXidCommitted)
> {
> - MyProc->delayChkptFlags &= ~DELAY_CHKPT_START;
> + MyProc->delayChkptFlags &= ~DELAY_CHKPT_IN_COMMIT;
> END_CRIT_SECTION();
> }
>
> [2]
> /*
> * Insert the commit XLOG record.
> */
> XactLogCommitRecord(GetCurrentTransactionStopTimestamp(),
> nchildren, children, nrels, rels,
> ndroppedstats, droppedstats,
> nmsgs, invalMessages,
> RelcacheInitFileInval,
> MyXactFlags,
> InvalidTransactionId, NULL /*
> plain commit */ );

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.

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