Re: Get memory contexts of an arbitrary backend process - Mailing list pgsql-hackers

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