[PATCH] O_CLOEXEC not honored on Windows - handle inheritance chain

Hello,

While reviewing the pg_ctl CreateProcess patch [1], I started looking
into handle inheritance on Windows and found something that puzzles me.
I think there's an issue with O_CLOEXEC, but wanted to walk through my
reasoning to make sure I'm not missing something obvious.

[1]
https://www.postgresql.org/message-id/flat/TYAPR01MB586654E2D74B838021BE77CAF5EEA(at)TYAPR01MB5866(dot)jpnprd01(dot)prod(dot)outlook(dot)com

The issue appears to be that O_CLOEXEC doesn't actually do anything on
Windows. When PostgreSQL opens a WAL file in xlog.c, it specifies
O_CLOEXEC in the OpenTransientFile() call, expecting the file handle to
be non-inheritable. However, O_CLOEXEC is defined as 0 in win32_port.h,
and our pgwin32_open() function in src/port/open.c unconditionally sets
sa.bInheritHandle = TRUE at line 80. So the flag is simply ignored, and
all file handles are created inheritable.

Now, for a handle to actually be inherited by a child process on
Windows, two conditions must both be true. First, the handle itself must
have been created with bInheritHandle = TRUE (which we do for
everything). Second, the parent must call CreateProcess with
bInheritHandles = TRUE. So the question becomes: does that second
condition ever happen?

It does. When archive_command runs, PostgreSQL calls pgwin32_system(),
which wraps the command in quotes and passes it to the Microsoft C
runtime's system() function. That function needs to make stdio work for
the child process, so it has no choice but to call CreateProcess with
bInheritHandles = TRUE. This means cmd.exe inherits all our file
handles, including any open database or WAL files, and cmd.exe passes
them along to copy.exe or whatever command is being run.

I wrote a test program that links against libpgport.a to verify this
behavior. It opens files with and without O_CLOEXEC using the actual
pgwin32_open() function, then spawns a child process with
bInheritHandles = TRUE (mimicking what system() does) and tries to
access the handles from the child. Both files were accessible from the
child process regardless of whether O_CLOEXEC was specified. The flag
has no effect.

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.

The practical impact of this seems low. Child processes spawned by
archive_command or COPY PROGRAM operate on file paths passed as
arguments, not on inherited file descriptors, so they don't actually
make use of the handles even though they have them. Even if a child
wanted to use an inherited handle, it would need to somehow learn the
numeric handle value, which isn't passed through our IPC mechanisms.
And Windows users probably employ archive_command less frequently than
Unix users anyway.

Nonetheless, if this is really a bug, it's a correctness issue. It
violates the documented semantics of O_CLOEXEC, contradicts what our
own comments claim, and means PostgreSQL behaves differently on Windows
than on Unix. It also creates unnecessary handle leaks where files
can't be deleted or renamed while child processes are running. While
reviewing my pg_ctl patch, I realized it would make handle inheritance
more explicit and direct, which made me want to understand whether
O_CLOEXEC actually works.

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;

We also have to add the O_CLOEXEC to the assertion in open.c that
validates that fileFlags only contains known flags.

I've tested this change locally and it works as expected. Files opened
with O_CLOEXEC are not accessible from child processes, while files
opened without it are accessible.

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.

For reference, the Microsoft documentation on handle inheritance is at:
https://learn.microsoft.com/en-us/windows/win32/api/processthreadsapi/nf-processthreadsapi-createprocessa

And I confirmed through research that UCRT's system() does use
bInheritHandles = TRUE, which makes sense since it needs stdio to work.

Bryan Green

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.

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.

--
Bryan Green
EDB: https://www.enterprisedb.com

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

On 11/7/2025 11:28 AM, Bryan Green wrote:
> On 11/6/2025 8:42 AM, Bryan Green wrote:
>> On 11/6/2025 7:43 AM, Thomas Munro wrote:
...
>> 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.
>

Thanks to CI discovered a mistake in the makefile and meson.build file
for the tests. New patch attached.
--
Bryan Green
EDB: https://www.enterprisedb.com

On Wed, Nov 12, 2025 at 2:01 PM Bryan Green <dbryan(dot)green(at)gmail(dot)com> wrote:
> [v3]

"The original commit included a comment suggesting that our open()
replacement doesn't create inheritable handles, but it appears this
may have been based on a misunderstanding of the code path. In
practice, the code was creating inheritable handles in all cases."

s/it appears this may have been been/was/ :-)

"To fix, define O_CLOEXEC to a nonzero value (0x80000, in the private
use range for open() flags). Then honor it in pgwin32_open_handle()"

Out of date, maybe skip mentioning the value in the commit message?
Maybe add a note here about the back-branches preserving existing
values and master cleaning up? Do you happen to have a fixup that
supplies the difference in the back-branches so we can see it passing
in CI?

+ * Note: We could instead use SetHandleInformation() after CreateFile() to
+ * clear HANDLE_FLAG_INHERIT, but setting bInheritHandle=FALSE is simpler
+ * and achieves the same result.
+ */

It also wouldn't be thread-safe. That is meaningful today for
frontend programs (and hopefully some day soon also in the backend).

+ sa.bInheritHandle = (fileFlags & O_CLOEXEC) ? FALSE : TRUE;

Just out of sheer curiosity, I often see gratuitous FALSE and TRUE in
Windowsian code, not false and true and not reduced to eg !(fileFlags
& O_CLOEXEC). Is that a code convention thing from somewhere in
Windows-land?

+++ b/src/test/modules/test_cloexec/test_cloexec.c

I like the test. Very helpful for people reliant on CI for Windows coverage.

Independently of all this, and just on the off-chance that it might be
interesting to you in future, I have previously tried to write tests
for our whole Windows filesystem shim layer and found lots of bugs and
understood lots of quirks that way, but never got it to be good enough
for inclusion in the tree:

https://www.postgresql.org/message-id/flat/CA%2BhUKG%2BajSQ_8eu2AogTncOnZ5me2D-Cn66iN_-wZnRjLN%2Bicg%40mail.gmail.com

There is some overlap with several of your recent patches, as I was
testing some of the same wrappers. One of the main things we've
battled with in this project is the whole asynchronous-unlink thing,
and generally the NT/VMS file locking concept which can't quite be
entirely emulated away, and that was one of my main focuses there
after we got CI and started debugging the madness. Doing so revealed
a bunch of interesting differences in Windows versions and file
systems, and to this day we don't really have a project policy on
which Windows filesystems PostgreSQL supports (cf your mention of
needing NTFS for sparse files in one of your other threads, though I
can't imagine that ReFS AKA DevDrive doesn't have those too being a
COW system).

