[PATCH] Fix TAP tests with recent IPC::Run on Windows

Hi,

On Fri, 25 Sept 2026 at 11:59, Andrew Bille <andrewbille@gmail.com> wrote:

While running the TAP test suite on Windows with IPC::Run 20260402.0,
I found a number of failures caused by changes in IPC::Run's Win32
stream handling.
There were two distinct symptoms.
First, IPC::Run now uses binary mode by default on Windows. Text
captured into Perl scalars therefore retains CRLF, while a number of
TAP tests expect the previous text-mode behavior. This caused many
comparisons to fail even though the displayed output looked identical.
The patch adds ipc_run_text_mode() and uses it for IPC::Run redirects
whose contents are treated as text by the tests.
Second, a few tests relied on implicit standard streams. With the
recent IPC::Run this resulted in failures such as:

psql: error: could not print result table: Bad file descriptor

and, in pg_upgrade tests, child xcopy commands failing to copy
pg_xact unless stdin was supplied explicitly.
Those cases now use explicit stream endpoints.
I tested the patch with the full test suite on Windows using both:
IPC::Run 20260402.0
IPC::Run 20231003.0
Both pass. I also tested the changes on Linux successfully.

I haven't looked at the patches yet but +1 to fixing these problems.

I saw same problems on the macOS [1]/messages/by-id/CAN55FZ3VOUSO2ZmBUE9wRNnL9fUQfGv5qWefk_1pWYqgJ1bRSQ@mail.gmail.com and MinGW CI [2]/messages/by-id/CAN55FZ06xanSbJdHe-CurjX_qNuBWZDEvS1kAk36L38YCtZXnw@mail.gmail.com task when we
were using Cirrus. These discussions might help.

[1]: /messages/by-id/CAN55FZ3VOUSO2ZmBUE9wRNnL9fUQfGv5qWefk_1pWYqgJ1bRSQ@mail.gmail.com
[2]: /messages/by-id/CAN55FZ06xanSbJdHe-CurjX_qNuBWZDEvS1kAk36L38YCtZXnw@mail.gmail.com

--
Regards,
Nazir Bilal Yavuz
Microsoft

Hi Nazir,

Thanks, these are very useful references.

The MinGW case in [2] looks particularly close to what I reproduced.
The failures started exactly after IPC::Run was updated from
20250809.0 to 20260322.0, and installing the older version made the CI
pass again. That seems to be the same compatibility change that
triggered the failures I saw.

The macOS case in [1] may be a somewhat different issue despite the
similar "Bad file descriptor" symptom. As far as I understand, those
failures went through the IO::Pty path in BackgroundPsql.pm, while this
patch deliberately leaves the PTY path unchanged and changes only the
non-PTY IPC::Run redirections.

I also found two older reports which seem relevant.

BUG #19541 from June reports 154 test failures on Windows. One of the
shown failures passes an LSN containing an unexpected line ending back
to psql, which looks very similar to the CRLF/text-mode failures fixed
by this patch:

/messages/by-id/19541-059eb8da45044c7c@postgresql.org

There is also an older report about pg_upgrade on Windows where xcopy
silently failed to copy pg_xact when stdout was redirected but stdin
was not. Redirecting stdin reportedly fixed it:

/messages/by-id/152112210959.1217.7911406465826869349@wrigleys.postgresql.org

That is particularly interesting because it matches what I observed in
the pg_upgrade tests: pg_upgrade ran the xcopy command, but the target
pg_xact did not contain the expected file. Providing an explicit stdin
endpoint made the test pass.

So at least the MinGW failures and the pg_upgrade stdin behavior seem
to have been observed independently before.

Thanks,
Andrew, Postgres Professional

On Fri, Sep 25, 2026 at 12:11:19PM +0300, Nazir Bilal Yavuz wrote:

I haven't looked at the patches yet but +1 to fixing these problems.

I saw same problems on the macOS [1] and MinGW CI [2] task when we
were using Cirrus. These discussions might help.

Looking at the patch, I cannot get on board with the idea that anybody
adding a test would need to add a new ipc_run_text_mode() call to make
sure that the redirection is able to work correctly. Couldn't there
be a more central solution so as we limit the footprint of this logic
in some way? This kind of solution is just going to create noise in
the long-term, especially when it comes to the buildfarm (aka I doubt
that we should spend cycles remembering that this is required)..
--
Michael

On 2026-09-27 Su 11:34 PM, Michael Paquier wrote:

On Fri, Sep 25, 2026 at 12:11:19PM +0300, Nazir Bilal Yavuz wrote:

I haven't looked at the patches yet but +1 to fixing these problems.

I saw same problems on the macOS [1] and MinGW CI [2] task when we
were using Cirrus. These discussions might help.

