Re: per backend I/O statistics

Hi,

On Mon, Dec 16, 2024 at 05:07:52PM +0900, Michael Paquier wrote:
> On Fri, Dec 13, 2024 at 09:20:13AM +0000, Bertrand Drouvot wrote:
> 
> Not feeling so sure about the value brought by the backend_type
> returned in pg_stat_get_backend_io(), but well..

It's not necessary per say, but it ensures that pg_stat_get_backend_io() does not
return rows full of "invalid" combinations.

For example:

With the filtering in place, on a "client backend" we would get:

postgres=# select * from pg_stat_get_backend_io(pg_backend_pid());
  backend_type  |    object     |  context  | reads | read_time | writes | write_time | writebacks | writeback_time | extends | extend_time | op_bytes | hits | evictions | reuses | fsyncs | fsync_time | stats_reset
----------------+---------------+-----------+-------+-----------+--------+------------+------------+----------------+---------+-------------+----------+------+-----------+--------+--------+------------+-------------
 client backend | relation      | bulkread  |     0 |         0 |      0 |          0 |          0 |              0 |         |             |     8192 |    0 |         0 |      0 |        |            |
 client backend | relation      | bulkwrite |     0 |         0 |      0 |          0 |          0 |              0 |       0 |           0 |     8192 |    0 |         0 |      0 |        |            |
 client backend | relation      | normal    |    86 |         0 |      0 |          0 |          0 |              0 |       0 |           0 |     8192 | 1604 |         0 |        |      0 |          0 |
 client backend | relation      | vacuum    |     0 |         0 |      0 |          0 |          0 |              0 |       0 |           0 |     8192 |    0 |         0 |      0 |        |            |
 client backend | temp relation | normal    |     0 |         0 |      0 |          0 |            |                |       0 |           0 |     8192 |    0 |         0 |        |        |            |
(5 rows)

and for example for the walsender:

postgres=# select * from pg_stat_get_backend_io(3982910);
 backend_type |    object     |  context  | reads | read_time | writes | write_time | writebacks | writeback_time | extends | extend_time | op_bytes | hits | evictions | reuses | fsyncs | fsync_time | stats_reset
--------------+---------------+-----------+-------+-----------+--------+------------+------------+----------------+---------+-------------+----------+------+-----------+--------+--------+------------+-------------
 walsender    | relation      | bulkread  |     0 |         0 |      0 |          0 |          0 |              0 |         |             |     8192 |    0 |         0 |      0 |        |            |
 walsender    | relation      | bulkwrite |     0 |         0 |      0 |          0 |          0 |              0 |       0 |           0 |     8192 |    0 |         0 |      0 |        |            |
 walsender    | relation      | normal    |     0 |         0 |      0 |          0 |          0 |              0 |       0 |           0 |     8192 |   54 |         0 |        |      0 |          0 |
 walsender    | relation      | vacuum    |     0 |         0 |      0 |          0 |          0 |              0 |       0 |           0 |     8192 |    0 |         0 |      0 |        |            |
 walsender    | temp relation | normal    |     0 |         0 |      0 |          0 |            |                |       0 |           0 |     8192 |    0 |         0 |        |        |            |
(5 rows)

While, without the filtering we would get:

postgres=# select * from pg_stat_get_backend_io(pg_backend_pid());
  backend_type  |    object     |  context  | reads | read_time | writes | write_time | writebacks | writeback_time | extends | extend_time | op_bytes | hits | evictions | reuses | fsyncs | fsync_time | stats_reset
----------------+---------------+-----------+-------+-----------+--------+------------+------------+----------------+---------+-------------+----------+------+-----------+--------+--------+------------+-------------
 client backend | relation      | bulkread  |     0 |         0 |      0 |          0 |          0 |              0 |         |             |     8192 |    0 |         0 |      0 |        |            |
 client backend | relation      | bulkwrite |     0 |         0 |      0 |          0 |          0 |              0 |       0 |           0 |     8192 |    0 |         0 |      0 |        |            |
 client backend | relation      | normal    |     4 |         0 |      0 |          0 |          0 |              0 |       0 |           0 |     8192 |   50 |         0 |        |      0 |          0 |
 client backend | relation      | vacuum    |     0 |         0 |      0 |          0 |          0 |              0 |       0 |           0 |     8192 |    0 |         0 |      0 |        |            |
 client backend | temp relation | bulkread  |       |           |        |            |            |                |         |             |     8192 |      |           |        |        |            |
 client backend | temp relation | bulkwrite |       |           |        |            |            |                |         |             |     8192 |      |           |        |        |            |
 client backend | temp relation | normal    |     0 |         0 |      0 |          0 |            |                |       0 |           0 |     8192 |    0 |         0 |        |        |            |
 client backend | temp relation | vacuum    |       |           |        |            |            |                |         |             |     8192 |      |           |        |        |            |
