Horton

Re: BackgroundPsql swallowing errors on windows

Andres Freund <andres@anarazel.de>

View thread
From: Andres Freund <andres@anarazel.de>
To: Daniel Gustafsson <daniel@yesql.se>, Michael Paquier <michael.paquier@gmail.com>, Heikki Linnakangas <heikki.linnakangas@iki.fi>, Jacob Champion <jacob.champion@enterprisedb.com>
Cc: PostgreSQL Hackers <pgsql-hackers@postgresql.org>, Noah Misch <noah@leadboat.com>
Date: 2025-02-14T16:53:24Z
Lists: pgsql-hackers

Commits

Same data as JSON: GET /api/v1/messages/:b64id/commits the thread's linked commits as JSON, with link sources. API reference →

  1. tests: BackgroundPsql: Fix potential for lost errors on windows

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

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

Attachments

Hi,

On 2025-02-14 13:35:40 +0100, Daniel Gustafsson wrote:
> > On 13 Feb 2025, at 18:39, Andres Freund <andres@anarazel.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@paquier.xyz>
Date:   2024-11-07 12:11:27 +0900

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

commit ba08edb0654
Author: Michael Paquier <michael@paquier.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