Re: Session WAL activity - Mailing list pgsql-hackers

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