Thread: Re: BackgroundPsql swallowing errors on windows
> 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.
> 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.
I think it could be worth it, +1 for keeping it until it's beeb proven a
problem somewhere.
> This was a rather painful exercise.
I have no trouble believing that.
--
Daniel Gustafsson
On Fri, Feb 14, 2025 at 8:53 AM Andres Freund <andres@anarazel.de> wrote: > 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? I think both should be backpatchable without too much risk, though it's possible that there are more useless ok() calls in back branches that would need to be touched when the first patch goes back. If we're concerned about the second for any reason, the only conflicting part should be the name and documentation of wait_connect, right? --Jacob
Hi, On 2025-02-14 09:52:24 -0800, Jacob Champion wrote: > On Fri, Feb 14, 2025 at 8:53 AM Andres Freund <andres@anarazel.de> wrote: > > 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? > > I think both should be backpatchable without too much risk I think so too. Obviously it'll have to wait until later next week due to the new minor releases, but after that I think we should backpatch them. > though it's possible that there are more useless ok() calls in back branches > that would need to be touched when the first patch goes back. FWIW, I don't really agree that such ok() calls are useless, they do make the output of the test more readable, particularly if the test ends up hanging. But that's really just an aside. > If we're concerned about the second for any reason, the only conflicting > part should be the name and documentation of wait_connect, right? It doesn't seem concerning to me either. The first commit seems much more likely to cause trouble and even that seems ok. Even if it were to cause problem for an extension (which I think is rather unlikely), it shouldn't be too hard to fix. Greetings, Andres Freund
Hi,
On 2025-02-17 08:52:58 +0900, Michael Paquier wrote:
> On Sat, Feb 15, 2025 at 04:13:37PM -0500, Andres Freund wrote:
> > On 2025-02-14 09:52:24 -0800, Jacob Champion wrote:
> >> On Fri, Feb 14, 2025 at 8:53 AM Andres Freund <andres@anarazel.de> wrote:
> >>> commit 70291a3c66e
>
> (Side question entirely unrelated as I'm reading that..)
> What's your magic recipe for showing up with commit format? The best
> thing I could come up with was to use "(14,trunc)%H" in format.pretty,
> but it has the idea of showing two dots at the end of the commit ID.
git {show|log|...} --abbrev-commit
> >> If we're concerned about the second for any reason, the only conflicting
> >> part should be the name and documentation of wait_connect, right?
> >
> > It doesn't seem concerning to me either. The first commit seems much more
> > likely to cause trouble and even that seems ok. Even if it were to cause
> > problem for an extension (which I think is rather unlikely), it shouldn't be
> > too hard to fix.
>
> FWIW, Debian Search reports that the only references to BackgroundPsql
> are in the Postgres tree, so backpatching 70291a3c66e does not worry
> me. Github has more much references due to forked code or direct
> copies of BackgroundPsql.pn modified for the purpose of the code.
Cool, will after the minor release freeze.
Greetings,
Andres Freund
Hi,
On 2025-02-17 09:24:24 +0900, Michael Paquier wrote:
> On Sun, Feb 16, 2025 at 07:03:39PM -0500, Andres Freund wrote:
> > Cool, will after the minor release freeze.
>
> Thanks, Andres. If you'd prefer that I double-check the code and do
> it as the former committer of these two ones, please let me know.
Thanks, but I think it's ok, it looks unproblematic enough. I've pushed the
backports (but not yet the new fix), after running them through CI [1]. Let's
hope the buildfarm thinks similarly.
I'm planning to wait for a few hours and see what the BF says and then push
the fix this thread is about to all supported branches.
Greetings,
Andres Freund
[1] I have WIP patches for CI support for 13, 14, with some work for windows
left to do. I plan to write an email about that at some point.
Hi, On 2025-02-19 10:41:53 -0500, Andres Freund wrote: > I'm planning to wait for a few hours and see what the BF says and then push > the fix this thread is about to all supported branches. Not having seen any issues, I pushed the fix. Thanks for the reviews etc! Greetings, Andres Freund