Hi On Wed, 12 Oct 2022 at 23:52, Andres Freund <and...@anarazel.de> wrote:
> Hi, > > 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): fetch timestamps from before the next test "from " should be removed. -- Dave Page Blog: https://pgsnake.blogspot.com Twitter: @pgsnake EDB: https://www.enterprisedb.com
