Feedback-ID: i0fe9450f:Fastmail
Date: Wed, 4 Sep 2024 15:12:37 +0900
From: Michael Paquier <michael@paquier.xyz>
To: Bertrand Drouvot <bertranddrouvot.pg@gmail.com>
Cc: Kyotaro Horiguchi <horikyota.ntt@gmail.com>,
	pgsql-hackers@lists.postgresql.org
Subject: Re: Add callbacks for fixed-numbered stats flush in pgstats
Message-ID: <Ztf6VYDdohSuRACZ@paquier.xyz>
References: <ZtaVO0N-aTwiAk3w@paquier.xyz>
 <20240904.140546.948122275033564187.horikyota.ntt@gmail.com>
 <ZtfwGONWW0MegydF@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="4uTjVnhHU7HVKh+5"
Content-Disposition: inline
In-Reply-To: <ZtfwGONWW0MegydF@ip-10-97-1-34.eu-west-3.compute.internal>
Archived-At: <https://www.postgresql.org/message-id/Ztf6VYDdohSuRACZ%40paquier.xyz>
Precedence: bulk


--4uTjVnhHU7HVKh+5
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
Content-Transfer-Encoding: quoted-printable

On Wed, Sep 04, 2024 at 05:28:56AM +0000, Bertrand Drouvot wrote:
> On Wed, Sep 04, 2024 at 02:05:46PM +0900, Kyotaro Horiguchi wrote:
>> The generalization sounds good to me, and hiding the private flags in
>> private places also seems good.
>=20
> +1 on the idea.

Thanks for the feedback.

>> Regarding pgstat_io_flush_cb, I think it no longer needs to check the
>> have_iostats variable, and that check should be moved to the new
>> pgstat_flush_io(). The same applies to pgstat_wal_flush_cb() (and
>> pgstat_flush_wal()). As for pgstat_slru_flush_cb, it simply doesn't
>> need the check anymore.
>=20
> Agree. Another option could be to add only one callback (the flush_fixed_=
cb one)
> and get rid of the have_fixed_pending_cb one. Then add an extra parameter=
 to the
> flush_fixed_cb that would only check if there is pending stats (without
> flushing them). I think those 2 callbacks are highly related that's why I=
=20
> think we could "merge" them, thoughts?

I would still value the shortcut that we can use if there is no
activity to avoid the clock check with GetCurrentTimestamp(), though,
which is why I'd rather stick with two callbacks as that can lead to a
much cheaper path.

Anyway, I am not sure to follow your point about removing the boolean
checks in the flush callbacks.  It's surely always a better to never
call LWLockAcquire() or LWLockConditionalAcquire() if we don't have
to, and we would go through the whole flush dance as long as we detect
some stats activity in at least one stats kind.  I mean, that's cheap
enough to keep around.
--
Michael

--4uTjVnhHU7HVKh+5
Content-Type: application/pgp-signature; name="signature.asc"

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

iQIzBAABCgAdFiEEG72nH6vTowiyblFKnvQgOdbyQH0FAmbX+lUACgkQnvQgOdby
QH1Jqg/+JRoJ8U1mn7Iclf4LV7vOsxQAhBBqMgJsp0YWPQG3acNzn+9tar8rSRhc
g763kFEuEtSBbTxLZqvYGwbyFoWkRaUBFEul6HHaLsWqJJ4sBEcFYPqZs4HTBIF8
+AQSPhSS1mJUvPYJLH2zJ0B7rB0mRgcA+69SAbEAFwbt+euWuxzSTSV9GcizL3F3
P3c2d56CTqNVO+VsV01nS5Tq9rayL/fg9Cw1WClRFSFsDCHKlNMJ+a+/llK37hKS
9/fhf1wpOQmD/GmDY93lfLPPbNbxGjyYbkR+V7YDIQFgBxH1J80Yjnj1KJ6PjJfN
L7lECEMT2CvSfJdYkVX4lBaFwg7bEcWolA37bHiWjzHMMNksmJIjOA5FT66PRFvU
tnVL+o0PH9IxCRp0Gdbz31JWLFtMig6dd7SYSNMkHUM5Ux1l0MYlAb7QnXuwgCK7
WSRUN2YpiFpF05PKJp+MS9pZ6VnOTlkg+Y87MUVlB4KSBBjDspqcF9t8rX/wT3kZ
cNFcJHfubBQpmi7ZgdZpx23W1UCOrQDoRKbLcmPKBKBnwz6XFm14QkFTvj1O7dtq
fb6GFmkvA7vCRk78q6hOiueZ1A0Yqq5crs+regDGHpKMpEqUH0C3jqCiI95VXRW9
tq9n7VvgR2pMmtwOqWm7MxGtrxwGmMLjhC6xXuVGI7Q7lyr8FJg=
=GJbW
-----END PGP SIGNATURE-----

--4uTjVnhHU7HVKh+5--