From: | Dilip Kumar <dilipbalaut(at)gmail(dot)com> |
---|---|
To: | Xuneng Zhou <xunengzhou(at)gmail(dot)com> |
Cc: | "Zhijie Hou (Fujitsu)" <houzj(dot)fnst(at)fujitsu(dot)com>, shveta malik <shveta(dot)malik(at)gmail(dot)com>, vignesh C <vignesh21(at)gmail(dot)com>, Masahiko Sawada <sawada(dot)mshk(at)gmail(dot)com>, Nisha Moond <nisha(dot)moond412(at)gmail(dot)com>, "Hayato Kuroda (Fujitsu)" <kuroda(dot)hayato(at)fujitsu(dot)com>, Amit Kapila <amit(dot)kapila16(at)gmail(dot)com>, pgsql-hackers(at)lists(dot)postgresql(dot)org |
Subject: | Re: Conflict detection for update_deleted in logical replication |
Date: | 2025-05-24 04:58:50 |
Message-ID: | CAFiTN-s-RibymLrXQsX7tpb50iSb+ZoZ1RusMZoJDNS6JsezOw@mail.gmail.com |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | pgsql-hackers |
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
From | Date | Subject | |
---|---|---|---|
Next Message | Amit Kapila | 2025-05-24 05:07:09 | Re: Replication slot is not able to sync up |
Previous Message | Dilip Kumar | 2025-05-24 04:34:25 | Re: Conflict detection for update_deleted in logical replication |