Re: pgsql: Add function to log the memory contexts of specified backend pro

Fujii Masao <masao.fujii@oss.nttdata.com>

From: Fujii Masao <masao.fujii@oss.nttdata.com>

To: Robert Haas <robertmhaas@gmail.com>

Cc: torikoshia <torikoshia@oss.nttdata.com>, PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>

Date: 2025-05-07T09:06:22Z

Lists: pgsql-hackers

Commits

Same data as JSON: GET /api/v1/messages/:b64id/commits the thread's linked commits as JSON, with link sources. API reference →

Add guard to prevent recursive memory context logging.
- 1a9a49699c21 14.21 landed
- 0fc2f533a96c 15.16 landed
- 3853f61681e8 16.12 landed
- 699293d2749a 17.8 landed
- b863d8d87fc1 18.2 landed
- b3ccb0a2cb2e 19 (unreleased) landed
Add function to log the memory contexts of specified backend process.
- 43620e328617 14.0 cited
Add pg_backend_memory_contexts system view.
- 3e98c0bafb28 14.0 cited

Attachments

v3-0001-PG14-PG16-Add-guard-to-prevent-recursive-memory-context-log.patch (text/plain) patch v3-0001
v3-0001-PG17-master-Add-guard-to-prevent-recursive-memory-context-log.patch (text/plain) patch v3-0001


On 2025/05/05 23:57, Robert Haas wrote:
> On Fri, May 2, 2025 at 9:54 PM Fujii Masao <masao.fujii@oss.nttdata.com> wrote:
>> Thanks for the review and testing! I've fixed the issue you pointed out.
>> Updated patch attached.
> 
> Thanks for addressing this. However, I believe this commit may need to
> take note of the following comment from elog.h:

Thanks for the review!


>   * Note: if a local variable of the function containing PG_TRY is modified
>   * in the PG_TRY section and used in the PG_CATCH section, that variable
>   * must be declared "volatile" for POSIX compliance.  This is not mere
>   * pedantry; we have seen bugs from compilers improperly optimizing code
>   * away when such a variable was not marked.  Beware that gcc's -Wclobbered
>   * warnings are just about entirely useless for catching such oversights.
> 
> Based on this comment, I believe in_progress must be declared volatile.

You're right. OTOH, setting the flag inside the PG_TRY() block isn't necessary,
so I've moved it outside instead of leaving it inside and marking the flag volatile.


> As a stylistic comment, I think I would prefer making in_progress a
> file-level global and giving it a less generic name (e.g.
> LogMemoryContextInProgress). However, perhaps others will disagree.

I'm fine with this. I've renamed the flag and made it a file-level global
variable as suggested. Updated patch is attached.

Regards,

-- 
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION