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
