Date: Sun, 16 Feb 2025 09:39:43 -0800
From: Noah Misch <noah@leadboat.com>
To: Andres Freund <andres@anarazel.de>
Cc: pgsql-hackers@postgresql.org
Subject: Re: BackgroundPsql swallowing errors on windows
Message-ID: <20250216173943.b6.nmisch@google.com>
References: <wmovm6xcbwh7twdtymxuboaoarbvwj2haasd3sikzlb3dkgz76@n45rzycluzft>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <wmovm6xcbwh7twdtymxuboaoarbvwj2haasd3sikzlb3dkgz76@n45rzycluzft>
User-Agent: Mutt/2.2.12 (2023-09-09)
Archived-At: 
 <https://www.postgresql.org/message-id/20250216173943.b6.nmisch%40google.com>
Precedence: bulk

On Thu, Feb 13, 2025 at 12:39:04PM -0500, Andres Freund wrote:
> My understanding is that IPC::Run uses a proxy process on windows to execute
> subprocesses and then communicates with that over TCP (or something along
> those lines).

Right.

> I suspect what's happening is that the communication with the
> external process allows for reordering between stdout/stderr.
> 
> And indeed, changing BackgroundPsql::query() to emit the banner on both stdout
> and stderr and waiting on both seems to fix the issue.

That makes sense.  I wondered how one might fix IPC::Run to preserve the
relative timing of stdout and stderr, not perturbing the timing the way that
disrupted your test run.  I can think of two strategies:

- Remove the proxy.

- Make pipe data visible to Perl variables only when at least one of the
  proxy<-program_under_test pipes had no data ready to read.  In other words,
  if both pipes have data ready, make all that data visible to Perl code
  simultaneously.  (When both the stdout pipe and the stderr pipe have data
  ready, one can't determine data arrival order.)

Is there a possibly-less-invasive change that might work?

> The banner being the same between queries made it hard to understand if a
> banner that appeared in the output was from the current query or a past
> query. Therefore I added a counter to it.

Sounds good.

> For debugging I added a "note" that shows stdout/stderr after executing the
> query, I think it may be worth keeping that, but I'm not sure.

It should be okay to keep.  We're not likely to funnel huge amounts of data
through BackgroundPsql.  If we ever do that, we could just skip the "note" for
payloads larger than some threshold.


v2 of the patch looks fine.