Re: Enhancing Memory Context Statistics Reporting

Hi Rahila,

I’ve spent some time reviewing the patch, and the review is still
ongoing. Here are the comments I’ve found so far.

1.
The tests are currently missing. Could you please add them?

2.
I have some concerns regarding the function name
‘pg_get_remote_backend_memory_contexts’. Specifically, the term
‘remote’ doesn’t seem appropriate to me. The function retrieves data
from other processes running on the same machine, which might give the
impression that it deals with processes on different machines. This
could be misleading or unclear in this context. The argument ‘pid’
already indicates that it can get data from different processes.
Additionally, the term ‘backend’ also seems inappropriate since we are
obtaining data from processes that are different from backend
processes.

3.
> + Datum values[10];
> + bool nulls[10];

Please consider #defining the column count, or you could reuse the
existing one ‘PG_GET_BACKEND_MEMORY_CONTEXTS_COLS’.

4.
> if (context_id <= 28)
> if (context_id == 29)
> if (context_id < 29)

#define these

5.
> for (MemoryContext cur_context = cur; cur_context != NULL; cur_context = cur_context->parent)
> {
> MemoryContextId *cur_entry;
>
> cur_entry = hash_search(context_id_lookup, &cur_context, HASH_FIND, &found);
>
> if (!found)
> {
> elog(LOG, "hash table corrupted, can't construct path value");
> break;
> }
> path = lcons_int(cur_entry->context_id, path);
> }

Similar code already exists in PutMemoryContextsStatsTupleStore().
Could you create a separate function to handle this?

6.
> /*
> * Shared memory is full, release lock and write to file from next
> * iteration
> */
> context_id++;
> if (context_id == 29)
> {

What if there are exactly 29 entries in the memory context? In that
case, creating the file would be unnecessary.

Best Regards,
Nitin Jadhav
Azure Database for PostgreSQL
Microsoft

On Wed, Oct 23, 2024 at 10:20 AM Rahila Syed <rahilasyed90(at)gmail(dot)com> wrote:
>
> 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