Re: Session WAL activity

Hi.

At Wed, 4 Dec 2019 16:40:27 +0300, Konstantin Knizhnik <k.knizhnik@postgrespro.ru> wrote in
>
>
> 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.

Operations that completes on shared buffers cannot be monitored that
way. This is the same with WAL writing.

> 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.

For clarity, I didn't suggest that this patch should include general
session IO statistics. Just the view name looked a bit specific.

> > 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?

No it cannot be used there for performance reasons as you are
saying. I'm not sure it's acceptable, but we can directly access
backend status the same way if we expose MyBEEntry (and update it
through a macro or a inline function).  If we don't need per record
resolution for the value, we can update a process local variable at
WAL-write time then write it to backend status at commit time or at
the same timing as pgstat reporting.

According to my faint memory, PGPROC is thought that it must be kept
as small as possible for the reasons of CPU caches, that is the reason
for PgBackendStatus.

> 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).

At least reader needs to take procarray lock to keep PID-WALwrite
consistency, in order to prevent reading WALwrite values for a wrong
process.

> 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.

I'm afraid that the reason doesn't justify expanding PGPROC..

> > 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.

regards.

--
Kyotaro Horiguchi
NTT Open Source Software Center