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. I suggest adding comments and probably a test case, if possible, so that if we acquire commit_ts before setting DELAY_CHKPT_IN_COMMIT flag, then the test should fail. I haven't checked the feasibility of such a test, so it may be that it is not feasible or may require some odd injection points, but even then, it seems better to add comments for this case. -- With Regards, Amit Kapila.
