On Sat, May 24, 2025 at 11:00 AM Amit Kapila <amit.kapil...@gmail.com> wrote: > > On Sat, May 24, 2025 at 10:29 AM Dilip Kumar <dilipbal...@gmail.com> wrote: > > > > On Sat, May 24, 2025 at 10:04 AM Dilip Kumar <dilipbal...@gmail.com> wrote: > > > > > > On Fri, May 23, 2025 at 9:21 PM Xuneng Zhou <xunengz...@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. -- Regards, Dilip Kumar EnterpriseDB: http://www.enterprisedb.com
