Feedback-ID: i0fe9450f:Fastmail
Date: Thu, 16 Mar 2023 15:29:31 +0900
From: Michael Paquier <michael@paquier.xyz>
To: "Drouvot, Bertrand" <bertranddrouvot.pg@gmail.com>
Cc: Andres Freund <andres@anarazel.de>,
	Kyotaro Horiguchi <horikyota.ntt@gmail.com>,
	pgsql-hackers@lists.postgresql.org
Subject: Re: Reconcile stats in find_tabstat_entry() and get rid of
 PgStat_BackendFunctionEntry
Message-ID: <ZBK3S5Na71pjgGCX@paquier.xyz>
References: <e4fe4710-09ff-2d74-9680-b63f4b46ebf9@gmail.com>
 <20230214.151102.1070436528757437157.horikyota.ntt@gmail.com>
 <9262b2a9-6785-1f83-bf7f-7fcf90c2c113@gmail.com>
 <20230215.095644.822493184688844040.horikyota.ntt@gmail.com>
 <130095b4-7c6d-b6f1-69d4-94f66231f81d@gmail.com>
 <20230216212121.uvav7xgdh2ysqjy5@awork3.anarazel.de>
 <f08fee25-1dac-b68b-19af-f4b55ceb3802@gmail.com>
MIME-Version: 1.0
Content-Type: multipart/signed; micalg=pgp-sha512;
	protocol="application/pgp-signature"; boundary="ZvIHBg8q//slyq2x"
Content-Disposition: inline
In-Reply-To: <f08fee25-1dac-b68b-19af-f4b55ceb3802@gmail.com>
Archived-At: <https://www.postgresql.org/message-id/ZBK3S5Na71pjgGCX%40paquier.xyz>
Precedence: bulk


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

On Mon, Mar 06, 2023 at 08:33:15AM +0100, Drouvot, Bertrand wrote:
> Thanks for having looked at it!

Looking at that, I have a few comments.

+    tabentry =3D (PgStat_TableStatus *) entry_ref->pending;
+    tablestatus =3D palloc(sizeof(PgStat_TableStatus));
+    *tablestatus =3D *tabentry;
+
[...]
+    for (trans =3D tabentry->trans; trans !=3D NULL; trans =3D trans->uppe=
r)
+    {
+        tablestatus->t_counts.t_tuples_inserted +=3D trans->tuples_inserte=
d;
+        tablestatus->t_counts.t_tuples_updated +=3D trans->tuples_updated;
+        tablestatus->t_counts.t_tuples_deleted +=3D trans->tuples_deleted;
+    }
=20
-    if (entry_ref)
-        return entry_ref->pending;
-    return NULL;
+    return tablestatus;

=46rom what I get with this change, the number of tuples changed by DMLs
have their computations done a bit earlier, meaning that it would make
all the callers of find_tabstat_entry() pay the computation cost.
Still it is not really going to matter, because we will just do the
computation once when looking at any pending changes of
pg_stat_xact_all_tables for each entry.  There are 9 callers of
find_tabstat_entry, with 7 being used for pg_stat_xact_all_tables.
How much do we need to care about the remaining two callers
pg_stat_get_xact_blocks_fetched() and pg_stat_get_xact_blocks_hit()?
Could it be a problem if these two also pay the extra computation cost
if a transaction with many subtransactions (aka )needs to look at their
data?  These two are used nowhere, they have pg_proc entries and they
are undocumented, so it is hard to say the impact of this change on
them..

Second question: once the data from the subtransactions is copied,
would it be cleaner to set trans to NULL after the data copy is done?

It would feel a bit safer to me to document that find_tabstat_entry()
is currently only used for this xact system view..  The extra
computation could lead to surprises, actually, if this routine is used
outside this context?  Perhaps that's OK, but it does not give me a
warm feeling, just to reshape three functions of pgstatfuncs.c with
macros.
--
Michael

--ZvIHBg8q//slyq2x
Content-Type: application/pgp-signature; name="signature.asc"

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

iQIzBAABCgAdFiEEG72nH6vTowiyblFKnvQgOdbyQH0FAmQSt0sACgkQnvQgOdby
QH1cLw/9FHrdLLYEOC44A9UOqPEtzvqMl4256/9QUNB13tyBjvQocmgemYPRhMFY
Oui7tUsea3kwkL8uGUwDsYHDeRZa6WTBLP24XDf3q24Ax0cnlXGH3/kaJLgB0ocZ
JKfroTM8G1MTPLfE0sGbOxVYu0HNCAv8VgEk/uDgGZl6bNEZt+icsR6jadpjcd2v
+ryUE1+L0ZdBCoTRhY2fFR2iDtd4GAtzQB/UDrzbOKN/3kA2fv6R1DDiUo+hj8FN
o8Nazg6umxKOkihAqGxEm8+Rr2wAYkDdnqbgfdWDSw6XJEHjwXdmvGEYQbFPBSVL
YIuEnuQMptyolzdRNeTT/FXQvhBdcqbZmJzZ3FHut0z742/c4rInIJl52q2ot9d8
4bHFz+wjYAZlaTT1SSlPmVuufhCfpYr+Afw7E36KlfJZwAj7on+z9TQNoY/l23pq
NgNrjYd82jpX/ZOzm1At4goQ9QnBuq9CovujNsuu6N+N2h4/LWhLKrOkSLI4CXQm
GRreBTB23JhJR6eSW4KC5G0IYesyC2QGjhBcAuNwF1hlxr6FfZjsRzSjU4Zdpig+
9Cifju4mZv+eLLD3TSXqsJBtCjy1ovDwhCUOMS4tjn61LCFTgvKMykODbr1j/M5f
nq+YuygQOiFY6wY05GuTX3vvEYB0La8ngMmAvzAKflvMhBTJx0I=
=o+fT
-----END PGP SIGNATURE-----

--ZvIHBg8q//slyq2x--