Thread: Re: pgsql: Add function to log the memory contexts of specified backend pro
On Tue, Apr 6, 2021 at 12:45 AM Fujii Masao <fujii@postgresql.org> wrote: > Add function to log the memory contexts of specified backend process. Hi, I think this might need a recursion guard. I tried this: diff --git a/src/backend/tcop/postgres.c b/src/backend/tcop/postgres.c index dc4c600922d..b219a934034 100644 --- a/src/backend/tcop/postgres.c +++ b/src/backend/tcop/postgres.c @@ -3532,7 +3532,7 @@ ProcessInterrupts(void) if (ParallelMessagePending) ProcessParallelMessages(); - if (LogMemoryContextPending) + if (true) ProcessLogMemoryContextInterrupt(); if (PublishMemoryContextPending) diff --git a/src/include/miscadmin.h b/src/include/miscadmin.h index 72f5655fb34..867fd7b0ad5 100644 --- a/src/include/miscadmin.h +++ b/src/include/miscadmin.h @@ -112,7 +112,7 @@ extern void ProcessInterrupts(void); /* Test whether an interrupt is pending */ #ifndef WIN32 #define INTERRUPTS_PENDING_CONDITION() \ - (unlikely(InterruptPending)) + (unlikely(InterruptPending) || true) #else #define INTERRUPTS_PENDING_CONDITION() \ (unlikely(UNBLOCKED_SIGNAL_QUEUE()) ? pgwin32_dispatch_queued_signals() : 0, \ That immediately caused infinite recursion, ending in a core dump: frame #13: 0x0000000104607b00 postgres`errfinish(filename=<unavailable>, lineno=<unavailable>, funcname=<unavailable>) at elog.c:543:2 [opt] frame #14: 0x0000000104637078 postgres`ProcessLogMemoryContextInterrupt at mcxt.c:1392:2 [opt] frame #15: 0x00000001044a901c postgres`ProcessInterrupts at postgres.c:3536:3 [opt] frame #16: 0x0000000104607b54 postgres`errfinish(filename=<unavailable>, lineno=<unavailable>, funcname=<unavailable>) at elog.c:608:2 [opt] [artificial] frame #17: 0x0000000104637078 postgres`ProcessLogMemoryContextInterrupt at mcxt.c:1392:2 [opt] frame #18: 0x00000001044a901c postgres`ProcessInterrupts at postgres.c:3536:3 [opt] <repeat until we have 174241 frames on the stack, then dump core> It might be unlikely that a process can be signalled fast enough to actually fail in this way, but I'm not sure it's impossible, and I think we should be defending against it. The most trivial recursion guard would be HOLD_INTERRUPTS()/RESUME_INTERRUPTS() around ProcessLogMemoryContextInterrupt(), but I think that's probably not quite good enough because it would make the backend impervious to pg_terminate_backend() while it's dumping memory contexts, and that could be a long time if the write blocks. -- Robert Haas EDB: http://www.enterprisedb.com
On 2025/05/01 2:15, Robert Haas wrote: > On Tue, Apr 6, 2021 at 12:45 AM Fujii Masao <fujii@postgresql.org> wrote: >> Add function to log the memory contexts of specified backend process. > > Hi, > > I think this might need a recursion guard. I tried this: > > diff --git a/src/backend/tcop/postgres.c b/src/backend/tcop/postgres.c > index dc4c600922d..b219a934034 100644 > --- a/src/backend/tcop/postgres.c > +++ b/src/backend/tcop/postgres.c > @@ -3532,7 +3532,7 @@ ProcessInterrupts(void) > if (ParallelMessagePending) > ProcessParallelMessages(); > > - if (LogMemoryContextPending) > + if (true) > ProcessLogMemoryContextInterrupt(); > > if (PublishMemoryContextPending) > diff --git a/src/include/miscadmin.h b/src/include/miscadmin.h > index 72f5655fb34..867fd7b0ad5 100644 > --- a/src/include/miscadmin.h > +++ b/src/include/miscadmin.h > @@ -112,7 +112,7 @@ extern void ProcessInterrupts(void); > /* Test whether an interrupt is pending */ > #ifndef WIN32 > #define INTERRUPTS_PENDING_CONDITION() \ > - (unlikely(InterruptPending)) > + (unlikely(InterruptPending) || true) > #else > #define INTERRUPTS_PENDING_CONDITION() \ > (unlikely(UNBLOCKED_SIGNAL_QUEUE()) ? > pgwin32_dispatch_queued_signals() : 0, \ > > That immediately caused infinite recursion, ending in a core dump: > > frame #13: 0x0000000104607b00 > postgres`errfinish(filename=<unavailable>, lineno=<unavailable>, > funcname=<unavailable>) at elog.c:543:2 [opt] > frame #14: 0x0000000104637078 > postgres`ProcessLogMemoryContextInterrupt at mcxt.c:1392:2 [opt] > frame #15: 0x00000001044a901c postgres`ProcessInterrupts at > postgres.c:3536:3 [opt] > frame #16: 0x0000000104607b54 > postgres`errfinish(filename=<unavailable>, lineno=<unavailable>, > funcname=<unavailable>) at elog.c:608:2 [opt] [artificial] > frame #17: 0x0000000104637078 > postgres`ProcessLogMemoryContextInterrupt at mcxt.c:1392:2 [opt] > frame #18: 0x00000001044a901c postgres`ProcessInterrupts at > postgres.c:3536:3 [opt] > <repeat until we have 174241 frames on the stack, then dump core> > > It might be unlikely that a process can be signalled fast enough to > actually fail in this way, but I'm not sure it's impossible, and I > think we should be defending against it. The most trivial recursion > guard would be HOLD_INTERRUPTS()/RESUME_INTERRUPTS() around > ProcessLogMemoryContextInterrupt(), but I think that's probably not > quite good enough because it would make the backend impervious to > pg_terminate_backend() while it's dumping memory contexts, and that > could be a long time if the write blocks. Just idea, what do you think about adding a flag to indicate whether ProcessLogMemoryContextInterrupt() is currently running? Then, when a backend receives a signal and ProcessLogMemoryContextInterrupt() is invoked, it can simply return immediately if the flag is already set like this: ------------------------------ @ -1383,8 +1383,14 @@ HandleGetMemoryContextInterrupt(void) void ProcessLogMemoryContextInterrupt(void) { + static bool loggingMemoryContext = false; + LogMemoryContextPending = false; + if (loggingMemoryContext) + return; + loggingMemoryContext = true; + /* * Use LOG_SERVER_ONLY to prevent this message from being sent to the * connected client. @@ -1406,6 +1412,8 @@ ProcessLogMemoryContextInterrupt(void) * details about individual siblings beyond 100 will not be large. */ MemoryContextStatsDetail(TopMemoryContext, 100, 100, false); + + loggingMemoryContext = false; } ------------------------------ This way, we can safely ignore overlapping pg_log_backend_memory_contexts() requests while the function is already running. Thoughts? Regards, -- Fujii Masao Advanced Computing Technology Center Research and Development Headquarters NTT DATA CORPORATION
On Thu, May 1, 2025 at 3:53 AM Fujii Masao <masao.fujii@oss.nttdata.com> wrote: > Just idea, what do you think about adding a flag to indicate whether > ProcessLogMemoryContextInterrupt() is currently running? Then, > when a backend receives a signal and ProcessLogMemoryContextInterrupt() > is invoked, it can simply return immediately if the flag is already set > like this: I think that something like this could work, but you would need more than this. Otherwise, if the function errors out, the flag would remain permanently set. -- Robert Haas EDB: http://www.enterprisedb.com
On 2025/05/01 21:42, Robert Haas wrote: > On Thu, May 1, 2025 at 3:53 AM Fujii Masao <masao.fujii@oss.nttdata.com> wrote: >> Just idea, what do you think about adding a flag to indicate whether >> ProcessLogMemoryContextInterrupt() is currently running? Then, >> when a backend receives a signal and ProcessLogMemoryContextInterrupt() >> is invoked, it can simply return immediately if the flag is already set >> like this: > > I think that something like this could work, but you would need more > than this. Otherwise, if the function errors out, the flag would > remain permanently set. Yes, we need to either use PG_TRY()/PG_FINALLY() or handle the flag as a global variable and reset it in the error handling path. I think using PG_TRY()/PG_FINALLY() is the simpler option. Regards, -- Fujii Masao Advanced Computing Technology Center Research and Development Headquarters NTT DATA CORPORATION