Speaking of file I/O, and just as an FYI, I have a bunch of
semi-working unfinished patches that bring true scatter/gather I/O
(instead of the simple looping fallback in pg_preadv()) and native
async I/O (for files, but actually also pipes and sockets but let me
stick to talking about files for now) to Windows (traditional
OVERLAPPED and/or IoRing.h, neither can do everything we need would
you believe). Development via trial-by-CI from the safety of my Unix
box is slow and painful going, but... let's say a real Windows
programmer with a systems programming bent showed up and were
interested in this stuff, I would be more than happy to write down
everything I think I know about those subjects and post the unfinished
work and then I bet development would go fast... just sayin'.

On 11/11/2025 8:34 PM, Thomas Munro wrote:
> On Wed, Nov 12, 2025 at 2:01 PM Bryan Green <dbryan(dot)green(at)gmail(dot)com> wrote:
>> [v3]
>
> "The original commit included a comment suggesting that our open()
> replacement doesn't create inheritable handles, but it appears this
> may have been based on a misunderstanding of the code path. In
> practice, the code was creating inheritable handles in all cases."
>
> s/it appears this may have been been/was/ :-)
>

Changed.

> "To fix, define O_CLOEXEC to a nonzero value (0x80000, in the private
> use range for open() flags). Then honor it in pgwin32_open_handle()"
>

Removed.
> Out of date, maybe skip mentioning the value in the commit message?
> Maybe add a note here about the back-branches preserving existing
> values and master cleaning up? Do you happen to have a fixup that
> supplies the difference in the back-branches so we can see it passing
> in CI?
>

I have attached a back-patch for v16-v18.

> + * Note: We could instead use SetHandleInformation() after CreateFile() to
> + * clear HANDLE_FLAG_INHERIT, but setting bInheritHandle=FALSE is simpler
> + * and achieves the same result.
> + */
>
> It also wouldn't be thread-safe. That is meaningful today for
> frontend programs (and hopefully some day soon also in the backend).
>
> + sa.bInheritHandle = (fileFlags & O_CLOEXEC) ? FALSE : TRUE;
>
> Just out of sheer curiosity, I often see gratuitous FALSE and TRUE in
> Windowsian code, not false and true and not reduced to eg !(fileFlags
> & O_CLOEXEC). Is that a code convention thing from somewhere in
> Windows-land?
>

Yes, old habits die hard. I learned this pattern on Windows.
Interestingly, enough when I am not on Windows I write the way you suggest.

> +++ b/src/test/modules/test_cloexec/test_cloexec.c
>
> I like the test. Very helpful for people reliant on CI for Windows coverage.
>
> Independently of all this, and just on the off-chance that it might be
> interesting to you in future, I have previously tried to write tests
> for our whole Windows filesystem shim layer and found lots of bugs and
> understood lots of quirks that way, but never got it to be good enough
> for inclusion in the tree:
>
> https://www.postgresql.org/message-id/flat/CA%2BhUKG%2BajSQ_8eu2AogTncOnZ5me2D-Cn66iN_-wZnRjLN%2Bicg%40mail.gmail.com
>

I shall take a look.

> There is some overlap with several of your recent patches, as I was
> testing some of the same wrappers. One of the main things we've
> battled with in this project is the whole asynchronous-unlink thing,
> and generally the NT/VMS file locking concept which can't quite be
> entirely emulated away, and that was one of my main focuses there
> after we got CI and started debugging the madness. Doing so revealed
> a bunch of interesting differences in Windows versions and file
> systems, and to this day we don't really have a project policy on
> which Windows filesystems PostgreSQL supports (cf your mention of
> needing NTFS for sparse files in one of your other threads, though I
> can't imagine that ReFS AKA DevDrive doesn't have those too being a
> COW system).
>
> Speaking of file I/O, and just as an FYI, I have a bunch of
> semi-working unfinished patches that bring true scatter/gather I/O
> (instead of the simple looping fallback in pg_preadv()) and native
> async I/O (for files, but actually also pipes and sockets but let me
> stick to talking about files for now) to Windows (traditional
> OVERLAPPED and/or IoRing.h, neither can do everything we need would
> you believe). Development via trial-by-CI from the safety of my Unix
> box is slow and painful going, but... let's say a real Windows
> programmer with a systems programming bent showed up and were
> interested in this stuff, I would be more than happy to write down
> everything I think I know about those subjects and post the unfinished
> work and then I bet development would go fast... just sayin'.

I would absolutely love to read everything you think you know about
those subjects and contribute to the work.

--
Bryan Green
EDB: https://www.enterprisedb.com

On 11/12/2025 4:10 PM, Bryan Green wrote:
> On 11/11/2025 8:34 PM, Thomas Munro wrote:
>> On Wed, Nov 12, 2025 at 2:01 PM Bryan Green <dbryan(dot)green(at)gmail(dot)com> wrote:
>>> [v3]
>>
>> "The original commit included a comment suggesting that our open()
>> replacement doesn't create inheritable handles, but it appears this
>> may have been based on a misunderstanding of the code path. In
>> practice, the code was creating inheritable handles in all cases."
>>
>> s/it appears this may have been been/was/ :-)
>>
>
> Changed.
>
>> "To fix, define O_CLOEXEC to a nonzero value (0x80000, in the private
>> use range for open() flags). Then honor it in pgwin32_open_handle()"
>>
>
> Removed.
>> Out of date, maybe skip mentioning the value in the commit message?
>> Maybe add a note here about the back-branches preserving existing
>> values and master cleaning up? Do you happen to have a fixup that
>> supplies the difference in the back-branches so we can see it passing
>> in CI?
>>
>
> I have attached a back-patch for v16-v18.
>
>> + * Note: We could instead use SetHandleInformation() after CreateFile() to
>> + * clear HANDLE_FLAG_INHERIT, but setting bInheritHandle=FALSE is simpler
>> + * and achieves the same result.
>> + */
>>
>> It also wouldn't be thread-safe. That is meaningful today for
>> frontend programs (and hopefully some day soon also in the backend).
>>
>> + sa.bInheritHandle = (fileFlags & O_CLOEXEC) ? FALSE : TRUE;
>>
>> Just out of sheer curiosity, I often see gratuitous FALSE and TRUE in
>> Windowsian code, not false and true and not reduced to eg !(fileFlags
>> & O_CLOEXEC). Is that a code convention thing from somewhere in
>> Windows-land?
>>
>
> Yes, old habits die hard. I learned this pattern on Windows.
> Interestingly, enough when I am not on Windows I write the way you suggest.
>
>> +++ b/src/test/modules/test_cloexec/test_cloexec.c
>>
>> I like the test. Very helpful for people reliant on CI for Windows coverage.
>>
>> Independently of all this, and just on the off-chance that it might be
>> interesting to you in future, I have previously tried to write tests
>> for our whole Windows filesystem shim layer and found lots of bugs and
>> understood lots of quirks that way, but never got it to be good enough
>> for inclusion in the tree:
>>
>> https://www.postgresql.org/message-id/flat/CA%2BhUKG%2BajSQ_8eu2AogTncOnZ5me2D-Cn66iN_-wZnRjLN%2Bicg%40mail.gmail.com
>>
>
> I shall take a look.
>
>> There is some overlap with several of your recent patches, as I was
>> testing some of the same wrappers. One of the main things we've
>> battled with in this project is the whole asynchronous-unlink thing,
>> and generally the NT/VMS file locking concept which can't quite be
>> entirely emulated away, and that was one of my main focuses there
>> after we got CI and started debugging the madness. Doing so revealed
>> a bunch of interesting differences in Windows versions and file
>> systems, and to this day we don't really have a project policy on
>> which Windows filesystems PostgreSQL supports (cf your mention of
>> needing NTFS for sparse files in one of your other threads, though I
>> can't imagine that ReFS AKA DevDrive doesn't have those too being a
>> COW system).
>>
>> Speaking of file I/O, and just as an FYI, I have a bunch of
>> semi-working unfinished patches that bring true scatter/gather I/O
>> (instead of the simple looping fallback in pg_preadv()) and native
>> async I/O (for files, but actually also pipes and sockets but let me
>> stick to talking about files for now) to Windows (traditional
>> OVERLAPPED and/or IoRing.h, neither can do everything we need would
>> you believe). Development via trial-by-CI from the safety of my Unix
>> box is slow and painful going, but... let's say a real Windows
>> programmer with a systems programming bent showed up and were
>> interested in this stuff, I would be more than happy to write down
>> everything I think I know about those subjects and post the unfinished
>> work and then I bet development would go fast... just sayin'.
>
> I would absolutely love to read everything you think you know about
> those subjects and contribute to the work.
>
>
Corrected master patch and back patch for v16-v18.

--
Bryan Green
EDB: https://www.enterprisedb.com

On Thu, Nov 13, 2025 at 11:17 AM Bryan Green <dbryan(dot)green(at)gmail(dot)com> wrote:
> Corrected master patch and back patch for v16-v18.

Thanks.

I wondered what system-generated new handles might appear in a child
process and potentially collide with a non-inherited handle's
numerical value (perhaps a thread handle or something like that?), but
I guess it'd also have to accept a write to confuse the test, which
seems unlikely, so that's probably OK. I also hope that the new test
could eventually be merged with a general port layer test suite as
mentioned earlier.

What do you think about these improvements? See attached.

* moved and adjusted new comment about flag conversion to cover all
three flags, since it's true for all of them
* adjusted the comment about why we don't use SetHandleInformation()
to highlight that it would be (slightly) leaky
* removed O_DIRECT's extra definition from port.h, since it's now in
win32_port.h

One question is why on earth the open() redirection is in port.h while
the "supplements to fcntl.h" are in win32_port.h. Obviously those are
tightly coupled. As far as I know there are two forces keeping some
Windows porting magic in port.h that we'd ideally isolate in
win32_port.h, and this case doesn't appear to qualify for either as
far as I can guess, anyway:

* some sleep/retry wrappers were thought to be needed by Cygwin too:
API-wise it's a POSIX, but it couldn't hide the underlying NT file
locking semantics
* sometimes we need a later definition time: I recall battling that
for #define ftruncate and/or lseek (if you define them before certain
system headers are included, you break them)

Cygwin's <fcntl.h> defines these flags, if I've found the right
file[1], and has its own open() that we're using directly. If it
didn't, our code would have failed to compile when Cygwin was being
tested in the build farm up until a year or so ago (and it will be
tested again soon[2]). So we could probably move at least open() into
win32_port.h, in a separate commit. I think it's also quite likely
that Cygwin turns on the Windows 10 POSIX directory entry semantics,
so even rename() etc could probably be moved over too. (Whether our
own porting layer should turn that stuff on too and delete the retry
stuff entirely is an open question which no Windows expert has ever
opined on, only us Unix hackers battling against random failures in
the build farm.) We should probably also set up a CI task for Cygwin
if we're keeping support.

[1] https://github.com/cygwin/cygwin/blob/main/newlib/libc/include/sys/_default_fcntl.h
[2] https://www.postgresql.org/message-id/flat/916d0fd1-a99b-41c4-a017-ff2428bf8cca%40dunslane.net

On Sun, Nov 30, 2025 at 1:13 PM Thomas Munro <thomas(dot)munro(at)gmail(dot)com> wrote:
> What do you think about these improvements? See attached.
>
> * moved and adjusted new comment about flag conversion to cover all
> three flags, since it's true for all of them
> * adjusted the comment about why we don't use SetHandleInformation()
> to highlight that it would be (slightly) leaky
> * removed O_DIRECT's extra definition from port.h, since it's now in
> win32_port.h

Hearing nothing, pushed.

I realised while back-patching that REL_16_STABLE was the last release
that also had the old Windows-only build/test machinery under
src/tools/msvc. It never ran all the tests anyway, and I don't think
we'd learn anything new by adding it, given that CI uses meson on that
branch, so I didn't worry about remembering how to adjust that for
now. If someone feels strongly about it, of course we can.

On 12/9/2025 3:03 PM, Thomas Munro wrote:
> On Sun, Nov 30, 2025 at 1:13 PM Thomas Munro <thomas(dot)munro(at)gmail(dot)com> wrote:
>> What do you think about these improvements? See attached.
>>
>> * moved and adjusted new comment about flag conversion to cover all
>> three flags, since it's true for all of them
>> * adjusted the comment about why we don't use SetHandleInformation()
>> to highlight that it would be (slightly) leaky
>> * removed O_DIRECT's extra definition from port.h, since it's now in
>> win32_port.h
>
> Hearing nothing, pushed.
>
> I realised while back-patching that REL_16_STABLE was the last release
> that also had the old Windows-only build/test machinery under
> src/tools/msvc. It never ran all the tests anyway, and I don't think
> we'd learn anything new by adding it, given that CI uses meson on that
> branch, so I didn't worry about remembering how to adjust that for
> now. If someone feels strongly about it, of course we can.
Well, my drafts folder had nothing but agreement in it...

Thanks,

--
Bryan Green
EDB: https://www.enterprisedb.com

Thomas Munro <thomas(dot)munro(at)gmail(dot)com> writes:
> Hearing nothing, pushed.

fairywren is unimpressed:

../pgsql/src/test/modules/test_cloexec/test_cloexec.c: In function 'run_parent_tests':
../pgsql/src/test/modules/test_cloexec/test_cloexec.c:137:29: warning: unused variable 'space_pos' [-Wunused-variable]
137 | char *space_pos;
| ^~~~~~~~~

