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