On 2022-10-12 12:50:31 -0700, Andres Freund wrote: > I think this should have at a basic test in src/test/regress/sql/stats.sql. If > I can write one in a few minutes I'll go for that, otherwise will reply > detailing difficulties.
Took a bit longer (+lunch). Attached.
In the attached 0001, the patch to make GetCurrentTransactionStopTimestamp() set xactStopTimestamp, I added a few comment updates and an Assert() to ensure that CurrentTransactionState->state is TRANS_(DEFAULT|COMMIT|ABORT|PREPARE). I am worried that otherwise we might end up with someone ending up using it in a place before the end of the transaction, which'd then end up recording the wrong timestamp in the commit/abort record.
For 0002, the commit adding lastscan, I added catversion/stats version bumps (because I was planning to commit it already...), a commit message, and that minor docs change mentioned earlier.
0003 adds the tests mentioned above. I plan to merge them with 0002, but left them separate for easier review for now.
To be able to compare timestamps for > not just >= we need to make sure that two subsequent timestamps differ. The attached achieves this by sleeping for 100ms between those points - we do that in other places already. I'd started out with 10ms, which I am fairly sure would suffice, but then deciced to copy the existing 100ms sleeps.
I verified tests pass under valgrind, debug_discard_caches and after I make pgstat_report_stat() only flush when force is passed in.
Thanks for that. It looks good to me, bar one comment (repeated 3 times in the sql and expected files):
