On 2022-09-06 14:15:56 +0100, Dave Page wrote: > Vik and I looked at this a little, and found that we actually don't have > generally have GetCurrentTransactionStopTimestamp() at this point - a > simple 'select * from pg_class' will result in 9 passes of this code, none > of which have xactStopTimestamp != 0.
Huh, pgstat_report_stat() used GetCurrentTransactionStopTimestamp() has used for a long time. Wonder when that was broken. Looks like it's set only when a xid is assigned. We should fix this.
> After discussing it a little, we came to the conclusion that for the stated > use case, xactStartTimestamp is actually accurate enough, provided that we > only ever update it with a newer value. It would only likely be in extreme > edge-cases where the difference between start and end transaction time > would have any bearing on whether or not one might drop a table/index for > lack of use.
I don't at all agree with this. Since we already use GetCurrentTransactionStopTimestamp() in this path we should fix it.
I just spent some time looking at this, and as far as I can see, we only set xactStopTimestamp if the transaction needs to be WAL logged (and in those cases, it is set before the stats callback runs). As you note though, we are already calling GetCurrentTransactionStopTimestamp() in the read-only case anyway, and thus already incurring the cost of gettimeofday().
Here's a v4 patch. This reverts to using GetCurrentTransactionStopTimestamp() for the last_scan times, and will set xactStopTimestamp the first time GetCurrentTransactionStopTimestamp() is called, thus avoiding multiple gettimeofday() calls. SetCurrentTransactionStopTimestamp() is removed, as is use of xactStopTimestamp (except when resetting it to 0).