It's right, but it seems to me that this stanza needs more help than
that:

/*
* Spawn child process with bInheritHandles=TRUE, passing handle values as
* hex strings
*/
snprintf(cmdline, sizeof(cmdline), "\"%s\" %p %p",
GetCommandLine(), h1, h2);

/*
* Find the actual executable path by removing any arguments from
* GetCommandLine().
*/
{
char exe_path[MAX_PATH];
char *space_pos;

GetModuleFileName(NULL, exe_path, sizeof(exe_path));
snprintf(cmdline, sizeof(cmdline), "\"%s\" %p %p",
exe_path, h1, h2);
}

What is the point of that first snprintf(cmdline, ...), when its
result is guaranteed to be overwritten just below?

I'm also dubious about using MAX_PATH here; see the commentary
about MAXPGPATH in pg_config_manual.h. Also, what's the point of
using MAX_PATH when the result is going to be transferred into
cmdline (with a hardwired size of 1024)?

regards, tom lane

On Sat, Dec 13, 2025 at 3:44 PM Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> Thomas Munro <thomas(dot)munro(at)gmail(dot)com> writes:
> > Hearing nothing, pushed.
>
> fairywren is unimpressed:
>
> ../pgsql/src/test/modules/test_cloexec/test_cloexec.c: In function 'run_parent_tests':
> ../pgsql/src/test/modules/test_cloexec/test_cloexec.c:137:29: warning: unused variable 'space_pos' [-Wunused-variable]
> 137 | char *space_pos;
> | ^~~~~~~~~

The CI MinGW task also shows this warning, but it doesn't use -Werror.
The separate CompileWarnings task does, being its purpose, and it
includes a MinGW cross-build step, but that uses configure, and this
test is built only by meson. That wasn't a great idea... we knew we
were only dealing with Windows but forgot about MinGW, so I'll go and
write a patch to fix that aspect later today so we're covered for
warnings. I'll also think about whether it's worth checking for MinGW
warnings in both assert and non-assert builds (as we do for regular
Linux gcc/clang), and I'd also like to try to catch warnings from MSVC
and had an idea for how to do that... I might also try to think about
meson-vs-configure cross checks...

> What is the point of that first snprintf(cmdline, ...), when its
> result is guaranteed to be overwritten just below?
>
> I'm also dubious about using MAX_PATH here; see the commentary
> about MAXPGPATH in pg_config_manual.h. Also, what's the point of
> using MAX_PATH when the result is going to be transferred into
> cmdline (with a hardwired size of 1024)?

Fair points, I'll wait and see if Bryan is free to write a patch on
Monday (US), and otherwise write one myself.

On 12/12/2025 9:57 PM, Thomas Munro wrote:
> On Sat, Dec 13, 2025 at 3:44 PM Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>> Thomas Munro <thomas(dot)munro(at)gmail(dot)com> writes:
>>> Hearing nothing, pushed.
>>
>> fairywren is unimpressed:
>>
>> ../pgsql/src/test/modules/test_cloexec/test_cloexec.c: In function 'run_parent_tests':
>> ../pgsql/src/test/modules/test_cloexec/test_cloexec.c:137:29: warning: unused variable 'space_pos' [-Wunused-variable]
>> 137 | char *space_pos;
>> | ^~~~~~~~~
>
> The CI MinGW task also shows this warning, but it doesn't use -Werror.
> The separate CompileWarnings task does, being its purpose, and it
> includes a MinGW cross-build step, but that uses configure, and this
> test is built only by meson. That wasn't a great idea... we knew we
> were only dealing with Windows but forgot about MinGW, so I'll go and
> write a patch to fix that aspect later today so we're covered for
> warnings. I'll also think about whether it's worth checking for MinGW
> warnings in both assert and non-assert builds (as we do for regular
> Linux gcc/clang), and I'd also like to try to catch warnings from MSVC
> and had an idea for how to do that... I might also try to think about
> meson-vs-configure cross checks...
>
>> What is the point of that first snprintf(cmdline, ...), when its
>> result is guaranteed to be overwritten just below?
>>
>> I'm also dubious about using MAX_PATH here; see the commentary
>> about MAXPGPATH in pg_config_manual.h. Also, what's the point of
>> using MAX_PATH when the result is going to be transferred into
>> cmdline (with a hardwired size of 1024)?
>
> Fair points, I'll wait and see if Bryan is free to write a patch on
> Monday (US), and otherwise write one myself.
I will write a patch tonight. This was my sloppiness from doing
incremental changes and not cleaning up behind myself. I'll clean it
up. Thanks for the checks...

--
Bryan Green
EDB: https://www.enterprisedb.com

On 12/12/2025 9:57 PM, Thomas Munro wrote:
> On Sat, Dec 13, 2025 at 3:44 PM Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>> Thomas Munro <thomas(dot)munro(at)gmail(dot)com> writes:
>>> Hearing nothing, pushed.
>>
>> fairywren is unimpressed:
>>
>> ../pgsql/src/test/modules/test_cloexec/test_cloexec.c: In function 'run_parent_tests':
>> ../pgsql/src/test/modules/test_cloexec/test_cloexec.c:137:29: warning: unused variable 'space_pos' [-Wunused-variable]
>> 137 | char *space_pos;
>> | ^~~~~~~~~
>
> The CI MinGW task also shows this warning, but it doesn't use -Werror.
> The separate CompileWarnings task does, being its purpose, and it
> includes a MinGW cross-build step, but that uses configure, and this
> test is built only by meson. That wasn't a great idea... we knew we
> were only dealing with Windows but forgot about MinGW, so I'll go and
> write a patch to fix that aspect later today so we're covered for
> warnings. I'll also think about whether it's worth checking for MinGW
> warnings in both assert and non-assert builds (as we do for regular
> Linux gcc/clang), and I'd also like to try to catch warnings from MSVC
> and had an idea for how to do that... I might also try to think about
> meson-vs-configure cross checks...
>
>> What is the point of that first snprintf(cmdline, ...), when its
>> result is guaranteed to be overwritten just below?
>>
>> I'm also dubious about using MAX_PATH here; see the commentary
>> about MAXPGPATH in pg_config_manual.h. Also, what's the point of
>> using MAX_PATH when the result is going to be transferred into
>> cmdline (with a hardwired size of 1024)?
>
> Fair points, I'll wait and see if Bryan is free to write a patch on
> Monday (US), and otherwise write one myself.
Thomas,

A sanity check would be appreciated after the somewhat embarrassing
sloppy code.

I removed the useless snprintf() call that was using GetCommandLine().
That was left in place when I moved to GetModuleFileName(). Also,
removed the unused 'space_pos' variable and the unneeded scope block. I
decided to just use 1024 for the exe_path size since that is what
cmdline is set to use. I also removed some self-evident comments that
were leftover from my practice of writing comments and then coding.

