On Sat, May 24, 2025 at 6:28 PM Dilip Kumar 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.
Thanks for the suggestion ! I think adding an Assert as suggested is OK. I am not adding more comments atop of the assert because we already have comments in a very close place that explains the importance of setting the flag first. Best Regards, Hou zj
