On 11/6/2025 8:42 AM, Bryan Green wrote:
> On 11/6/2025 7:43 AM, Thomas Munro wrote:
>> On Sat, Nov 1, 2025 at 6:16 AM Bryan Green <dbryan(dot)green(at)gmail(dot)com> wrote:
>>> Hello,
>>
>> Catching up with all your emails, and I must say it's great to see
>> some solid investigation of PostgreSQL-on-Windows problems. There are
>> ... more.
>>
>>> Commit 1da569ca1f (March 2023) added O_CLOEXEC to many call sites
>>> throughout the backend with a comment saying "Our open() replacement
>>> does not create inheritable handles, so it is safe to ignore
>>> O_CLOEXEC." But that doesn't appear to match what the code actually
>>> does. I'm wondering if I've misunderstood something about how handle
>>> inheritance works on Windows, or if the comment was based on a
>>> misunderstanding of the code path.
>>
>> Yeah, it looks like I was just wrong. Oops. Your analysis looks good to me.
>>
>>> The fix would be straightforward if this is indeed wrong. Define
>>> O_CLOEXEC to a non-zero value like 0x80000 (in the private use range
>>> for open() flags), and then honor it in pgwin32_open() by setting
>>> sa.bInheritHandle based on whether the flag is present:
>>>
>>> sa.bInheritHandle = (fileFlags & O_CLOEXEC) ? FALSE : TRUE;
>>
>> Looking at fcntl.h, that's the next free bit, but also the one they'll
>> presumably define next (I guess "private use range" is just a turn of
>> phrase and not a real thing?), so why not use the highest free bit
>> after O_DIRECT? We have three fake open flags, one of which
>> cybersquats a real flag from fcntl.h, ironically the one that actually
>> means O_CLOEXEC. We can't change existing values in released
>> branches, so that'd give:
>>
>> #define O_DIRECT 0x80000000
>> #define O_CLOEXEC 0x04000000
>> #define O_DSYNC _O_NO_INHERIT
>>
>> Perhaps in master we could rearrange them:
>>
>> #define O_DIRECT 0x80000000
>> #define O_DSYNC 0x04000000
>> #define O_CLOEXEC _O_NO_INHERIT
>>
>>> So my questions are: Am I correct that both conditions for handle
>>> inheritance are met, meaning handles really are being inherited by
>>> archive_command children? Is there something in Windows that prevents
>>> inheritance that I don't know about? If this is a real bug, would it
>>> make sense to backpatch to v16 where O_CLOEXEC was added? I'm happy to
>>> provide my test code or do additional testing if that would help.
>>
>> Yeah, seems like it, and like we should back-patch this. I vote for
>> doing that after the upcoming minor releases. Holding files open on
>> Windows unintentionally is worse on Windows than on Unix (preventing
>> directories from being unlinked etc). Of course we've done that for
>> decades so I doubt it's really a big deal, but we should clean up this
>> mistake.
>
> Thanks for reviewing this and confirming the analysis. Good to know I
> wasn't missing something about Windows handle inheritance.
>
> Your point about the bit value makes sense - using 0x04000000 (highest
> free bit after O_DIRECT) is definitely safer than 0x80000 which could
> collide with future fcntl.h additions. I also appreciate the irony you
> pointed out - we're currently using _O_NO_INHERIT (which literally
> prevents handle inheritance on Windows) for O_DSYNC instead of
> O_CLOEXEC. The rearrangement in master to use _O_NO_INHERIT for what it
> actually means semantically makes a lot of sense.
>
> So the plan would be:
>
> Backport branches (v16+):
> #define O_DIRECT 0x80000000
> #define O_CLOEXEC 0x04000000
> #define O_DSYNC _O_NO_INHERIT
>
> Master:
> #define O_DIRECT 0x80000000
> #define O_DSYNC 0x04000000
> #define O_CLOEXEC _O_NO_INHERIT
>
> And then in pgwin32_open():
> sa.bInheritHandle = (fileFlags & O_CLOEXEC) ? FALSE : TRUE;
>
> I will prepare a new version of the patch that implements the suggested
> change for master.
>
>
The changes for master, along with a tap test, are provided with the
attached patch.
--
Bryan Green
EDB: https://www.enterprisedb.com