Feedback-ID: i0fe9450f:Fastmail
Date: Thu, 12 Dec 2024 13:52:03 +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: <Z1pr86DCR_P6iTSV@paquier.xyz>
References: <Zz0m1k03k5LA_c8S@paquier.xyz>
 <Zz3wIuQUJysX5O3d@ip-10-97-1-34.eu-west-3.compute.internal>
 <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>
MIME-Version: 1.0
Content-Type: multipart/signed; micalg=pgp-sha512;
	protocol="application/pgp-signature"; boundary="8FQr48JqJw73SR9H"
Content-Disposition: inline
In-Reply-To: <Z0ScL5W6EeWXShb3@ip-10-97-1-34.eu-west-3.compute.internal>
Archived-At: <https://www.postgresql.org/message-id/Z1pr86DCR_P6iTSV%40paquier.xyz>
Precedence: bulk


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

On Mon, Nov 25, 2024 at 03:47:59PM +0000, Bertrand Drouvot wrote:
> =3D=3D=3D Remarks
>=20
> R1: as compared to v5, v6 removes the per-backend I/O stats reset from=20
> pg_stat_reset_shared(). I think it makes more sense that way, since we are
> adding pg_stat_reset_single_backend_io_counters(). The per-backend I/O st=
ats
> behaves then as the subscription stats as far the reset is concerned.

Makes sense to me to not include that in pg_stat_reset_shared().

> R2: as we can't merge the flush cb anymore, only the patches related to
> the stats_fetch_consistency/'snapshot' are missing in v6 (as compared to =
v5).
> I propose to re-submit them, re-start the discussion once 0001 goes in.

Yeah, thanks.  I should think more about this part, but I'm still kind
of unconvinced.  Let's do things step by step.  For now, I have looked
at v6.

+        view. The function does not return I/O statistics for the checkpoi=
nter,
+        the background writer, the startup process and the autovacuum laun=
cher
+        as they are already visible in the <link linkend=3D"monitoring-pg-=
stat-io-view"> <structname>pg_stat_io</structname></link>
+        view and there is only one of those.

This last sentence seems unnecessary?  The function is named
"backend", and well, all these processes are not backends.

+    /*
+     * Maybe an auxiliary process? That should not be possible, due to
+     * pgstat_tracks_per_backend_bktype() though.
+     */
+    if (proc =3D=3D NULL)
+        proc =3D AuxiliaryPidGetProc(backend_pid);
[...]
+        /*
+         * Maybe an auxiliary process? That should not be possible, due to
+         * pgstat_tracks_per_backend_bktype() though.
+         */
+        if (proc =3D=3D NULL)
+            proc =3D AuxiliaryPidGetProc(pid);

This does not seem right.  Shouldn't we return immediately if
BackendPidGetProc() finds nothing matching with the PID?

+	/* Look for the backend type */
+	for (curr_backend =3D 1; curr_backend <=3D num_backends; curr_backend++)
+	{
+		LocalPgBackendStatus *local_beentry;
+		PgBackendStatus *beentry;
+
+		/* Get the next one in the list */
+		local_beentry =3D pgstat_get_local_beentry_by_index(curr_backend);
+		beentry =3D &local_beentry->backendStatus;
+
+		/* looking for specific PID, ignore all the others */
+		if (beentry->st_procpid !=3D pid)
+			continue;
+
+		bktype =3D beentry->st_backendType;
+		break;
+	}

Sounds to me that the backend type is not strictly required in this
function call if pg_stat_activity can tell already that?

+       (void) pgstat_per_backend_flush_cb(entry_ref, nowait);

I'd recommend to not directly call the callback, use a wrapper
function instead if need be.

 pgstat_count_io_op_time(IOObject io_object, IOContext io_context, IOOp io_=
op,
 						instr_time start_time, uint32 cnt)
 {
+
 	if (track_io_timing)

Noise diff.

 /*
- * Simpler wrapper of pgstat_io_flush_cb()
+ * Simpler wrapper of pgstat_io_flush_cb() and pgstat_per_backend_flush_cb=
().
  */
 void
 pgstat_flush_io(bool nowait)

This is also called in the checkpointer and the bgwriter and the
walwriter via pgstat_report_wal(), which is kind of useless.  Perhaps
just use a different, separate function instead and use that where it
makes sense (per se also the argument of upthread that backend stats
may not be only IO-related..).

Sounds to me that PgStat_BackendPendingIO should be
PgStat_BackendPendingStats?

+{ oid =3D> '8806', descr =3D> 'statistics: per backend IO statistics',
+  proname =3D> 'pg_stat_get_backend_io', prorows =3D> '5', proretset =3D> =
't',

Similarly, s/pg_stat_get_backend_io/pg_stat_get_backend_stats/?

+  descr =3D> 'statistics: reset collected IO statistics for a single backe=
nd',
+  proname =3D> 'pg_stat_reset_single_backend_io_counters', provolatile =3D=
> 'v',=20

And here, pg_stat_reset_backend_stats?

 #define PGSTAT_KIND_SUBSCRIPTION   5   /* per-subscription statistics */
+#define PGSTAT_KIND_PER_BACKEND    6=20

Missing one comment here.

FWIW, I'm so-so about the addition of pg_my_stat_io, knowing that
pg_stat_get_backend_io(NULL/pg_backend_pid()) does the same job.  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();
--
Michael

--8FQr48JqJw73SR9H
Content-Type: application/pgp-signature; name="signature.asc"

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

iQIzBAABCgAdFiEEG72nH6vTowiyblFKnvQgOdbyQH0FAmdaa/MACgkQnvQgOdby
QH3BbRAAjZTcNUWcXprtCVuOc8HZHuwjIp1zHayhDlqtB2buD1Dr5H5HVT/S42TK
rdxOe7AI8yEq1wer/6QH3hWNj7nVnfP61Vw9Sh+Qst6Mgw/9Y0KSg5LiOhC8pPRY
66XlVPF+jv2oLelrIDbR40/54WX6lNw60MdvpCu/CMFAISWaKkyR2t29cRl5Kue8
KX+LGnZPLAWqu6RKEcXCkGl8+WHEdVzi7dsZxLvaMZb/Z34/UKyiUnX1o7tBVU91
8FX6bt+YFB+fu15GT69hJIKnxZLBqwvSOQodKqzzEOlqXC3AV+bYwWbPuYAWx9V9
L79M59zPciyIFpSep57rHwzopdHl2momt6R3u4HXyiaKyhppjx3UXyYL11IaGjHr
td8JZ4l94HNqCIGufCv8xzwLEeFT0SOmU0kwimyXhLYZZK4PNsF5m/qTpIkYDMZB
4E5+n1zznVkqMz+MT/qDEghI+HgB/Qq/NBaSUEo7ss5FCfoJ+TF2lyuvpkVxmXLJ
2t9rMgUh1Qxdk7OZCEV6NyNZrMbI6zkLXl5mfmuRidoyFBVXhYJ8i9M22ggevh2I
l8YOdfT/Ad9yVIDGNmkBXzYxJVyTtTSSlSgreo2S+OpmDvF3dUNn7C09tF/tpQ9y
WCdq4bcCKdLtLseTKKua2oIlNw3/1EFn6mJgOwwhn9VohKJQ96o=
=hruS
-----END PGP SIGNATURE-----

--8FQr48JqJw73SR9H--