Re: per backend I/O statistics

Hi,

On Fri, Dec 13, 2024 at 11:02:53AM +0900, Michael Paquier wrote:
> On Thu, Dec 12, 2024 at 02:02:38PM +0000, Bertrand Drouvot wrote:
>
> Anyway, isn't it possible that this lookup loop finishes by finding
> nothing depending on concurrent updates of other beentries? It sounds
> to me that this warrants an early exit in the function.

Right, done that way in the attached.

> Perhaps, yes. pgstat_tracks_io_bktype() has always been discarded
> walwriters since pgstat_io.c exists.

Yeap. The comment on top of pgstat_tracks_io_bktype() says that it's not done
"for now". I think that we could update the code as proposed until it's done.

> >> + descr => 'statistics: reset collected IO statistics for a single backend',
> >> + proname => 'pg_stat_reset_single_backend_io_counters', provolatile => 'v',
> >>
> >> And here, pg_stat_reset_backend_stats?
> >
> > Same as above, we could imagine that in the future the backend would get mutiple
> > stats and that one would want to reset only the I/O ones for example.
>
> Disagreed about this part. It is slightly simpler to do a full reset
> of the stats in a single entry. If another subset of stats is added
> to the backend-level entries, we could always introduce a new function
> that has more control over what subset of a single backend entry is
> reset. And I'm pretty sure that we are going to need the function
> that does the full reset anyway.

Yeah, would have added it when a new stats subset would be added. It's fine
by me to have it now though, so done that way.

> As far as I can see, the patch relies entirely on write_to_file to
> prevent any entries to be flushed out.

Yes.

> It means that we leave in the
> dshash entries that may sit idle for as long as the server is up once
> a pgproc slot is used at least once. This scales depending on
> max_connections. It also means that we skip the sanity check about
> dropped entries at shutdown, which may be a good thing to do because
> we don't need to loop through them when writing the stats file.

Agree.

> Hmm.
> Could it be better to be more aggressive with the handling of these
> stats, marking them as dropped when their backend exists and cleanup
> the dshash, without relying on the write flag to make sure that all
> the entries are discarded at shutdown?

Yeah we can do it to be consistent with other stats kind, done.

> The point is that we do
> shutdown in a controlled manner, with all backends exiting before the
> checkpointer writes the stats file after the shutdown checkpoint is
> completed. The patch handles things so as entries are reset when a
> procnum is reused, leaving past stats around until that happens. We
> should perhaps aim for more consistency with the way beentry is
> refreshed and be more proactive with the backend entry drop or reset
> at backend shutdown (pgstat_beshutdown_hook?), so as what is in the
> dshash reflects exactly what's in shared memory for each PGPROC and
> beentry.

That can't be done in pgstat_beshutdown_hook because pgstat_shutdown_hook is called
before and so resets the pgStatLocal.shared_hash during pgstat_detach_shmem().

So, did it in pgstat_shutdown_hook instead.

> Not sure that the "_per_" added in the various references of the patch
> are good to keep, like pgstat_tracks_per_backend_bktype. These could
> be removed, I guess, doing also a PGSTAT_KIND_PER_BACKEND =>
> PGSTAT_KIND_BACKEND?

Yeah makes sense, that's consistent with other kinds: done.

Regards,

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