Apologies for the loss of time.

Thanks,

--
Bryan Green
EDB: https://www.enterprisedb.com

Bryan Green <dbryan(dot)green(at)gmail(dot)com> writes:
> I removed the useless snprintf() call that was using GetCommandLine().
> That was left in place when I moved to GetModuleFileName(). Also,
> removed the unused 'space_pos' variable and the unneeded scope block.

All good to my eye.

> I decided to just use 1024 for the exe_path size since that is what
> cmdline is set to use.

Personally I'd have gone the other way, say

char exe_path[MAXPGPATH];
char cmdline[MAXPGPATH + 100];

> I also removed some self-evident comments that
> were leftover from my practice of writing comments and then coding.

I think you went way overboard on removing "self-evident" comments.
Signposts as to what the code intends to do are pretty helpful IMO.
They do have to be accurate though, for instance this previous
comment:

- * Find the actual executable path by removing any arguments from
- * GetCommandLine().

didn't seem to convey what the code was doing (which I neglected
to complain about before).

BTW, pgindent will undo some of the whitespace changes you made
here.

regards, tom lane

On Sat, Dec 13, 2025 at 7:33 PM Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>
> Bryan Green <dbryan(dot)green(at)gmail(dot)com> writes:
> > I removed the useless snprintf() call that was using GetCommandLine().
> > That was left in place when I moved to GetModuleFileName(). Also,
> > removed the unused 'space_pos' variable and the unneeded scope block.
>
> All good to my eye.

Thanks both.

> > I decided to just use 1024 for the exe_path size since that is what
> > cmdline is set to use.
>
> Personally I'd have gone the other way, say
>
> char exe_path[MAXPGPATH];
> char cmdline[MAXPGPATH + 100];

Done in this version.

> > I also removed some self-evident comments that
> > were leftover from my practice of writing comments and then coding.
>
> I think you went way overboard on removing "self-evident" comments.
> Signposts as to what the code intends to do are pretty helpful IMO.
> They do have to be accurate though, for instance this previous
> comment:
>
> - * Find the actual executable path by removing any arguments from
> - * GetCommandLine().
>
> didn't seem to convey what the code was doing (which I neglected
> to complain about before).

Comments restored in the attached version of Bryan's patch.

My earlier guess about the Makefile was wrong, and when I looked into
it the actual problems were (1) that the CompilerWarnings task in CI
runs make world-bin, which doesn't descend into src/test, and (2) that
the test ifeq ($(PORTNAME), win32) was not satisfied due to make's
rules for variable evaluation. I thought about how to fix that but
realised that this is going to be much easier to maintain if it's not
different on Unix, so here are some fixes in that direction. With
just 0001 and 0002 applied, we'd have known about the compiler warning
before commit, with a failure like this:

https://cirrus-ci.com/task/5863371716689920

With 0003 applied on top, it's green and there are no warnings from
either Windows task:

https://cirrus-ci.com/build/4775547869331456

I also changed the comment style of some single-line comments.
replaced the memset() with initializer syntax and ran pgindent which
undid a change or two.

On Sun, Dec 14, 2025 at 6:09 PM Thomas Munro <thomas(dot)munro(at)gmail(dot)com> wrote:
> My earlier guess about the Makefile was wrong, and when I looked into
> it the actual problems were (1) that the CompilerWarnings task in CI
> runs make world-bin, which doesn't descend into src/test, and (2) that
> the test ifeq ($(PORTNAME), win32) was not satisfied due to make's
> rules for variable evaluation. I thought about how to fix that but
> realised that this is going to be much easier to maintain if it's not
> different on Unix, so here are some fixes in that direction. With
> just 0001 and 0002 applied, we'd have known about the compiler warning
> before commit, with a failure like this:

I pushed the cleanup patch.

I wondered if there might be any other C code that could be checked
for compiler warnings by CI and isn't yet, and the only thing I have
some up with so far is the .pgc -> .c stuff. Here is a new version
that does that too. I also back-patched a fix for a warning (see
eab2323c) that would break if we back-patched this. Is there anything
else like this hiding somewhere?

Thomas Munro <thomas(dot)munro(at)gmail(dot)com> writes:
> I pushed the cleanup patch.

fairywren's still not happy, though only in the v16 branch:

Could not determine contrib module type for test_cloexec
at build.pl line 54.

This evidently is because the old MSVC build system doesn't
recognize

PROGRAM += test_cloexec
OBJS += $(WIN32RES) test_cloexec.o

Is there a reason for using += not just = here? We could certainly
modify Mkvcbuild.pm to parse this if we need to, but it looks more
like a gratuitous difference from everyplace else than a useful
behavior.

regards, tom lane

On Mon, Dec 22, 2025 at 11:48 AM Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> fairywren's still not happy, though only in the v16 branch:
>
> Could not determine contrib module type for test_cloexec
> at build.pl line 54.
>
> This evidently is because the old MSVC build system doesn't
> recognize
>
> PROGRAM += test_cloexec
> OBJS += $(WIN32RES) test_cloexec.o
>
> Is there a reason for using += not just = here? We could certainly
> modify Mkvcbuild.pm to parse this if we need to, but it looks more
> like a gratuitous difference from everyplace else than a useful
> behavior.

Yeah. Will drop the + signs. I've also written a patch to enable a
separate "Windows - Server 2022, VS 2019 - Mkvcbuild.pm" CI task
alongside the Meson one in the REL_16_STABLE branch. I had run the
affected branches through CI, but of course 16 switched to Meson so it
wasn't testing the third build system... without that, this is just
too painful. I need to step away for a couple of hours, but more in a
bit...

On Mon, Dec 22, 2025 at 1:24 PM Thomas Munro <thomas(dot)munro(at)gmail(dot)com> wrote:
> On Mon, Dec 22, 2025 at 11:48 AM Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> > fairywren's still not happy, though only in the v16 branch:
> >
> > Could not determine contrib module type for test_cloexec
> > at build.pl line 54.
> >
> > This evidently is because the old MSVC build system doesn't
> > recognize
> >
> > PROGRAM += test_cloexec
> > OBJS += $(WIN32RES) test_cloexec.o
> >
> > Is there a reason for using += not just = here? We could certainly
> > modify Mkvcbuild.pm to parse this if we need to, but it looks more
> > like a gratuitous difference from everyplace else than a useful
> > behavior.
>
> Yeah. Will drop the + signs. I've also written a patch to enable a
> separate "Windows - Server 2022, VS 2019 - Mkvcbuild.pm" CI task
> alongside the Meson one in the REL_16_STABLE branch. I had run the
> affected branches through CI, but of course 16 switched to Meson so it
> wasn't testing the third build system... without that, this is just
> too painful. I need to step away for a couple of hours, but more in a
> bit...

