Horton

Thread

Commits

  1. Restore robustness of TAP tests that wait for postmaster restart.

  1. recovery test failures on hoverfly

    Amit Kapila <amit.kapila16@gmail.com> — 2021-06-11T12:08:34Z

    I noticed that we are getting random failures [1][2][3] in the
recovery test on hoverfly. The failures are in 022_crash_temp_files
and 013_crash_restart. Both the tests failed due to same reason:

ack Broken pipe: write( 13, 'SELECT 1' ) at
/home/nm/src/build/IPC-Run-0.94/lib/IPC/Run/IO.pm line 558.

It seems the error happens in both the tests when after issuing a
KILL, we are trying to reconnect. Can we do anything for this?

[1] - https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=hoverfly&dt=2021-06-11%2006%3A59%3A59
[2] - https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=hoverfly&dt=2021-06-06%2007%3A09%3A53
[3] - https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=hoverfly&dt=2021-06-05%2008%3A40%3A49

-- 
With Regards,
Amit Kapila.

  2. Re: recovery test failures on hoverfly

    Michael Paquier <michael@paquier.xyz> — 2021-06-11T12:20:07Z

    On Fri, Jun 11, 2021 at 05:38:34PM +0530, Amit Kapila wrote:
> It seems the error happens in both the tests when after issuing a
> KILL, we are trying to reconnect. Can we do anything for this?

This is the same problem as c757a3da and 6d41dd0, where we write a
query to a pipe but the kill, causing a failure, makes the test fail
with a SIGPIPE in IPC::Run as a query is sent down to a pipe.

I think that using SELECT 1 to test if the server has been restarted
is a bit crazy.  I would suggest to use instead a loop based on
pg_isready.
--
Michael

  3. Re: recovery test failures on hoverfly

    Tom Lane <tgl@sss.pgh.pa.us> — 2021-06-11T14:37:21Z

    Michael Paquier <michael@paquier.xyz> writes:
> On Fri, Jun 11, 2021 at 05:38:34PM +0530, Amit Kapila wrote:
>> It seems the error happens in both the tests when after issuing a
>> KILL, we are trying to reconnect. Can we do anything for this?

> This is the same problem as c757a3da and 6d41dd0, where we write a
> query to a pipe but the kill, causing a failure, makes the test fail
> with a SIGPIPE in IPC::Run as a query is sent down to a pipe.

Indeed.

> I think that using SELECT 1 to test if the server has been restarted
> is a bit crazy.  I would suggest to use instead a loop based on
> pg_isready.

The precedent of the previous fixes would seem to suggest seeing if
we can replace 'SELECT 1' with "undef".  Not sure if that'll work
without annoying changes to poll_query_until, though.

			regards, tom lane

  4. Re: recovery test failures on hoverfly

    Tom Lane <tgl@sss.pgh.pa.us> — 2021-06-11T22:28:41Z

    I wrote:
> Michael Paquier <michael@paquier.xyz> writes:
>> This is the same problem as c757a3da and 6d41dd0, where we write a
>> query to a pipe but the kill, causing a failure, makes the test fail
>> with a SIGPIPE in IPC::Run as a query is sent down to a pipe.

> The precedent of the previous fixes would seem to suggest seeing if
> we can replace 'SELECT 1' with "undef".  Not sure if that'll work
> without annoying changes to poll_query_until, though.

I noticed that elver failed this same way today, so that got me
annoyed enough to pursue a fix.  Using "undef" as poll_query_until's
input almost works, except it turns out that it fails to notice psql
connection failures in that case!  It is *only* looking at psql's
stdout, not at either stderr or the exit status, which seems seriously
bogus in its own right; not least because poll_query_until's own
documentation claims it will continue waiting after an error, which
is exactly what it's not doing.  So I propose the attached.

(I first tried to make it check $result == 0, but it seems there are a
lot of cases where psql returns status 1 in these tests.  That seems
pretty bogus too, but probably beta is no time to change that
behavior.)

			regards, tom lane

  5. Re: recovery test failures on hoverfly

    Tom Lane <tgl@sss.pgh.pa.us> — 2021-06-12T19:15:11Z

    >> Michael Paquier <michael@paquier.xyz> writes:
>>> This is the same problem as c757a3da and 6d41dd0, where we write a
>>> query to a pipe but the kill, causing a failure, makes the test fail
>>> with a SIGPIPE in IPC::Run as a query is sent down to a pipe.

