Re: BackgroundPsql swallowing errors on windows

Hi,

On 2025-02-14 13:35:40 +0100, Daniel Gustafsson wrote:
> > On 13 Feb 2025, at 18:39, Andres Freund <andres(at)anarazel(dot)de> wrote:
>
> > 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.
>
> + my $banner = "background_psql: QUERY_SEPARATOR $query_cnt";
> + my $banner_match = qr/(^|\n)$banner\r?\n/;
> + $self->{stdin} .= "$query\n;\n\\echo $banner\n\\warn $banner\n";
> + pump_until(
> + $self->{run}, $self->{timeout},
> + \$self->{stdout}, qr/$banner_match/);
>
> Won't this allow "QUERY_SEPARATOR 11" to match against "QUERY_SEPARATOR 1"?
> It's probably only of academic interest but appending an end-of-banner
> character like "_" or something after the query counter should fix that.

You're right. I went with ":".

Thanks for reviewing!

Updated patch attached.

I also applied similar changes to wait_connect(), it had the same issues as
query(). This mostly matters for interactive_psql() uses. The fact that we
matched on the \echo itself, lead to the first query() having additional query
output, along the lines of

\echo background_psql: ready
psql (18devel)
Type "help" for help.

postgres=# \echo background_psql: ready
background_psql: ready

Which is rather confusing and can throw off checks based on the query results.

It does seem rather weird that psql outputs its input twice in this case, but
that's a separate issue.

I was thinking that this really ought to be backported, debugging failures due
to the set of fied bugs is really painful (just cost me 1 1/2 days), but
unfortunately there have been a bunch of recent changes that haven't been
backpatched:

commit 70291a3c66e
Author: Michael Paquier <michael(at)paquier(dot)xyz>
Date: 2024-11-07 12:11:27 +0900

Improve handling of empty query results in BackgroundPsql::query()

commit ba08edb0654
Author: Michael Paquier <michael(at)paquier(dot)xyz>
Date: 2024-11-06 15:31:14 +0900

Extend Cluster.pm's background_psql() to be able to start asynchronously

Particularly the former makes it hard to backpatch, as it's a behavioural
difference that really interacts with the problems described in this thread.

Michael, Jacob, thoughts?

Greetings,

Andres Freund