On 2020/05/07 13:47, Fujii Masao wrote:
On 2020/05/03 1:59, Tomas Vondra wrote:
On Sat, May 02, 2020 at 12:55:00PM +0200, Tomas Vondra wrote:
On Sat, May 02, 2020 at 03:56:07PM +0900, Fujii Masao wrote:
...
Another thing I found is; pgstat_send_slru() should be called also by
other processes than backend? For example, since clog data is flushed
basically by checkpointer, checkpointer seems to need to send slru stats.
Otherwise, pg_stat_slru.flushes would not be updated.
Hmmm, that's a good point. If I understand the issue correctly, the
checkpointer accumulates the stats but never really sends them because
it never calls pgstat_report_stat/pgstat_send_slru. That's only called
from PostgresMain, but not from CheckpointerMain.
Yes.
I think we could simply add pgstat_send_slru() right after the existing
call in CheckpointerMain, right?
Checkpointer sends off activity statistics to the stats collector in
two places, by calling pgstat_send_bgwriter(). What about calling
pgstat_send_slru() just after pgstat_send_bgwriter()?
Yep, that's what I proposed.
In previous email, I mentioned checkpointer just as an example.
So probably we need to investigate what process should send slru stats,
other than checkpointer. I guess that at least autovacuum worker,
logical replication walsender and parallel query worker (maybe this has
been already covered by parallel query some mechanisms. Sorry I've
not checked that) would need to send its slru stats.
Probably. Do you have any other process type in mind?
No. For now what I'm in mind are just checkpointer, autovacuum worker,
logical replication walsender and parallel query worker. Seems logical
replication worker and syncer have sent slru stats via pgstat_report_stat().
I've looked at places calling pgstat_send_* functions, and I found
thsese places:
src/backend/postmaster/bgwriter.c
- AFAIK it merely writes out dirty shared buffers, so likely irrelevant.
src/backend/postmaster/checkpointer.c
- This is what we're already discussing here.
src/backend/postmaster/pgarch.c
- Seems irrelevant.
I'm a bit puzzled why we're not sending any stats from walsender, which
I suppose could do various stuff during logical decoding.
Not sure why, but that seems an oversight...
Also I found another minor issue; SLRUStats has not been initialized to 0
and which could update the counters unexpectedly. Attached patch fixes
this issue.
This is minor issue, but basically it's better to fix that before
v13 beta1 release. So barring any objection, I will commit the patch.
+ values[8] = Int64GetDatum(stat.stat_reset_timestamp);
Also I found another small issue: pg_stat_get_slru() returns the timestamp
when pg_stat_slru was reset by using Int64GetDatum(). This works maybe
because the timestamp is also int64. But TimestampTzGetDatum() should
be used here, instead. Patch attached. Thought?
Regards,
--
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION
diff --git a/src/backend/utils/adt/pgstatfuncs.c
b/src/backend/utils/adt/pgstatfuncs.c
index 446044609e..6b47617328 100644
--- a/src/backend/utils/adt/pgstatfuncs.c
+++ b/src/backend/utils/adt/pgstatfuncs.c
@@ -1757,7 +1757,7 @@ pg_stat_get_slru(PG_FUNCTION_ARGS)
values[5] = Int64GetDatum(stat.blocks_exists);
values[6] = Int64GetDatum(stat.flush);
values[7] = Int64GetDatum(stat.truncate);
- values[8] = Int64GetDatum(stat.stat_reset_timestamp);
+ values[8] = TimestampTzGetDatum(stat.stat_reset_timestamp);
tuplestore_putvalues(tupstore, tupdesc, values, nulls);
}