That revealed another problem: Mkvcbuild.pm didn't add -lpgport. It
looks out for the pattern PG_LIBS_INTERNAL = $(libpq_pgport), so
that's an easy way to fix that -- is there a better way? I couldn't
figure out how to tell it that we need libpqport but not libpq.
Here's a CI run of these patches on top of REL_16_STABLE, showing the
new Mkvcbuild.pm task passing:

https://cirrus-ci.com/build/5900754273173504

Thomas Munro <thomas(dot)munro(at)gmail(dot)com> writes:
> That revealed another problem: Mkvcbuild.pm didn't add -lpgport. It
> looks out for the pattern PG_LIBS_INTERNAL = $(libpq_pgport), so
> that's an easy way to fix that -- is there a better way? I couldn't
> figure out how to tell it that we need libpqport but not libpq.

AFAICT from looking at v16 Mkvcbuild.pm, PG_LIBS_INTERNAL = $(libpq_pgport)
will do exactly what you want because that only triggers it to add
libpgport and libpgcommon (cf. lines 1053ff). I'm a little baffled
by that --- shouldn't it be pulling in libpq as well? But let's let
sleeping dogs lie.

regards, tom lane

On Tue, Dec 23, 2025 at 10:50 AM Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> Thomas Munro <thomas(dot)munro(at)gmail(dot)com> writes:
> > That revealed another problem: Mkvcbuild.pm didn't add -lpgport. It
> > looks out for the pattern PG_LIBS_INTERNAL = $(libpq_pgport), so
> > that's an easy way to fix that -- is there a better way? I couldn't
> > figure out how to tell it that we need libpqport but not libpq.
>
> AFAICT from looking at v16 Mkvcbuild.pm, PG_LIBS_INTERNAL = $(libpq_pgport)
> will do exactly what you want because that only triggers it to add
> libpgport and libpgcommon (cf. lines 1053ff). I'm a little baffled
> by that --- shouldn't it be pulling in libpq as well? But let's let
> sleeping dogs lie.

Thanks for looking. Yeah. That line should probably be v16-only,
conditional on Windows and have a comment to explain. Will confirm
that and push this later today, along with those two CI changes[1][2]
that would have avoided all this trouble.

[1] https://www.postgresql.org/message-id/CA%2BhUKG%2BFRtqt0ZZVxb4e4h1gRnpECvehGB5JHqK73q8LxhbbVQ%40mail.gmail.com
[2] https://www.postgresql.org/message-id/CA%2BhUKGJreZe9jzPA6jDsLLB18d7M0rvypdbV18UBMxiFxdsf2g%40mail.gmail.com

Hello Bryan,

13.12.2025 04:44, Tom Lane wrote:
> Thomas Munro<thomas(dot)munro(at)gmail(dot)com> writes:
>> Hearing nothing, pushed.
> fairywren is unimpressed:

Now that fairywren is pretty satisfied with the test, it has produced an
interesting failure [1]:
208/222 postgresql:test_cloexec / test_cloexec/001_cloexec                        ERROR 4.09s   exit status 1

regress_log_001_cloexec
[14:36:36.820](1.047s) 1..1
[14:36:36.821](0.001s) # Using test program:
C:\\tools\\xmsys64\\home\\pgrunner\\bf\\root\\REL_16_STABLE\\pgsql.build\\tmp_install\\tools\\xmsys64\\home\\pgrunner\\bf\\root\\REL_16_STABLE\\inst\\bin\\test_cloexec.exe
[14:36:37.445](0.624s) # Test program output:
[14:36:37.446](0.001s) # Child: Received HANDLE1=00000000000000B0 (should fail - O_CLOEXEC)
# Child: Received HANDLE2=00000000000000C0 (should work - no O_CLOEXEC)
# Child: Successfully wrote to HANDLE1
# Child: Successfully wrote to HANDLE2
# Child: HANDLE1 (O_CLOEXEC): ACCESSIBLE (BAD!)
# Child: HANDLE2 (no O_CLOEXEC): ACCESSIBLE (GOOD!)
# Child: Test FAILED - O_CLOEXEC not working correctly
# Parent: Opening test files...
# Parent: fd1=3 (O_CLOEXEC) -> HANDLE=00000000000000B0
# Parent: fd2=4 (no O_CLOEXEC) -> HANDLE=00000000000000C0
# Parent: Spawning child process...
# Parent: Command line:
"C:\\tools\\xmsys64\\home\\pgrunner\\bf\\root\\REL_16_STABLE\\pgsql.build\\tmp_install\\tools\\xmsys64\\home\\pgrunner\\bf\\root\\REL_16_STABLE\\inst\\bin\\test_cloexec.exe"
00000000000000B0 00000000000000C0
# Parent: Waiting for child process...
# Parent: Child exit code: 1
# Parent: FAILURE - O_CLOEXEC not working correctly
[14:36:37.447](0.002s) not ok 1 - O_CLOEXEC prevents handle inheritance
[14:36:37.448](0.001s) #   Failed test 'O_CLOEXEC prevents handle inheritance'
#   at C:/tools/xmsys64/home/pgrunner/bf/root/REL_16_STABLE/pgsql/src/test/modules/test_cloexec/t/001_cloexec.pl line 57.
[14:36:37.449](0.001s) # Looks like you failed 1 test of 1.

I've managed to reproduce it locally with:
--- a/src/test/modules/test_cloexec/t/001_cloexec.pl
+++ b/src/test/modules/test_cloexec/t/001_cloexec.pl
@@ -19,3 +19,3 @@ if (!$PostgreSQL::Test::Utils::windows_os)

-plan tests => 1;
+plan tests => 1000;

@@ -44,2 +44,4 @@ note("Using test program: $test_prog");

+for (my $i = 1; $i <= 1000; $i++)
+{
 my ($stdout, $stderr);
@@ -58,3 +60,3 @@ ok($result && $stdout =~ /SUCCESS.*O_CLOEXEC behavior verified/s,
    "O_CLOEXEC prevents handle inheritance");
-
+}
 done_testing();

and with the test cloned:
perl -i.bak -ne "print unless /test_cloexec_/" "src/test/modules/meson.build" && python3 -c "for i in
range(1,50+1):import os;import
shutil;sd=\"src/test/modules/test_cloexec/\";td=f\"src/test/modules/test_cloexec_{i}\";shutil.rmtree(td,ignore_errors=1);shutil.copytree(sd,
td);assert(os.system(f'perl -i.bak -pe
\"s#(subdir.\'test_cloexec\'.)#subdir(\'test_cloexec_{i}\'){chr(92)}n{chr(92)}1#\"
\"src/test/modules/meson.build\"')==0);assert(os.system(f'perl -i.bak -pe \"s#: \'test_cloexec\',#:
\'test_cloexec_{i}\',#\" \"{td}/meson.build\"')==0)"

