Re: Get memory contexts of an arbitrary backend process

Thanks for reviewing!

I'm going to modify the patch according to your comments.

On 2020-09-01 10:54, Andres Freund wrote:
> Hi,
>
> On 2020-08-31 20:22:18 +0900, torikoshia wrote:
>> After commit 3e98c0bafb28de, we can display the usage of the
>> memory contexts using pg_backend_memory_contexts system
>> view.
>>
>> However, its target is limited to the process attached to
>> the current session.
>>
>> As discussed in the thread[1], it'll be useful to make it
>> possible to get the memory contexts of an arbitrary backend
>> process.
>>
>> Attached PoC patch makes pg_get_backend_memory_contexts()
>> display meory contexts of the specified PID of the process.
>
> Awesome!
>
>
>> It doesn't display contexts of all the backends but only
>> the contexts of specified process.
>> I think it would be enough because I suppose this function
>> is used after investigations using ps command or other OS
>> level utilities.
>
> It can be used as a building block if all are needed. Getting the
> infrastructure right is the big thing here, I think. Adding more
> detailed views on top of that data later is easier.
>
>
>
>> diff --git a/src/backend/catalog/system_views.sql
>> b/src/backend/catalog/system_views.sql
>> index a2d61302f9..88fb837ecd 100644
>> --- a/src/backend/catalog/system_views.sql
>> +++ b/src/backend/catalog/system_views.sql
>> @@ -555,10 +555,10 @@ REVOKE ALL ON pg_shmem_allocations FROM PUBLIC;
>> REVOKE EXECUTE ON FUNCTION pg_get_shmem_allocations() FROM PUBLIC;
>>
>> CREATE VIEW pg_backend_memory_contexts AS
>> - SELECT * FROM pg_get_backend_memory_contexts();
>> + SELECT * FROM pg_get_backend_memory_contexts(-1);
>
> -1 is odd. Why not use NULL or even 0?
>
>> + else
>> + {
>> + int rc;
>> + int parent_len = strlen(parent);
>> + int name_len = strlen(name);
>> +
>> + /*
>> + * write out the current memory context information.
>> + * Since some elements of values are reusable, we write it out.
>
> Not sure what the second comment line here is supposed to mean?
>
>
>> + */
>> + fputc('D', fpout);
>> + rc = fwrite(values, sizeof(values), 1, fpout);
>> + rc = fwrite(nulls, sizeof(nulls), 1, fpout);
>> +
>> + /* write out information which is not resuable from serialized
>> values */
>
> s/resuable/reusable/
>
>
>> + rc = fwrite(&name_len, sizeof(int), 1, fpout);
>> + rc = fwrite(name, name_len, 1, fpout);
>> + rc = fwrite(&idlen, sizeof(int), 1, fpout);
>> + rc = fwrite(clipped_ident, idlen, 1, fpout);
>> + rc = fwrite(&level, sizeof(int), 1, fpout);
>> + rc = fwrite(&parent_len, sizeof(int), 1, fpout);
>> + rc = fwrite(parent, parent_len, 1, fpout);
>> + (void) rc; /* we'll check for error with ferror */
>> +
>> + }
>
> This format is not descriptive. How about serializing to json or
> something? Or at least having field names?
>
> Alternatively, build the same tuple we build for the SRF, and serialize
> that. Then there's basically no conversion needed.
>
>
>> @@ -117,6 +157,8 @@ PutMemoryContextsStatsTupleStore(Tuplestorestate
>> *tupstore,
>> Datum
>> pg_get_backend_memory_contexts(PG_FUNCTION_ARGS)
>> {
>> + int pid = PG_GETARG_INT32(0);
>> +
>> ReturnSetInfo *rsinfo = (ReturnSetInfo *) fcinfo->resultinfo;
>> TupleDesc tupdesc;
>> Tuplestorestate *tupstore;
>> @@ -147,11 +189,258 @@
>> pg_get_backend_memory_contexts(PG_FUNCTION_ARGS)
>>
>> MemoryContextSwitchTo(oldcontext);
>>
>> - PutMemoryContextsStatsTupleStore(tupstore, tupdesc,
>> - TopMemoryContext, NULL, 0);
>> + if (pid == -1)
>> + {
>> + /*
>> + * Since pid -1 indicates target is the local process, simply
>> + * traverse memory contexts.
>> + */
>> + PutMemoryContextsStatsTupleStore(tupstore, tupdesc,
>> + TopMemoryContext, "", 0, NULL);
>> + }
>> + else
>> + {
>> + /*
>> + * Send signal for dumping memory contexts to the target process,
>> + * and read the dumped file.
>> + */
>> + FILE *fpin;
>> + char dumpfile[MAXPGPATH];
>> +
>> + SendProcSignal(pid, PROCSIG_DUMP_MEMORY, InvalidBackendId);
>> +
>> + snprintf(dumpfile, sizeof(dumpfile), "pg_memusage/%d", pid);
>> +
>> + while (true)
>> + {
>> + CHECK_FOR_INTERRUPTS();
>> +
>> + pg_usleep(10000L);
>> +
>
> Need better signalling back/forth here.

Do you mean I should also send another signal from the dumped
process to the caller of the pg_get_backend_memory_contexts()
when it finishes dumping?

Regards,

--
Atsushi Torikoshi
NTT DATA CORPORATION

>
>
>
>> +/*
>> + * dump_memory_contexts
>> + * Dumping local memory contexts to a file.
>> + * This function does not delete the file as it is intended to be
>> read by
>> + * another process.
>> + */
>> +static void
>> +dump_memory_contexts(void)
>> +{
>> + FILE *fpout;
>> + char tmpfile[MAXPGPATH];
>> + char dumpfile[MAXPGPATH];
>> +
>> + snprintf(tmpfile, sizeof(tmpfile), "pg_memusage/%d.tmp", MyProcPid);
>> + snprintf(dumpfile, sizeof(dumpfile), "pg_memusage/%d", MyProcPid);
>> +
>> + /*
>> + * Open a temp file to dump the current memory context.
>> + */
>> + fpout = AllocateFile(tmpfile, PG_BINARY_W);
>> + if (fpout == NULL)
>> + {
>> + ereport(LOG,
>> + (errcode_for_file_access(),
>> + errmsg("could not write temporary memory context file \"%s\":
>> %m",
>> + tmpfile)));
>> + return;
>> + }
>
> Probably should be opened with O_CREAT | O_TRUNC?
>
>
> Greetings,
>
> Andres Freund