Feedback-ID: i0fe9450f:Fastmail
Date: Tue, 22 Oct 2024 15:48:39 +0900
From: Michael Paquier <michael@paquier.xyz>
To: Rahila Syed <rahilasyed90@gmail.com>
Cc: PostgreSQL-development <pgsql-hackers@postgresql.org>
Subject: Re: Enhancing Memory Context Statistics Reporting
Message-ID: <ZxdKx0DywUTAvkEF@paquier.xyz>
References: <CAH2L28v8mc9HDt8QoSJ8TRmKau_8FM_HKS41NeO9-6ZAkuZKXw@mail.gmail.com>
MIME-Version: 1.0
Content-Type: multipart/signed; micalg=pgp-sha512;
	protocol="application/pgp-signature"; boundary="KHQneR2SzURBtvH5"
Content-Disposition: inline
In-Reply-To: <CAH2L28v8mc9HDt8QoSJ8TRmKau_8FM_HKS41NeO9-6ZAkuZKXw@mail.gmail.com>
Archived-At: <https://www.postgresql.org/message-id/ZxdKx0DywUTAvkEF%40paquier.xyz>
Precedence: bulk


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

On Mon, Oct 21, 2024 at 11:54:21PM +0530, Rahila Syed wrote:
> On the other hand, [2] provides the statistics for all backends but logs
> them in a file, which may not be convenient for quick access.

To be precise, pg_log_backend_memory_contexts() pushes the memory
context stats to LOG_SERVER_ONLY or stderr, hence this is appended to
the server logs.

> A fixed-size shared memory block, currently accommodating 30 records,
>  is used to store the statistics. This number was chosen arbitrarily,
>  as it covers all parent contexts at level 1 (i.e., direct children of the
> top memory context)
> based on my tests.
> Further experiments are needed to determine the optimal number
> for summarizing memory statistics.

+ * Statistics are shared via fixed shared memory which
+ * can hold statistics for 29 contexts. The rest of the
[...]
+   MemoryContextInfo memctx_infos[30];=20
[...]
+       memset(&memCtxState->memctx_infos, 0, 30 * sizeof(MemoryContextInfo=
));=20
[...]
+   size =3D add_size(size, mul_size(30, sizeof(MemoryContextInfo)));
[...]
+   memset(&memCtxState->memctx_infos, 0, 30 * sizeof(MemoryContextInfo));=
=20
[...]
+       memset(&memCtxState->memctx_infos, 0, 30 * sizeof(MemoryContextInfo=
));=20

This number is tied to MemoryContextState added by the patch.  Sounds
like this would be better as a constant properly defined rather than
hardcoded in all these places.  This would make the upper-bound more
easily switchable in the patch.

+   Datum       path[128];
+   char        type[128];
[...]
+   char        name[1024];
+   char        ident[1024];
+   char        type[128];
+   Datum       path[128];

Again, constants.  Why these values?  You may want to use more
#defines here.

> Any additional statistics that exceed the shared memory capacity
> are written to a file per backend in the PG_TEMP_FILES_DIR. The client
> backend
>  first reads from the shared memory, and if necessary, retrieves the
> remaining data from the file,
> combining everything into a unified view. The files are cleaned up
> automatically
> if a backend crashes or during server restarts.

Is the addition of the file to write any remaining stats really that
useful?  This comes with a heavy cost in the patch with the "in_use"
flag, the various tweaks around the LWLock release/acquire protecting
the shmem area and the extra cleanup steps required after even a clean
restart.  That's a lot of facility for this kind of information.
Another thing that may be worth considering is to put this information
in a DSM per the variable-size nature of the information, perhaps cap
it to a max to make the memory footprint cheaper, and avoid all
on-disk footprint because we don't need it to begin with as this is
information that makes sense only while the server is running.

Also, why the single-backend limitation?  One could imagine a shared
memory area indexed similarly to pgproc entries, that includes
auxiliary processes as much as backends, so as it can be possible to
get more memory footprints through SQL for more than one single
process at one moment in time.  If each backend has its own area of
shmem to deal with, they could use a shared LWLock on the shmem area
with an extra spinlock while the context data is dumped into memory as
the copy is short-lived.  Each one of them could save the information
in a DSM created only when a dump of the shmem is requested for a
given PID, for example.
--
Michael

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

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

iQIzBAABCgAdFiEEG72nH6vTowiyblFKnvQgOdbyQH0FAmcXSscACgkQnvQgOdby
QH2+PQ/7BjMJig7VdJ7fkYh+nUFotSn5o2rcABJR016yqBKGhX2YN40IY+RZQYvV
A0WfTGWVuBkqMoBzpetzWtEv9c+iA/6wZljrv/YbLXNSWhFZjlDW+YMLYgsQzMXZ
XNuDhjq/vQe9y5QmN6dq6+rJriGSz9he31xA5SFjghqKmRDdxEUrWosqvluUx1kO
Jyv4CObatUvtYMNRKcBYN4/l5HpZJuYv92fQaAnvHm5FB94QfbrHmcBjXgeoNtCr
dv+bJeWr7cOU7Tz2SgzePVmsPX/naJDqfycFPrbHctNCvfE+2qKIlheO1IVidXhV
Hj3jizAbYpXxzBFGzl/ouTAjEN/s0tOen07NAEKI+2gPJOgQt+COVmjrRkzqiZ0G
6+ETBqNRDFDHIwjb34qlRceQ4162XXr0DR192BlGSw+lBRumS4oXZhJIcZBt1hmp
wWcTJlLZob24YHthU2yEn23rrA3ypF41wuSywSr7CoOXPKeNEp4amsKtNFDLKXO3
08JIAakeRRqivYPANkZ9InuWKn5S4M5lukB0dn1CFvdCZDAvqkQhNOENshd1gpO1
wjnUYqDG2pMDVq8YihaFR0dVhTzfwsHyLxKg6GbKMzMYLUi3F7jaefj0is2zDPKu
uYjf/fixizvf++zlRciihh+1S5eD0nQlAfHCtqTveFLpsMlesJg=
=0SDV
-----END PGP SIGNATURE-----

--KHQneR2SzURBtvH5--