meson test test_cloexec_*/001_cloexec --num-processes 50
failed for me as below:
Ok:                19
Fail:              31

# grep 'not ok' meson-logs/testlog.txt
not ok 541 - O_CLOEXEC prevents handle inheritance
not ok 303 - O_CLOEXEC prevents handle inheritance
not ok 798 - O_CLOEXEC prevents handle inheritance
not ok 901 - O_CLOEXEC prevents handle inheritance
not ok 634 - O_CLOEXEC prevents handle inheritance
not ok 640 - O_CLOEXEC prevents handle inheritance
not ok 770 - O_CLOEXEC prevents handle inheritance
not ok 938 - O_CLOEXEC prevents handle inheritance
not ok 420 - O_CLOEXEC prevents handle inheritance
not ok 674 - O_CLOEXEC prevents handle inheritance
not ok 114 - O_CLOEXEC prevents handle inheritance
not ok 649 - O_CLOEXEC prevents handle inheritance
not ok 51 - O_CLOEXEC prevents handle inheritance
not ok 692 - O_CLOEXEC prevents handle inheritance
not ok 874 - O_CLOEXEC prevents handle inheritance
not ok 955 - O_CLOEXEC prevents handle inheritance
not ok 690 - O_CLOEXEC prevents handle inheritance
not ok 33 - O_CLOEXEC prevents handle inheritance
not ok 116 - O_CLOEXEC prevents handle inheritance
not ok 398 - O_CLOEXEC prevents handle inheritance
not ok 773 - O_CLOEXEC prevents handle inheritance
not ok 2 - O_CLOEXEC prevents handle inheritance
not ok 253 - O_CLOEXEC prevents handle inheritance
not ok 10 - O_CLOEXEC prevents handle inheritance
not ok 902 - O_CLOEXEC prevents handle inheritance
not ok 848 - O_CLOEXEC prevents handle inheritance
not ok 832 - O_CLOEXEC prevents handle inheritance
not ok 38 - O_CLOEXEC prevents handle inheritance
not ok 986 - O_CLOEXEC prevents handle inheritance
not ok 688 - O_CLOEXEC prevents handle inheritance
not ok 872 - O_CLOEXEC prevents handle inheritance
not ok 582 - O_CLOEXEC prevents handle inheritance
not ok 816 - O_CLOEXEC prevents handle inheritance
not ok 277 - O_CLOEXEC prevents handle inheritance
not ok 570 - O_CLOEXEC prevents handle inheritance
not ok 691 - O_CLOEXEC prevents handle inheritance
not ok 871 - O_CLOEXEC prevents handle inheritance
not ok 770 - O_CLOEXEC prevents handle inheritance
not ok 108 - O_CLOEXEC prevents handle inheritance
not ok 610 - O_CLOEXEC prevents handle inheritance
not ok 866 - O_CLOEXEC prevents handle inheritance
not ok 909 - O_CLOEXEC prevents handle inheritance
not ok 276 - O_CLOEXEC prevents handle inheritance
not ok 190 - O_CLOEXEC prevents handle inheritance

It looks like HANDLE1 passed from parent to child can accidentally point
to a child slot occupied by some other writable object, not inherited from
parent.

[1] https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=fairywren&dt=2026-06-19%2013%3A28%3A44

Best regards,
Alexander

