Feedback-ID: i0fe9450f:Fastmail
Date: Fri, 13 Dec 2024 11:02:53 +0900
From: Michael Paquier <michael@paquier.xyz>
To: Bertrand Drouvot <bertranddrouvot.pg@gmail.com>
Cc: Nazir Bilal Yavuz <byavuz81@gmail.com>,
	Alvaro Herrera <alvherre@alvh.no-ip.org>,
	Kyotaro Horiguchi <horikyota.ntt@gmail.com>,
	pgsql-hackers@lists.postgresql.org
Subject: Re: per backend I/O statistics
Message-ID: <Z1uVzXFE9WhSicOx@paquier.xyz>
References: <Zz5gMM9w59guO9S_@paquier.xyz>
 <Zz9sno+JJbWqdXhQ@ip-10-97-1-34.eu-west-3.compute.internal>
 <Zz_gHQ2g8MUe8oJ1@paquier.xyz>
 <Z0A3pmORCbhRe0Ug@ip-10-97-1-34.eu-west-3.compute.internal>
 <Z0PNpAjhcS7S-1LI@paquier.xyz>
 <Z0QjeIkwC0HNI16K@ip-10-97-1-34.eu-west-3.compute.internal>
 <Z0Qk3nZcaVpbtCKU@paquier.xyz>
 <Z0ScL5W6EeWXShb3@ip-10-97-1-34.eu-west-3.compute.internal>
 <Z1pr86DCR_P6iTSV@paquier.xyz>
 <Z1rs/j5JMdTbUIJJ@ip-10-97-1-34.eu-west-3.compute.internal>
MIME-Version: 1.0
Content-Type: multipart/signed; micalg=pgp-sha512;
	protocol="application/pgp-signature"; boundary="51tFf754fswbFjqZ"
Content-Disposition: inline
In-Reply-To: <Z1rs/j5JMdTbUIJJ@ip-10-97-1-34.eu-west-3.compute.internal>
Archived-At: <https://www.postgresql.org/message-id/Z1uVzXFE9WhSicOx%40paquier.xyz>
Precedence: bulk


--51tFf754fswbFjqZ
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
Content-Transfer-Encoding: quoted-printable

On Thu, Dec 12, 2024 at 02:02:38PM +0000, Bertrand Drouvot wrote:
> On Thu, Dec 12, 2024 at 01:52:03PM +0900, Michael Paquier wrote:
> Yeah, but it's needed in pg_stat_get_backend_io() for the stats filtering=
 (to
> display only those linked to this backend type), later in the function he=
re:
>=20
> +    /*
> +     * Some combinations of BackendType, IOObject, and IOContext are
> +     * not valid for any type of IOOp. In such cases, omit the entire
> +     * row from the view.
> +     */
> +    if (!pgstat_tracks_io_object(bktype, io_obj, io_context))
> +          continue;
>=20
> OTOH, now that we get rid of the AuxiliaryPidGetProc() call in
> pg_stat_reset_single_backend_io_counters() we can also remove the need to
> look for the backend type in this function.

Ah, you need that for the pgstat_tracks_io_object() filtering.  I'm
wondering if we should think about making that cheaper at some point.
As a O(N^2) when coupling a call of pg_stat_get_backend_io() with a
scan of pg_stat_activity, perhaps it does not matter much anyway..

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.

> BTW, not related to this particular patch but I realized that pgstat_flus=
h_io()
> is called for the walwriter. Indeed, it's coming from the pgstat_report_w=
al()
> call in WalWriterMain(). That can not report any I/O stats activity (as t=
he
> walwriter is not part of the I/O stats tracking, see pgstat_tracks_io_bkt=
ype()).
>
> So it looks like, we could move pgstat_flush_io() outside of pgstat_repor=
t_wal()
> and add the pgstat_flush_io() calls only where they need to be made (and =
so, not
> in WalWriterMain()).

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

> But I'm not sure that's worth it until we don't have a need to add more
> pending stats per-backend, thoughts?