After checking the git logs, I realized that this failure is actually
new since 11e9caff8: before that, poll_query_until passed the query
on the command line not stdin, so it wasn't vulnerable to SIGPIPE.
So that explains why we only recently started to see this.

The fix I proposed seems to work fine in all branches, so I went
ahead and pushed it.

			regards, tom lane

  6. Re: recovery test failures on hoverfly

    Andrew Dunstan <andrew@dunslane.net> — 2021-06-12T21:19:38Z

    On 6/12/21 3:15 PM, Tom Lane wrote:
>>> Michael Paquier <michael@paquier.xyz> writes:
>>>> This is the same problem as c757a3da and 6d41dd0, where we write a
>>>> query to a pipe but the kill, causing a failure, makes the test fail
>>>> with a SIGPIPE in IPC::Run as a query is sent down to a pipe.
> After checking the git logs, I realized that this failure is actually
> new since 11e9caff8: before that, poll_query_until passed the query
> on the command line not stdin, so it wasn't vulnerable to SIGPIPE.
> So that explains why we only recently started to see this.
>
> The fix I proposed seems to work fine in all branches, so I went
> ahead and pushed it.
>
> 			


I'm a bit dubious about this. It doesn't seem more robust to insist that
we pass undef in certain cases. If passing the SQL via stdin is fragile,
as we also found to be the case with passing it via the command line,
perhaps we should try passing it via a tmp file. Then there would
presumably be no SIGPIPE.


cheers


andrew

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

  7. Re: recovery test failures on hoverfly

    Tom Lane <tgl@sss.pgh.pa.us> — 2021-06-12T21:28:19Z

    Andrew Dunstan <andrew@dunslane.net> writes:
> I'm a bit dubious about this. It doesn't seem more robust to insist that
> we pass undef in certain cases.

True, it'd be nicer if that didn't matter; mainly because people
will get it wrong in future.

> If passing the SQL via stdin is fragile,
> as we also found to be the case with passing it via the command line,
> perhaps we should try passing it via a tmp file. Then there would
> presumably be no SIGPIPE.

Seems kind of inefficient.  Maybe writing and reading a file would
be a negligible cost compared to everything else involved, but
I'm not sure.

Another angle is that the SIGPIPE complaints aren't necessarily
a bad thing: if psql doesn't read what we send, it's good to
know about that.  IMO the real problem is that the errors are
so darn nonrepeatable.  I wonder if there is a way to make them
more reproducible?

			regards, tom lane

  8. Re: recovery test failures on hoverfly

    Andrew Dunstan <andrew@dunslane.net> — 2021-06-12T21:50:46Z

    On 6/12/21 5:28 PM, Tom Lane wrote:
> Andrew Dunstan <andrew@dunslane.net> writes:
>> I'm a bit dubious about this. It doesn't seem more robust to insist that
>> we pass undef in certain cases.
> True, it'd be nicer if that didn't matter; mainly because people
> will get it wrong in future.


Right, that's what I'm worried about.


>
>> If passing the SQL via stdin is fragile,
>> as we also found to be the case with passing it via the command line,
>> perhaps we should try passing it via a tmp file. Then there would
>> presumably be no SIGPIPE.
> Seems kind of inefficient.  Maybe writing and reading a file would
> be a negligible cost compared to everything else involved, but
> I'm not sure.


Well, in poll_query_until we would of course set up the file outside the
loop. I suspect the cost would in fact be negligible.


Note, too that the psql and safe_psql methods also pass the query via stdin.


>
> Another angle is that the SIGPIPE complaints aren't necessarily
> a bad thing: if psql doesn't read what we send, it's good to
> know about that.  IMO the real problem is that the errors are
> so darn nonrepeatable.  I wonder if there is a way to make them
> more reproducible?
>
> 			


I don't know.


cheers


andrew


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

  9. Re: recovery test failures on hoverfly

    Tom Lane <tgl@sss.pgh.pa.us> — 2021-06-12T21:57:48Z

    Andrew Dunstan <andrew@dunslane.net> writes:
> Note, too that the psql and safe_psql methods also pass the query via stdin.

Yeah.  We need all of these to act the same, IMO.  Recall that
the previous patches that introduced the undef hack were changing
callers of those routines, not poll_query_until.

			regards, tom lane