Looking at the patch, I cannot get on board with the idea that anybody
adding a test would need to add a new ipc_run_text_mode() call to make
sure that the redirection is able to work correctly. Couldn't there
be a more central solution so as we limit the footprint of this logic
in some way? This kind of solution is just going to create noise in
the long-term, especially when it comes to the buildfarm (aka I doubt
that we should spend cycles remembering that this is required)..

I think the right way is to have a wrapper function as in the attached,
so instead of calling IPC::Run::run you would call ipc_run ...

Note that this patch only touches the sites that will actually be
affected by this. If we wanted to be consistent we would replace calls
in a further 23 files.

cheers

andrew

--
Andrew Dunstan
EDB: https://www.enterprisedb.com

On Mon, Sep 28, 2026 at 12:54:18PM -0400, Andrew Dunstan wrote:

I think the right way is to have a wrapper function as in the attached, so
instead of calling IPC::Run::run you would call ipc_run ...

Note that this patch only touches the sites that will actually be affected
by this. If we wanted to be consistent we would replace calls in a further
23 files.

That seems much better in the long-run, thanks. The
ipc_run_text_mode() being published in Utils.pm does not seem
necessary, though. This is a part that I'm sure could become
confusing if we allow code outside of Utils.pm to use it.
--
Michael

Hi,

On Tue, 29 Sept 2026 at 02:24, Michael Paquier <michael@paquier.xyz> wrote:

On Mon, Sep 28, 2026 at 12:54:18PM -0400, Andrew Dunstan wrote:

I think the right way is to have a wrapper function as in the attached, so
instead of calling IPC::Run::run you would call ipc_run ...

Note that this patch only touches the sites that will actually be affected
by this. If we wanted to be consistent we would replace calls in a further
23 files.

That seems much better in the long-run, thanks. The
ipc_run_text_mode() being published in Utils.pm does not seem
necessary, though. This is a part that I'm sure could become
confusing if we allow code outside of Utils.pm to use it.

I tested Andrew Dunstan's patch in CI after removing the IPC::Run
version pin from the Windows tasks. The 035_standby_logical_decoding
test failed on both the Windows VS and MinGW tasks [1]https://github.com/nbyavuz/postgres/actions/runs/36537214540:

```
stderr:
# die: IPC::Run: timeout on timer #17 at
C:/Strawberry/perl/site/lib/IPC/Run.pm line 3361.
```

Using ipc_start() in the 035_standby_logical_decoding test fixes the
failure, and CI now passes [2]https://github.com/nbyavuz/postgres/actions/runs/36537155885.

Here are three patches:
- 0001 is Andrew Dunstan's patch, unchanged.
- 0002 is a fixup for the 035_standby_logical_decoding test.
- 0003 removes the IPC::Run version pin from the Windows tasks.

[1]: https://github.com/nbyavuz/postgres/actions/runs/36537214540
[2]: https://github.com/nbyavuz/postgres/actions/runs/36537155885

--
Regards,
Nazir Bilal Yavuz
Microsoft

On 2026-09-29 Tu 4:20 AM, Nazir Bilal Yavuz wrote:

Hi,

On Tue, 29 Sept 2026 at 02:24, Michael Paquier <michael@paquier.xyz> wrote:

On Mon, Sep 28, 2026 at 12:54:18PM -0400, Andrew Dunstan wrote:

I think the right way is to have a wrapper function as in the attached, so
instead of calling IPC::Run::run you would call ipc_run ...

Note that this patch only touches the sites that will actually be affected
by this. If we wanted to be consistent we would replace calls in a further
23 files.

That seems much better in the long-run, thanks. The
ipc_run_text_mode() being published in Utils.pm does not seem
necessary, though. This is a part that I'm sure could become
confusing if we allow code outside of Utils.pm to use it.

I tested Andrew Dunstan's patch in CI after removing the IPC::Run
version pin from the Windows tasks. The 035_standby_logical_decoding
test failed on both the Windows VS and MinGW tasks [1]:

```
stderr:
# die: IPC::Run: timeout on timer #17 at
C:/Strawberry/perl/site/lib/IPC/Run.pm line 3361.
```

Using ipc_start() in the 035_standby_logical_decoding test fixes the
failure, and CI now passes [2].

Here are three patches:
- 0001 is Andrew Dunstan's patch, unchanged.
- 0002 is a fixup for the 035_standby_logical_decoding test.
- 0003 removes the IPC::Run version pin from the Windows tasks.

[1] https://github.com/nbyavuz/postgres/actions/runs/36537214540
[2] https://github.com/nbyavuz/postgres/actions/runs/36537155885

Great, thanks for testing. I agree with Michael. In fact I think we
could just get rid of ipc_run_text_mode altogether and just put the
binary call inline in the only place it's used.

cheers

andrew

--
Andrew Dunstan
EDB: https://www.enterprisedb.com