Okay with your argument here.

>> +{ oid =3D> '8806', descr =3D> 'statistics: per backend IO statistics',
>> +  proname =3D> 'pg_stat_get_backend_io', prorows =3D> '5', proretset =
=3D> 't',
>>=20
>> Similarly, s/pg_stat_get_backend_io/pg_stat_get_backend_stats/?
>=20
> I think that's fine to keep pg_stat_get_backend_io(). If we add more per-=
backend
> stats in the future then we could add "dedicated" get functions too and a=
 generic
> one retrieving all of them.

Okay about this layer, discarding my remark.  You have a point about
that: other stats associated to a single backend may be OK if not
returned as a SRF, and they'll most likely return a different set of
attributes.

>> +  descr =3D> 'statistics: reset collected IO statistics for a single ba=
ckend',
>> +  proname =3D> 'pg_stat_reset_single_backend_io_counters', provolatile =
=3D> 'v',=20
>>=20
>> And here, pg_stat_reset_backend_stats?
>=20
> 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.

>> I
>> would just add a note in the docs with a query showing how to use it
>> with pg_stat_activity.  An example with LATERAL, doing the same work:
>> select a.pid, s.* from pg_stat_activity as a,
>>    lateral pg_stat_get_backend_io(a.pid) as s
>>   where pid =3D pg_backend_pid();
>=20
> I'm not sure it's worth it. I think that's clear that to get our own stats
> then we need to provide our own backend pid. For example pg_stat_get_acti=
vity()
> does not provide such an example using pg_stat_activity or using somethin=
g like=20
> pg_stat_get_activity(pg_backend_pid()).

Okay.

As far as I can see, the patch relies entirely on write_to_file to
prevent any entries to be flushed out.  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.  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?  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.

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 =3D>
PGSTAT_KIND_BACKEND?
--
Michael

--51tFf754fswbFjqZ
Content-Type: application/pgp-signature; name="signature.asc"

-----BEGIN PGP SIGNATURE-----

iQIzBAABCgAdFiEEG72nH6vTowiyblFKnvQgOdbyQH0FAmdblc0ACgkQnvQgOdby
QH3fBxAAjhMQROEoWULNSgRJ9cl9hh25jMZAjwcYyqpI3agFgSgEhvfbNftPJwi2
Y6UMsHJl/Yja6LXIk8YEH4CEEYIOhGawQH5J+18vE0N19r4ipEr25CvlirdjwpuC
AgLT301EGp9Vi+GZZ9HObUbuNLhPaNc/5/NHNLUFxcgTHOWqiGDm0f5AxvzrNvpS
iqzMEUYskrf3lTYYmmqY6Z2CCUxGWB4cba2hZ4DWfnwiiPe/GNxUqqGhbcY4ue7H
M2TZ95F2zbHhV1Z8o2ZUgxCBxCWg5HdiwvXSRj3zZX17Z/JG5Xh50RfRN9BF+Cdd
aDN1Y1Dgm4O9CrzmejIQaoq3RGbOaJfMwZufxKVcDdnxa0Y1q6HJX57lbK225N02
46hyhxPpVDdV30xiAzNZ8pjc/8jLEcP8e+yQuL9dQdzQ2IRWX8yOGuAYWGgbclct
7t1xqSXTrotmqev8QvNIPUV9xWqmqX7rFs9ZvuUSHmCVfMSy0GozsqUriFre8UTO
FnmfoVSOcTk6vBTv+kUOk5VFH1IKDHYQ3er+U7nr+zGC8voBRBDxjKm/qWw3wQbt
hRXjyk9CHK8MbJ5AFw98tX95RyTkjHmA4m8lfot0eyuYCQ90nCfe+lSj3ZsZodA9
laKNqdkz4kUJ5aIQyhC5vwMNfO4QNmMwscQK4mJ2F8h+pY1I0c8=
=3nzr
-----END PGP SIGNATURE-----

--51tFf754fswbFjqZ--