(8 rows)

and for a walsender:

postgres=# select * from pg_stat_get_backend_io(3981588);
 backend_type |    object     |  context  | reads | read_time | writes | write_time | writebacks | writeback_time | extends | extend_time | op_bytes | hits | evictions | reuses | fsyncs | fsync_time | stats_reset
--------------+---------------+-----------+-------+-----------+--------+------------+------------+----------------+---------+-------------+----------+------+-----------+--------+--------+------------+-------------
 walsender    | relation      | bulkread  |     0 |         0 |      0 |          0 |          0 |              0 |         |             |     8192 |    0 |         0 |      0 |        |            |
 walsender    | relation      | bulkwrite |     0 |         0 |      0 |          0 |          0 |              0 |       0 |           0 |     8192 |    0 |         0 |      0 |        |            |
 walsender    | relation      | normal    |     6 |         0 |      0 |          0 |          0 |              0 |       0 |           0 |     8192 |   48 |         0 |        |      0 |          0 |
 walsender    | relation      | vacuum    |     0 |         0 |      0 |          0 |          0 |              0 |       0 |           0 |     8192 |    0 |         0 |      0 |        |            |
 walsender    | temp relation | bulkread  |       |           |        |            |            |                |         |             |     8192 |      |           |        |        |            |
 walsender    | temp relation | bulkwrite |       |           |        |            |            |                |         |             |     8192 |      |           |        |        |            |
 walsender    | temp relation | normal    |     0 |         0 |      0 |          0 |            |                |       0 |           0 |     8192 |    0 |         0 |        |        |            |
 walsender    | temp relation | vacuum    |       |           |        |            |            |                |         |             |     8192 |      |           |        |        |            |
(8 rows)

It would not be possible to remove those "extra 3 rows" with a join on the
backend type on pg_stat_activity because only the C code knows about what the
compatibility is (though pgstat_tracks_io_object).

> +	/* drop the backend stats entry */
> +	pgstat_drop_entry(PGSTAT_KIND_BACKEND, InvalidOid, MyProcNumber);
> 
> Oh, I've missed something.  Shouldn't pgstat_request_entry_refs_gc()
> be called when this returns false?

I'm not sure that would be possible for a PGSTAT_KIND_BACKEND kind to return
false here but I agree that's better to call pgstat_request_entry_refs_gc()
if that's the case. Done in v9 attached.

> The creation of the dshash entry is a bit too early, I think..  How
> about delaying it more so as we don't create entries that could be
> useless if we fail the last steps of authentication?  One spot would
> be to delay the creation of the new entry at the end of
> pgstat_bestart(), where we know that we are done with authentication
> and that the backend is ready to report back to the client connected
> to it.  It is true that some subsystems could produce stats as of the 
> early transactions they generate, which is why pgstat_initialize() is
> done that early in BaseInit(), but that's not really relevant for this
> case?

I think that makes sense to move the stats entry creation in pgstat_bestart(),
done that way in v9.

> I'm still feeling a bit uneasy about the drop done in
> pgstat_shutdown_hook(); it would be nice to make sure that this
> happens in a path that would run just after we're done with the
> creation of the entry to limit windows where we have an entry but no
> way to drop it, or vice-versa,

I don't believe that's possible as MyProcNumber can't be "reused" (and is
guaranteed to remain valid) until ProcKill() is done (which happens after the
pgstat_shutdown_hook()).

> Perhaps there's an argument for an entirely separate
> callback that would run before pgstat is plugged off, like a new
> before_shmem_exit() callback registered after the entry is created?

As the ProcKill() is run in shmem_exit() (and so after before_shmem_exit()),
I think that the way we currently drop the backend stats entry is fine (unless
I miss something about your concern).

Regards,

-- 
Bertrand Drouvot
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com