Re: Enhancing Memory Context Statistics Reporting

Hi Michael,

Thank you for the review.

On Tue, Oct 22, 2024 at 12:18 PM Michael Paquier <michael(at)paquier(dot)xyz>
wrote:

> 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];
> [...]
> + memset(&memCtxState->memctx_infos, 0, 30 *
> sizeof(MemoryContextInfo));
> [...]
> + size = add_size(size, mul_size(30, sizeof(MemoryContextInfo)));
> [...]
> + memset(&memCtxState->memctx_infos, 0, 30 * sizeof(MemoryContextInfo));
> [...]
> + memset(&memCtxState->memctx_infos, 0, 30 *
> sizeof(MemoryContextInfo));
>
> 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.
>
>
Makes sense. Fixed in the attached 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.
>
> I added the #defines for these in the attached patch.
Size of the path array should match the number of levels in the memory
context tree and type is a constant string.

For the name and ident, I have used the existing #define
MEMORY_CONTEXT_IDENT_DISPLAY_SIZE as the size limit.

> > 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.
>

The rationale behind using the file is to cater to the unbounded
number of memory contexts.
The "in_use" flag is used to govern the access to shared memory
as I am reserving enough memory for only one backend.
It ensures that another backend does not overwrite the statistics
in the shared memory, before it is read by a client backend.

> 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.
>
> Thank you for the suggestion. I will look into using DSMs especially
if there is a way to limit the statistics dump, while still providing a
user
with enough information to debug memory consumption.

In this draft, I preferred using a file over DSMs, as a file can provide
ample space for dumping a large number of memory context statistics
without the risk of DSM creation failure due to insufficient memory.

Also, why the single-backend limitation?

To reduce the memory footprint, the shared memory is
created for only one backend.
Each backend has to wait for previous operation
to finish before it can write.

I think a good use case for this would be a background process
periodically running the monitoring function on each of the
backends sequentially to fetch the statistics.
This way there will be little contention for shared memory.

In case a shared memory is not available, a backend immediately
returns from the interrupt handler without blocking its normal
operations.

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.
>

I agree that such an infrastructure would be useful for fetching memory
statistics concurrently without significant synchronization overhead.
However, a drawback of this approach is reserving shared
memory slots up to MAX_BACKENDS without utilizing them
when no concurrent monitoring is happening.
As you mentioned, creating a DSM on the fly when a dump
request is received could help avoid over-allocating shared memory.
I will look into this suggestion

Thank you for your feedback!

Rahila Syed