On Sat, May 24, 2025 at 3:58 PM Dilip Kumar <dilipbal...@gmail.com> wrote: > > 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. >
This sounds reasonable to me. Let us see what others think. -- With Regards, Amit Kapila.
