Date: Mon, 31 Aug 2020 18:54:29 -0700
From: Andres Freund <andres@anarazel.de>
To: torikoshia <torikoshia@oss.nttdata.com>
Cc: pgsql-hackers@postgresql.org
Subject: Re: Get memory contexts of an arbitrary backend process
Message-ID: <20200901015429.nhhte6wkbplingc6@alap3.anarazel.de>
References: <0271f440ac77f2a4180e0e56ebd944d1@oss.nttdata.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <0271f440ac77f2a4180e0e56ebd944d1@oss.nttdata.com>
Precedence: bulk

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.


> +/*
> + * 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