Andres Freund <andres@anarazel.de> writes:
> On 2022-09-15 15:59:52 -0400, Tom Lane wrote:
>> It seems likely to me that the n_sessions increment hasn't made it to shared
>> memory because some background process happened to have a lock on the stats
>> entry when we tried. Don't we need a pg_stat_force_next_flush() call before
>> trying to inspect the sessions count?
> Indeed. Pushed a fix. To see if there are other omissions, I made
> pgstat_report_stat() never flush stats unless force is passed in. No other
> failures.
Cool, thanks for dealing with that.
>> BTW, the header comment for pgstat_report_stat is badly in need of
>> copy-editing.
> Hm - you're talking about this sentence?
/*
* Must be called by processes that performs DML: tcop/postgres.c, logical
^^^^^^^^^^^^^^^^^^^^^^^
Plural agreement problem. "all processes that perform" or "each process
that performs" would be OK.
* receiver processes, SPI worker, etc. to flush pending statistics updates to
* shared memory.
*
* Unless called with 'force', pending stats updates are flushed happen once
^^^^^^^^^^^^^^^^^^
One verb too many.
* per PGSTAT_MIN_INTERVAL (1000ms). When not forced, stats flushes do not
* block on lock acquisition, except if stats updates have been pending for
* longer than PGSTAT_MAX_INTERVAL (60000ms).
*
* Whenever pending stats updates remain at the end of pgstat_report_stat() a
* suggested idle timeout is returned. Currently this is always
* PGSTAT_IDLE_INTERVAL (10000ms). Callers can use the returned time to set up
* a timeout after which to call pgstat_report_stat(true), but are not
* required to to do so.
This isn't grammatically bad, but it fails to explain how callers are to
know whether they need to do that. It looks like the convention is to
return 0 if no updates remain, but you have to read the code and guess.
*
* Note that this is called only when not within a transaction, so it is fair
* to use transaction stop time as an approximation of current time.
*/
This should be inside the function, it's not part of the API spec nor of
any concern to callers. Or maybe the API spec should say "This must be
called immediately after finishing a transaction", and then inside the
function say "We assume GetCurrentTransactionStopTimestamp gives a
close-enough approximation of current time."
regards, tom lane