pg.ddx.io pgsql-hackers@postgresql.org mailing list archive
help / color / mirror / Atom feedRe: Get memory contexts of an arbitrary backend process
7+ messages / 5 participants
[nested] [flat]
* Re: Get memory contexts of an arbitrary backend process
@ 2020-09-01 01:54 Andres Freund <andres@anarazel.de>
1 sibling, 1 reply; 7+ messages in thread
From: Andres Freund @ 2020-09-01 01:54 UTC (permalink / raw)
To: torikoshia <torikoshia@oss.nttdata.com>; +Cc: pgsql-hackers
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
^ permalink raw reply [nested|flat] 7+ messages in thread
* Re: Get memory contexts of an arbitrary backend process
@ 2020-09-03 06:38 torikoshia <torikoshia@oss.nttdata.com>
1 sibling, 1 reply; 7+ messages in thread
From: torikoshia @ 2020-09-03 06:38 UTC (permalink / raw)
To: "Pavel Stehule <pavel.stehule@gmail.com>; Kasahara Tatsuhito" <kasahara.tatsuhito@gmail.com>; +Cc: pgsql-hackers
On 2020-09-01 03:29, Pavel Stehule wrote:
> Hi
>
> po 31. 8. 2020 v 17:03 odesÃlatel Kasahara Tatsuhito
> <kasahara.tatsuhito@gmail.com> napsal:
>
>> Hi,
>>
>> On Mon, Aug 31, 2020 at 8:22 PM torikoshia
>> <torikoshia@oss.nttdata.com> wrote:
>>> As discussed in the thread[1], it'll be useful to make it
>>> possible to get the memory contexts of an arbitrary backend
>>> process.
>> +1
>>
>>> Attached PoC patch makes pg_get_backend_memory_contexts()
>>> display meory contexts of the specified PID of the process.
>> Thanks, it's a very good patch for discussion.
>>
>>> It doesn't display contexts of all the backends but only
>>> the contexts of specified process.
>> or we can "SELECT (pg_get_backend_memory_contexts(pid)).* FROM
>> pg_stat_activity WHERE ...",
>> so I don't think it's a big deal.
>>
>>> The rough idea of implementation is like below:
>>>
>>> 1. send a signal to the specified process
>>> 2. signaled process dumps its memory contexts to a file
>>> 3. read the dumped file and display it to the user
>> I agree with the overview of the idea.
>> Here are some comments and questions.
Thanks for the comments!
>>
>> - Currently, "the signal transmission for dumping memory
>> information"
>> and "the read & output of dump information "
>> are on the same interface, but I think it would be better to
>> separate them.
>> How about providing the following three types of functions for
>> users?
>> - send a signal to specified pid
>> - check the status of the signal sent and received
>> - read the dumped information
Is this for future extensibility to make it possible to get
other information like the current execution plan which was
suggested by Pavel?
If so, I agree with considering extensibility, but I'm not
sure it's necessary whether providing these types of
functions for 'users'.
>> - How about managing the status of signal send/receive and dump
>> operations on a shared hash or others ?
>> Sending and receiving signals, dumping memory information, and
>> referencing dump information all work asynchronously.
>> Therefore, it would be good to have management information to
>> check
>> the status of each process.
>> A simple idea is that ..
>> - send a signal to dump to a PID, it first record following
>> information into the shared hash.
>> pid (specified pid)
>> loc (dump location, currently might be ASAP)
>> recv (did the pid process receive a signal? first false)
>> dumped (did the pid process dump a mem information? first
>> false)
>> - specified process receive the signal, update the status in the
>> shared hash, then dumped at specified location.
>> - specified process finish dump mem information, update the
>> status
>> in the shared hash.
Adding management information on shared memory seems necessary
when we want to have more controls over dumping like 'dump
location' or any other information such as 'current execution
plan'.
I'm going to consider this.
>> - Does it allow one process to output multiple dump files?
>> It appears to be a specification to overwrite at present, but I
>> thought it would be good to be able to generate
>> multiple dump files in different phases (e.g., planning phase and
>> execution phase) in the future.
>> - How is the dump file cleaned up?
>
> For a very long time there has been similar discussion about taking
> session query and session execution plans from other sessions.
>
> I am not sure how necessary information is in the memory dump, but I
> am sure so taking the current execution plan and complete text of the
> current query is pretty necessary information.
>
> but can be great so this infrastructure can be used for any debugging
> purpose.
Thanks!
It would be good if some part of this effort can be an infrastructure
of other debugging.
It may be hard, but I will keep your comment in mind.
Regards,
--
Atsushi Torikoshi
NTT DATA CORPORATION
>
> Regards
>
> Pavel
>
>> Best regards,
>>
>> --
>> Tatsuhito Kasahara
>> kasahara.tatsuhito _at_ gmail.com [1]
>
>
> Links:
> ------
> [1] http://gmail.com
^ permalink raw reply [nested|flat] 7+ messages in thread
* Re: Get memory contexts of an arbitrary backend process
@ 2020-09-03 06:40 torikoshia <torikoshia@oss.nttdata.com>
parent: Andres Freund <andres@anarazel.de>
0 siblings, 0 replies; 7+ messages in thread
From: torikoshia @ 2020-09-03 06:40 UTC (permalink / raw)
To: Andres Freund <andres@anarazel.de>; +Cc: 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
^ permalink raw reply [nested|flat] 7+ messages in thread
* Re: Get memory contexts of an arbitrary backend process
@ 2020-09-03 17:18 Kasahara Tatsuhito <kasahara.tatsuhito@gmail.com>
parent: torikoshia <torikoshia@oss.nttdata.com>
0 siblings, 1 reply; 7+ messages in thread
From: Kasahara Tatsuhito @ 2020-09-03 17:18 UTC (permalink / raw)
To: torikoshia <torikoshia@oss.nttdata.com>; +Cc: pgsql-hackers
Hi,
On Thu, Sep 3, 2020 at 3:38 PM torikoshia <torikoshia@oss.nttdata.com> wrote:
> >> - Currently, "the signal transmission for dumping memory
> >> information"
> >> and "the read & output of dump information "
> >> are on the same interface, but I think it would be better to
> >> separate them.
> >> How about providing the following three types of functions for
> >> users?
> >> - send a signal to specified pid
> >> - check the status of the signal sent and received
> >> - read the dumped information
>
> Is this for future extensibility to make it possible to get
> other information like the current execution plan which was
> suggested by Pavel?
Yes, but it's not only for future expansion, but also for the
usability and the stability of this feature.
For example, if you want to read one dumped file multiple times and analyze it,
you will want the ability to just read the dump.
Moreover, when it takes a long time from the receive the signal to the
dump output,
or the dump output itself takes a long time, users can investigate
where it takes time
if each process is separated.
> If so, I agree with considering extensibility, but I'm not
> sure it's necessary whether providing these types of
> functions for 'users'.
Of course, it is possible and may be necessary to provide a wrapped
sequence of processes
from sending a signal to reading dump files.
But IMO, some users would like to perform the signal transmission,
state management and
dump file reading processes separately.
Best regards,
--
Tatsuhito Kasahara
kasahara.tatsuhito _at_ gmail.com
^ permalink raw reply [nested|flat] 7+ messages in thread
* Re: Get memory contexts of an arbitrary backend process
@ 2020-09-03 17:40 Tom Lane <tgl@sss.pgh.pa.us>
parent: Kasahara Tatsuhito <kasahara.tatsuhito@gmail.com>
0 siblings, 1 reply; 7+ messages in thread
From: Tom Lane @ 2020-09-03 17:40 UTC (permalink / raw)
To: Kasahara Tatsuhito <kasahara.tatsuhito@gmail.com>; +Cc: torikoshia <torikoshia@oss.nttdata.com>; pgsql-hackers
Kasahara Tatsuhito <kasahara.tatsuhito@gmail.com> writes:
> Yes, but it's not only for future expansion, but also for the
> usability and the stability of this feature.
> For example, if you want to read one dumped file multiple times and analyze it,
> you will want the ability to just read the dump.
If we design it to make that possible, how are we going to prevent disk
space leaks from never-cleaned-up dump files?
regards, tom lane
^ permalink raw reply [nested|flat] 7+ messages in thread
* Re: Get memory contexts of an arbitrary backend process
@ 2020-09-04 02:47 Kasahara Tatsuhito <kasahara.tatsuhito@gmail.com>
parent: Tom Lane <tgl@sss.pgh.pa.us>
0 siblings, 1 reply; 7+ messages in thread
From: Kasahara Tatsuhito @ 2020-09-04 02:47 UTC (permalink / raw)
To: Tom Lane <tgl@sss.pgh.pa.us>; +Cc: torikoshia <torikoshia@oss.nttdata.com>; pgsql-hackers
On Fri, Sep 4, 2020 at 2:40 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Kasahara Tatsuhito <kasahara.tatsuhito@gmail.com> writes:
> > Yes, but it's not only for future expansion, but also for the
> > usability and the stability of this feature.
> > For example, if you want to read one dumped file multiple times and analyze it,
> > you will want the ability to just read the dump.
>
> If we design it to make that possible, how are we going to prevent disk
> space leaks from never-cleaned-up dump files?
In my thought, with features such as a view that allows us to see a
list of dumped files,
it would be better to have a function that simply deletes the dump
files associated with a specific PID,
or to delete all dump files.
Some files may be dumped with unexpected delays, so I think the
cleaning feature will be necessary.
( Also, as the pgsql_tmp file, it might better to delete dump files
when PostgreSQL start.)
Or should we try to delete the dump file as soon as we can read it?
Best regards,
--
Tatsuhito Kasahara
kasahara.tatsuhito _at_ gmail.com
^ permalink raw reply [nested|flat] 7+ messages in thread
* Re: Get memory contexts of an arbitrary backend process
@ 2020-09-04 12:46 Tomas Vondra <tomas.vondra@2ndquadrant.com>
parent: Kasahara Tatsuhito <kasahara.tatsuhito@gmail.com>
0 siblings, 0 replies; 7+ messages in thread
From: Tomas Vondra @ 2020-09-04 12:46 UTC (permalink / raw)
To: Kasahara Tatsuhito <kasahara.tatsuhito@gmail.com>; +Cc: Tom Lane <tgl@sss.pgh.pa.us>; torikoshia <torikoshia@oss.nttdata.com>; pgsql-hackers
On Fri, Sep 04, 2020 at 11:47:30AM +0900, Kasahara Tatsuhito wrote:
>On Fri, Sep 4, 2020 at 2:40 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> Kasahara Tatsuhito <kasahara.tatsuhito@gmail.com> writes:
>> > Yes, but it's not only for future expansion, but also for the
>> > usability and the stability of this feature.
>> > For example, if you want to read one dumped file multiple times and analyze it,
>> > you will want the ability to just read the dump.
>>
>> If we design it to make that possible, how are we going to prevent disk
>> space leaks from never-cleaned-up dump files?
>In my thought, with features such as a view that allows us to see a
>list of dumped files,
>it would be better to have a function that simply deletes the dump
>files associated with a specific PID,
>or to delete all dump files.
>Some files may be dumped with unexpected delays, so I think the
>cleaning feature will be necessary.
>( Also, as the pgsql_tmp file, it might better to delete dump files
>when PostgreSQL start.)
>
>Or should we try to delete the dump file as soon as we can read it?
>
IMO making the cleanup a responsibility of the users (e.g. by exposing
the list of dumped files through a view and expecting users to delete
them in some way) is rather fragile.
I don't quite see what's the point of designing it this way. It was
suggested this improves stability and usability of this feature, but
surely making it unnecessarily complex contradicts both points?
IMHO if the user needs to process the dump repeatedly, what's preventing
him/her from storing it in a file, or something like that? At that point
it's clear it's up to them to remove the file. So I suggest to keep the
feature as simple as possible - hand the dump over and delete.
regards
--
Tomas Vondra http://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
^ permalink raw reply [nested|flat] 7+ messages in thread
end of thread, other threads:[~2020-09-04 12:46 UTC | newest]
Thread overview: 7+ messages (download: mbox mbox.gz follow: Atom feed)
-- links below jump to the message on this page --
2020-09-01 01:54 ` Andres Freund <andres@anarazel.de>
2020-09-03 06:40 ` torikoshia <torikoshia@oss.nttdata.com>
2020-09-03 06:38 ` torikoshia <torikoshia@oss.nttdata.com>
2020-09-03 17:18 ` Kasahara Tatsuhito <kasahara.tatsuhito@gmail.com>
2020-09-03 17:40 ` Tom Lane <tgl@sss.pgh.pa.us>
2020-09-04 02:47 ` Kasahara Tatsuhito <kasahara.tatsuhito@gmail.com>
2020-09-04 12:46 ` Tomas Vondra <tomas.vondra@2ndquadrant.com>
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox