On 04.12.2019 8:33, Kyotaro Horiguchi wrote:
It seems to be useful to me. We also might want statistics of other
session IOs. In that case the table name would be
"pg_stat_session/process_activity". We are aleady collecting most
kinds of the IO activity but it loses session information...
Well, actually monitoring disk activity for the particular
backend/session can be easily done using some external tools
(just because now in Postgres session=backend=process). So we can
monitor IO of processes, for example using iotop at Unix
or Performance Monitor at Windows.
Certainly it is more convenient to have such statstic inside Postgres.
But I am not sure if it is really needed.
Concerning WAL activity situation is more obscure: records can be added
to the WAL by one process, but written by another.
This is why it is not possible to use some external tools.
Briefly looking the patch, I have some comments on it.
As mentioned above, if we are intending future exantion of the
session-stats table, the name should be changed.
Backend status is more appropriate than PGPROC. See pgstat.c.
Do you mean pgstat_fetch_stat_beentry?
But why it is better than storing this information directly in PGPROC?
As far as this information ha to be updated from XLogInsertRecord and
it seems to be very performance critical function my intention was to
minimize
overhead of maintaining this statistic. It is hard to imagine something
more efficient than just MyProc->walWriten += write_len;
Also pgstat_fetch_stat_beentry is taken backend id, which is not
reported in pg_stat_activity view and this is why it is more
convenient to pass PID to pg_stat_get_wal_activity. Certainly it is
possible to map PID to backendid, but... why actually do we need to
perform such mapping if simpler solution exists?
Some kind of locking is needed to update the fields on shared segment.
(LWLocks for PGPROC and PGSTAT_BEGIN/END_WRITE_ACTIVITY for
PgBackendStatus)
This information is updated locally only by backend itself.
Certainly update of 64 bit field is not atomic at 32-but architectures.
But it is just statistic. I do not think that it will be fatal if for a
moment
we can see some incorrect value of written WAL bytes (and at most
platforms this
update will be atomic).
As I already wrote above, this information in updated in performance
critical place and this is why
I want to avoid any expensive operations (such as locking or atomic
updates) as much as possible.
Knitpickings:
The patch contains a trace of older trial in
pg_stat_get_activity. Proc OID should be >= 8000 in
patches. src/include/catalog/unused_oids offers some OID for you.
Will fix it.
--
Konstantin Knizhnik
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company
