Hi, On 2022-09-15 15:59:52 -0400, Tom Lane wrote: > Andres Freund <and...@anarazel.de> writes: > > pgstat: add/extend tests for resetting various kinds of stats. > > crake just failed [1] with what appears to be a race condition in > a test case added by this commit: > > diff -U3 /home/andrew/bf/root/HEAD/pgsql/src/test/regress/expected/stats.out > /home/andrew/bf/root/HEAD/pgsql.build/src/test/recovery/tmp_check/results/stats.out > --- /home/andrew/bf/root/HEAD/pgsql/src/test/regress/expected/stats.out > 2022-07-18 13:32:25.155847949 -0400 > +++ > /home/andrew/bf/root/HEAD/pgsql.build/src/test/recovery/tmp_check/results/stats.out > 2022-09-15 12:44:10.022975164 -0400 > @@ -563,7 +563,7 @@ > SELECT sessions > :db_stat_sessions FROM pg_stat_database WHERE datname = > (SELECT current_database()); > ?column? > ---------- > - t > + f > (1 row) > > -- Test pg_stat_bgwriter checkpointer-related stats, together with > pg_stat_wal > > 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. > BTW, the header comment for pgstat_report_stat is badly in need of > copy-editing. Hm - you're talking about this sentence? ... 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. Reads awkward. I'm struggling with the english language trying to improve it :(. "Callers can, but are not required to, use the returned time to set up a timer, whose expiration should trigger a call to pgstat_report_stat(true)."? Greetings, Andres Freund