On 6/20/2026 2:00 AM, Alexander Lakhin wrote:
> Hello Bryan,
>
> 13.12.2025 04:44, Tom Lane wrote:
>> Thomas Munro<thomas(dot)munro(at)gmail(dot)com> writes:
>>> Hearing nothing, pushed.
>> fairywren is unimpressed:
>
> Now that fairywren is pretty satisfied with the test, it has produced an
> interesting failure [1]:
> 208/222 postgresql:test_cloexec /
> test_cloexec/001_cloexec                        ERROR 4.09s   exit status 1
>
> regress_log_001_cloexec
> [14:36:36.820](1.047s) 1..1
> [14:36:36.821](0.001s) # Using test program: C:\\tools\\xmsys64\\home\
> \pgrunner\\bf\\root\\REL_16_STABLE\\pgsql.build\\tmp_install\\tools\
> \xmsys64\\home\\pgrunner\\bf\\root\\REL_16_STABLE\\inst\\bin\
> \test_cloexec.exe
> [14:36:37.445](0.624s) # Test program output:
> [14:36:37.446](0.001s) # Child: Received HANDLE1=00000000000000B0
> (should fail - O_CLOEXEC)
> # Child: Received HANDLE2=00000000000000C0 (should work - no O_CLOEXEC)
> # Child: Successfully wrote to HANDLE1
> # Child: Successfully wrote to HANDLE2
> # Child: HANDLE1 (O_CLOEXEC): ACCESSIBLE (BAD!)
> # Child: HANDLE2 (no O_CLOEXEC): ACCESSIBLE (GOOD!)
> # Child: Test FAILED - O_CLOEXEC not working correctly
> # Parent: Opening test files...
> # Parent: fd1=3 (O_CLOEXEC) -> HANDLE=00000000000000B0
> # Parent: fd2=4 (no O_CLOEXEC) -> HANDLE=00000000000000C0
> # Parent: Spawning child process...
> # Parent: Command line: "C:\\tools\\xmsys64\\home\\pgrunner\\bf\\root\
> \REL_16_STABLE\\pgsql.build\\tmp_install\\tools\\xmsys64\\home\
> \pgrunner\\bf\\root\\REL_16_STABLE\\inst\\bin\\test_cloexec.exe"
> 00000000000000B0 00000000000000C0
> # Parent: Waiting for child process...
> # Parent: Child exit code: 1
> # Parent: FAILURE - O_CLOEXEC not working correctly
> [14:36:37.447](0.002s) not ok 1 - O_CLOEXEC prevents handle inheritance
> [14:36:37.448](0.001s) #   Failed test 'O_CLOEXEC prevents handle
> inheritance'
> #   at C:/tools/xmsys64/home/pgrunner/bf/root/REL_16_STABLE/pgsql/src/
> test/modules/test_cloexec/t/001_cloexec.pl line 57.
> [14:36:37.449](0.001s) # Looks like you failed 1 test of 1.
>
> I've managed to reproduce it locally with:
> --- a/src/test/modules/test_cloexec/t/001_cloexec.pl
> +++ b/src/test/modules/test_cloexec/t/001_cloexec.pl
> @@ -19,3 +19,3 @@ if (!$PostgreSQL::Test::Utils::windows_os)
>
> -plan tests => 1;
> +plan tests => 1000;
>
> @@ -44,2 +44,4 @@ note("Using test program: $test_prog");
>
> +for (my $i = 1; $i <= 1000; $i++)
> +{
>  my ($stdout, $stderr);
> @@ -58,3 +60,3 @@ ok($result && $stdout =~ /SUCCESS.*O_CLOEXEC behavior
> verified/s,
>     "O_CLOEXEC prevents handle inheritance");
> -
> +}
>  done_testing();
>
> and with the test cloned:
> perl -i.bak -ne "print unless /test_cloexec_/" "src/test/modules/
> meson.build" && python3 -c "for i in range(1,50+1):import os;import
> shutil;sd=\"src/test/modules/test_cloexec/\";td=f\"src/test/modules/
> test_cloexec_{i}\";shutil.rmtree(td,ignore_errors=1);shutil.copytree(sd,
> td);assert(os.system(f'perl -i.bak -pe \"s#(subdir.
> \'test_cloexec\'.)#subdir(\'test_cloexec_{i}\'){chr(92)}n{chr(92)}1#\"
> \"src/test/modules/meson.build\"')==0);assert(os.system(f'perl -i.bak -
> pe \"s#: \'test_cloexec\',#: \'test_cloexec_{i}\',#\" \"{td}/
> meson.build\"')==0)"
>
> meson test test_cloexec_*/001_cloexec --num-processes 50
> failed for me as below:
> Ok:                19
> Fail:              31
>
> # grep 'not ok' meson-logs/testlog.txt
> not ok 541 - O_CLOEXEC prevents handle inheritance
> not ok 303 - O_CLOEXEC prevents handle inheritance
> not ok 798 - O_CLOEXEC prevents handle inheritance
> not ok 901 - O_CLOEXEC prevents handle inheritance
> not ok 634 - O_CLOEXEC prevents handle inheritance
> not ok 640 - O_CLOEXEC prevents handle inheritance
> not ok 770 - O_CLOEXEC prevents handle inheritance
> not ok 938 - O_CLOEXEC prevents handle inheritance
> not ok 420 - O_CLOEXEC prevents handle inheritance
> not ok 674 - O_CLOEXEC prevents handle inheritance
> not ok 114 - O_CLOEXEC prevents handle inheritance
> not ok 649 - O_CLOEXEC prevents handle inheritance
> not ok 51 - O_CLOEXEC prevents handle inheritance
> not ok 692 - O_CLOEXEC prevents handle inheritance
> not ok 874 - O_CLOEXEC prevents handle inheritance
> not ok 955 - O_CLOEXEC prevents handle inheritance
> not ok 690 - O_CLOEXEC prevents handle inheritance
> not ok 33 - O_CLOEXEC prevents handle inheritance
> not ok 116 - O_CLOEXEC prevents handle inheritance
> not ok 398 - O_CLOEXEC prevents handle inheritance
> not ok 773 - O_CLOEXEC prevents handle inheritance
> not ok 2 - O_CLOEXEC prevents handle inheritance
> not ok 253 - O_CLOEXEC prevents handle inheritance
> not ok 10 - O_CLOEXEC prevents handle inheritance
> not ok 902 - O_CLOEXEC prevents handle inheritance
> not ok 848 - O_CLOEXEC prevents handle inheritance
> not ok 832 - O_CLOEXEC prevents handle inheritance
> not ok 38 - O_CLOEXEC prevents handle inheritance
> not ok 986 - O_CLOEXEC prevents handle inheritance
> not ok 688 - O_CLOEXEC prevents handle inheritance
> not ok 872 - O_CLOEXEC prevents handle inheritance
> not ok 582 - O_CLOEXEC prevents handle inheritance
> not ok 816 - O_CLOEXEC prevents handle inheritance
> not ok 277 - O_CLOEXEC prevents handle inheritance
> not ok 570 - O_CLOEXEC prevents handle inheritance
> not ok 691 - O_CLOEXEC prevents handle inheritance
> not ok 871 - O_CLOEXEC prevents handle inheritance
> not ok 770 - O_CLOEXEC prevents handle inheritance
> not ok 108 - O_CLOEXEC prevents handle inheritance
> not ok 610 - O_CLOEXEC prevents handle inheritance
> not ok 866 - O_CLOEXEC prevents handle inheritance
> not ok 909 - O_CLOEXEC prevents handle inheritance
> not ok 276 - O_CLOEXEC prevents handle inheritance
> not ok 190 - O_CLOEXEC prevents handle inheritance
>
> It looks like HANDLE1 passed from parent to child can accidentally point
> to a child slot occupied by some other writable object, not inherited from
> parent.
>
> [1] https://buildfarm.postgresql.org/cgi-bin/show_log.pl?
> nm=fairywren&dt=2026-06-19%2013%3A28%3A44
>
> Best regards,
> Alexander

Yeah, the test script has problems. Passing raw numeric handle values
is problematic. When using CreateProcess with bInheritHandles=TRUE, the
kernel duplicates "inheritable" handles into the child at the same
numeric value. So, HANDLE2 works reliably since 0xc0 is he same file
object in both processes.

The non-inherited handle is simply unmanaged in the child. O_CLOEXEC
means that 0xB0 will not be duplicated into the child process. The
problem is that we have no promise that some other kernel object handle
is not at that offset in the child. The child's own startup populates
its handle table and windows reuses low handle values aggressively.

Assuming, the handle was marked as not inheritable in the parent but the
child's handle table has an entry at that same offset for another kernel
object: try_write_to_handle() does a writefile against another kernel
object and succeeds. This is a false failure. This test can't tell the
difference between the actual inherited file and another kernel object
that just happened to have this offset in the handle table assigned to it.

Running in parallel will hit this problem often.

Also, by writing into whatever random kernel object that is referenced
by this offset in the handle table in the child could cause real side
effects when it misfires.

We have a couple of options: 1) use GetHandleInformation for each
handle and test the HANDLE_FLAG_INHERIT bit. This would be a completely
deterministic test and no spawn shenanigans. 2) If we want to keep an
end to end spawn test-- we pass the file paths to the child as well as
the expected handles and use GetFinalPathNameByHandle() to test that the
passed in handles resolve to the correct paths in the child.

The end to end test just seems to be testing that CreateProcess honors
the O_CLOEXEC, which is testing the OS behavior. The
GetHandleInformation(handle1/handle2) check for HANDLE_FLAG_INHERIT is
simpler and would show that the handle has been marked as inheritable or
not. We should trust the kernel to do what it says it does and just
test that our code path for O_CLOEXEC actually creates the handle as
either inheritable or not. This is what GetHandleInformation can tell
us and it removes the need for the spawn.

I am busy with an unfortunate family emergency or I would just code this
as a patch now given agreement on path, but I can get to this by the end
of the week if no one else cares to implement.

--
Bryan Green
EDB: https://www.enterprisedb.com