Feedback-ID: i0fe9450f:Fastmail
Date: Thu, 16 Mar 2023 20:46:15 +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: <ZBMBh5ve3tyqH5/m@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>
 <ZBK3S5Na71pjgGCX@paquier.xyz>
 <621ed53c-4d9a-8f77-ab8c-20235f2d95a1@gmail.com>
MIME-Version: 1.0
Content-Type: multipart/signed; micalg=pgp-sha512;
	protocol="application/pgp-signature"; boundary="wHQSRdwqq9BFgo/d"
Content-Disposition: inline
In-Reply-To: <621ed53c-4d9a-8f77-ab8c-20235f2d95a1@gmail.com>
Archived-At: <https://www.postgresql.org/message-id/ZBMBh5ve3tyqH5/m%40paquier.xyz>
Precedence: bulk


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

On Thu, Mar 16, 2023 at 11:32:56AM +0100, Drouvot, Bertrand wrote:
> On 3/16/23 7:29 AM, Michael Paquier wrote:
>>  From what I get with this change, the number of tuples changed by DMLs
>> have their computations done a bit earlier,
>=20
> Thanks for looking at it!
>=20
> Right, but note this is in a dedicated new tablestatus (created
> within find_tabstat_entry()).

Sure, however it copies the pointer of the PgStat_TableXactStatus from
tabentry, isn't it?  This means that it keeps a reference of the chain
of subtransactions.  It does not matter for the functions but it could
for out-of-core callers of find_tabstat_entry(), no?  Perhaps you are
right and that's not worth worrying, still I don't feel particularly
confident that this is the best approach we can take.

>> 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()?
>=20
> Regarding pg_stat_get_xact_blocks_fetched() and pg_stat_get_xact_blocks_h=
it()
> the callers (if any) are outside of the core PG (as from what I can
> see they are not used at all).
>=20
> I don't think we should pay any particular attention to those 2 ones
> as anyway nothing prevent the 7 others to be called outside of the
> pg_stat_xact_all_tables view.

I am not quite sure, TBH.  Did you look at the difference with a long
chain of subtrans, like savepoints?  The ODBC driver "loves" producing
a lot of savepoints, for example.

>> 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.
>=20
> That's a fair point. On the other hand those 9 functions (which can
> all be used outside of the pg_stat_xact_all_tables view) are not
> documented, so I'm not sure this is that much of a concern (and if
> we think it is we still gave the option to add an extra flag to
> indicate whether or not the extra computation is needed.)

That's not quite exact, I think.  The first 7 functions are used in a
system catalog that is documented.  Still we have a problem here.  I
can actually see a few projects relying on these two functions while
looking a bit around, so they are used.  And the issue comes from
ddfc2d9, that has removed these functions from the documentation
ignoring that they are used in no system catalogs.  I think that we
should fix that and re-add the two missing functions with a proper
description in the docs, at least?  There is no trace of them.
Perhaps the ones exposted through pg_stat_xact_all_tables are fine if
not listed.
--
Michael

--wHQSRdwqq9BFgo/d
Content-Type: application/pgp-signature; name="signature.asc"

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

iQIzBAABCgAdFiEEG72nH6vTowiyblFKnvQgOdbyQH0FAmQTAYcACgkQnvQgOdby
QH26Gg/+IOENnupcHW7NfqdHyvASdSwzLhjYmcEmAsCEV6g4NxozxZm4Qqeet2GK
P4IzXs/yBJQxBHVruGX35YOL8hL/olo2dPxPyHN1ssULyQpmxn/RM/EbRzkaMzMu
Tm6LcRXLaJJG5YQVkkZWfa8KRqPjoK1TSc0eHP/C0A4rBBMifEL0ofPz0yKb23n8
cQtm+UTtWCx0iwU8XQc+/8CDvLO4MwLTERvGbMElJLluLrgB58Idl1CR6648T8sB
N6WH9hI+KBQPS8brSKZ9hL7AZKQryEzBn1Uum7pCJkXqJyp5ahh50/251gHLOpM+
kKZ+rdmgJ7OtvR5Xt50uL+hPJ9BJNSSLJhT9hvER0BUlUwyWYewpQpMq3qBa0sh/
1q8jLzd7PJdO6cv+4w9Nt9lb1Mlj2R7BNyH81mqbJ4hcn4qj/TEGEaefwKy2cN+M
Pi8TngWEx64QP4JkTTw8p4PFJvDNzKDoqzV11v+hxz2ICUWqdUF0/3clClFQXeyL
IWRcuVAw4EdQxjS12afZ2xHr6gDKPBHojoUsUZDBaLbvBbIt99Ew1ye5G5eHr6JW
EklXYAkDabBiNpjXQWkpPUD7IRxa9oX7uGPUXPl4R9dQcygjCPDTkBPVmN+CM6Kt
KP6tIWBBRXXj0XPMwUhHG0ycRy7upxKZ78p7jkbnyHHXlxLT5BY=
=USRM
-----END PGP SIGNATURE-----

--wHQSRdwqq9BFgo/d--