Horton

Thread

Commits

  1. Close psql processes gracefully in recovery tests

  2. Fix recovery test hang in 021_row_visibility.pl on windows.

  1. buildfarm windows checks / tap tests on windows

    Andres Freund <andres@anarazel.de> — 2021-03-01T20:07:15Z

    Hi,

As part of trying to make the aio branch tests on cirrus CI pass with
some tap tests I noticed that "src/tools/msvc/vcregress.pl recoverycheck"
hangs. A long phase of remote debugging later I figured out that that's
not a fault of the aio branch - it also doesn't pass on master (fixed in
[1]).

Which confused me - shouldn't the buildfarm have noticed? But it seems
that all msvc animals either don't have tap tests enabled or they
disable 'misc-checks' which in turn is what the buildfarm client uses to
trigger all the 'recovery' checks.

It seems we're not just skipping recovery, but also e.g. tap tests in
contrib, all the tests in src/test/modules/...

Andrew, what's the reason for that? Is it just that they hung at some
point? Were too slow?


On that last point: Running the tap tests on windows appears to be
*excruciatingly* slow. How does anybody develop on windows without a
mechanism to actually run tests in parallel?

I think it'd be good if vcregress.pl at least respected PROVE_FLAGS from
the environment - it can't currently be passed in for several of the
vcregress.pl tests, and it does seem to make things to be at least
somewhat less painful.


This makes it even clearer to me that we really need a builtin
testrunner that runs tests efficiently *and* debuggable on windows.

Greetings,

Andres Freund

[1] https://git.postgresql.org/gitweb/?p=postgresql.git;a=commit;h=1e6e40447115ca7b4749d7d117b81b016ee5e2c2

  2. Re: buildfarm windows checks / tap tests on windows

    Andrew Dunstan <andrew@dunslane.net> — 2021-03-02T12:48:28Z

    On 3/1/21 3:07 PM, Andres Freund wrote:
> Hi,
>
> As part of trying to make the aio branch tests on cirrus CI pass with
> some tap tests I noticed that "src/tools/msvc/vcregress.pl recoverycheck"
> hangs. A long phase of remote debugging later I figured out that that's
> not a fault of the aio branch - it also doesn't pass on master (fixed in
> [1]).
>
> Which confused me - shouldn't the buildfarm have noticed? But it seems
> that all msvc animals either don't have tap tests enabled or they
> disable 'misc-checks' which in turn is what the buildfarm client uses to
> trigger all the 'recovery' checks.
>
> It seems we're not just skipping recovery, but also e.g. tap tests in
> contrib, all the tests in src/test/modules/...
>
> Andrew, what's the reason for that? Is it just that they hung at some
> point? Were too slow?



I don't think speed is the issue. I probably disabled misc-tests on
drongo and bowerbird (my two animals in question) because I got  either
instability or errors I was unable to diagnose. I'll go back and take
another look to narrow this down. It's possible to disable individual tests.



>
>
> On that last point: Running the tap tests on windows appears to be
> *excruciatingly* slow. How does anybody develop on windows without a
> mechanism to actually run tests in parallel?


I think most people develop elsewhere and then adapt/test on Windows if
necessary.


>
> I think it'd be good if vcregress.pl at least respected PROVE_FLAGS from
> the environment - it can't currently be passed in for several of the
> vcregress.pl tests, and it does seem to make things to be at least
> somewhat less painful.



+1


>
>
> This makes it even clearer to me that we really need a builtin
> testrunner that runs tests efficiently *and* debuggable on windows.
>

"show me the code" :-)


cheers


andrew


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

  3. Re: buildfarm windows checks / tap tests on windows

    Andres Freund <andres@anarazel.de> — 2021-03-02T20:57:57Z

    Hi,

On 2021-03-02 07:48:28 -0500, Andrew Dunstan wrote:
> I don't think speed is the issue. I probably disabled misc-tests on
> drongo and bowerbird (my two animals in question) because I got  either
> instability or errors I was unable to diagnose. I'll go back and take
> another look to narrow this down. It's possible to disable individual tests.

Yea, there was one test that hung in 021_row_visibility.pl which was a
weird perl hang - with weird symptoms. But that should be fixed now.

There's another failure in recoverycheck that I at first thought was the
fault of the aio branch. But it also see it on master - but only on one
of the two machines I use to test. Pretty odd.

t/003_recovery_targets.pl ............ 7/9
#   Failed test 'multiple conflicting settings'
#   at t/003_recovery_targets.pl line 151.

#   Failed test 'recovery end before target reached is a fatal error'
#   at t/003_recovery_targets.pl line 177.
t/003_recovery_targets.pl ............ 9/9 # Looks like you failed 2 tests of 9.
t/003_recovery_targets.pl ............ Dubious, test returned 2 (wstat 512, 0x200)
Failed 2/9 subtests

I think it's pretty dangerous if we have a substantial number of tests
that aren't run on windows - I think a lot of us just assume that the
BF would catch windows specific problems...


> > On that last point: Running the tap tests on windows appears to be
> > *excruciatingly* slow. How does anybody develop on windows without a
> > mechanism to actually run tests in parallel?
> 
> 
> I think most people develop elsewhere and then adapt/test on Windows if
> necessary.

Yea, but even that overstretches my patience by a good bit. I can't deal
with a serial check-world on linux either - I think that's the biggest
differentiator. It's a lot less painful to deal with slow-ish tests if
they do all their slowness concurrently. But that's basically impossible
with vcregress.pl, and the bf etc can't easily do it either until we
have a decent way to see the correct logfiles & output for individual
tests...


> > I think it'd be good if vcregress.pl at least respected PROVE_FLAGS from
> > the environment - it can't currently be passed in for several of the
> > vcregress.pl tests, and it does seem to make things to be at least
> > somewhat less painful.
> +1

K, will send a patch for that in a bit.


> > This makes it even clearer to me that we really need a builtin
> > testrunner that runs tests efficiently *and* debuggable on windows.
> >
> 
> "show me the code" :-)

The biggest obstacle on that front is perl. I started to write one, but
hit several perl issues within an hour. I think I might write one in
python, that'd be a lot less painful.


One windows build question I have is why the msvc infrastructure doesn't
accept msys perl in places like this:
		guid                  => $^O eq "MSWin32" ? Win32::GuidGen() : 'FAKE',
If I change them to accept msys perl then the build ends up working.

The reason it'd be nice to accept msys perl is that git for windows
bundles that - and that's already installed on most CI projects...

Greetings,

Andres Freund

  4. Re: buildfarm windows checks / tap tests on windows

    Andrew Dunstan <andrew@dunslane.net> — 2021-03-02T22:04:16Z

    On 3/2/21 7:48 AM, Andrew Dunstan wrote:
> On 3/1/21 3:07 PM, Andres Freund wrote:
>> Hi,
>>
>> As part of trying to make the aio branch tests on cirrus CI pass with
>> some tap tests I noticed that "src/tools/msvc/vcregress.pl recoverycheck"
>> hangs. A long phase of remote debugging later I figured out that that's
>> not a fault of the aio branch - it also doesn't pass on master (fixed in
>> [1]).
>>
>> Which confused me - shouldn't the buildfarm have noticed? But it seems
>> that all msvc animals either don't have tap tests enabled or they
>> disable 'misc-checks' which in turn is what the buildfarm client uses to
>> trigger all the 'recovery' checks.
>>
>> It seems we're not just skipping recovery, but also e.g. tap tests in
>> contrib, all the tests in src/test/modules/...
>>
>> Andrew, what's the reason for that? Is it just that they hung at some
>> point? Were too slow?
>
>
> I don't think speed is the issue. I probably disabled misc-tests on
> drongo and bowerbird (my two animals in question) because I got  either
> instability or errors I was unable to diagnose. I'll go back and take
> another look to narrow this down. It's possible to disable individual tests.



Well, I though it was, but it wasn't. Fixed now, see
<https://github.com/PGBuildFarm/client-code/commit/377e9129a08d607bf7aa32a42bcf6ebecb92ba4d>


I've deployed this on drongo, which will now run almost all the TAP
tests. The exception is the recovery tests, because
021_row_visibility.pl crashes so badly it brings down the whole
buildfarm client.


cheers


andrew


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

  5. Re: buildfarm windows checks / tap tests on windows

    Andres Freund <andres@anarazel.de> — 2021-03-03T00:54:57Z

    Hi,

On 2021-03-02 17:04:16 -0500, Andrew Dunstan wrote:
> Well, I though it was, but it wasn't. Fixed now, see
> <https://github.com/PGBuildFarm/client-code/commit/377e9129a08d607bf7aa32a42bcf6ebecb92ba4d>
> 
> 
> I've deployed this on drongo, which will now run almost all the TAP
> tests.

Thanks!


> The exception is the recovery tests, because
> 021_row_visibility.pl crashes so badly it brings down the whole
> buildfarm client.

It still does, even after

commit 1e6e40447115ca7b4749d7d117b81b016ee5e2c2 (upstream/master, master)
Author: Andres Freund <andres@anarazel.de>
Date:   2021-03-01 09:52:15 -0800

    Fix recovery test hang in 021_row_visibility.pl on windows.

? I didn't see failures after that?

Greetings,

Andres Freund

  6. Re: buildfarm windows checks / tap tests on windows

    Michael Paquier <michael@paquier.xyz> — 2021-03-03T01:27:57Z

    On Tue, Mar 02, 2021 at 04:54:57PM -0800, Andres Freund wrote:
> It still does, even after
> 
> commit 1e6e40447115ca7b4749d7d117b81b016ee5e2c2 (upstream/master, master)
> Author: Andres Freund <andres@anarazel.de>
> Date:   2021-03-01 09:52:15 -0800
> 
>     Fix recovery test hang in 021_row_visibility.pl on windows.
> 
> ? I didn't see failures after that?

Yes.  Testing this morning on top of 5b2f2af, it fails for me with a
"Terminating on signal SIGBREAK".

Having a support for PROVE_TESTS would be nice in src/tools/msvc/,
wrapping any ENV{PROVE_TESTS} value within an extra glob() before
passing that down to the prove command.
--
Michael

  7. Re: buildfarm windows checks / tap tests on windows

    Andrew Dunstan <andrew@dunslane.net> — 2021-03-03T02:20:52Z

    On 3/2/21 8:27 PM, Michael Paquier wrote:
> On Tue, Mar 02, 2021 at 04:54:57PM -0800, Andres Freund wrote:
>> It still does, even after
>>
>> commit 1e6e40447115ca7b4749d7d117b81b016ee5e2c2 (upstream/master, master)
>> Author: Andres Freund <andres@anarazel.de>
>> Date:   2021-03-01 09:52:15 -0800
>>
>>     Fix recovery test hang in 021_row_visibility.pl on windows.
>>
>> ? I didn't see failures after that?
> Yes.  Testing this morning on top of 5b2f2af, it fails for me with a
> "Terminating on signal SIGBREAK".
>
> Having a support for PROVE_TESTS would be nice in src/tools/msvc/,
> wrapping any ENV{PROVE_TESTS} value within an extra glob() before
> passing that down to the prove command.



Yes, I saw similar this morning, which woud have been after that commit.


cheers


andrew

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

  8. Re: buildfarm windows checks / tap tests on windows

    Andres Freund <andres@anarazel.de> — 2021-03-03T05:20:11Z

    Hi,

On 2021-03-02 12:57:57 -0800, Andres Freund wrote:
> t/003_recovery_targets.pl ............ 7/9
> #   Failed test 'multiple conflicting settings'
> #   at t/003_recovery_targets.pl line 151.
> 
> #   Failed test 'recovery end before target reached is a fatal error'
> #   at t/003_recovery_targets.pl line 177.
> t/003_recovery_targets.pl ............ 9/9 # Looks like you failed 2 tests of 9.
> t/003_recovery_targets.pl ............ Dubious, test returned 2 (wstat 512, 0x200)
> Failed 2/9 subtests

This appears to be caused by stderr in windows docker containers to
somehow not work quite right. cirrus-ci uses docker on windows.

If you look e.g. at https://cirrus-ci.com/task/6111560255930368, and
specifically at the relevant log file:
https://api.cirrus-ci.com/v1/artifact/task/6111560255930368/log/src/test/recovery/tmp_check/log/003_recovery_targets_primary.log
you can see that it's, uh, less full than we normally expect:
        1 file(s) copied.
        1 file(s) copied.
        1 file(s) copied.
        1 file(s) copied.

As that test uses the log file to determine the state of servers:
> my $logfile = slurp_file($node_standby->logfile());
> ok($logfile =~ qr/multiple recovery targets specified/,
>         'multiple conflicting settings');

that doesn't work.


I was *very* confused by this for a while. But finally the cluebait hit
when I discovered that stderr works just fine for *other*
programs. Including the programs that evidently log into
003_recovery_targets_primary.log.  The problem is that
pgwin32_is_service() somehow decides that postgres is running as a
service. Despite that not really being the case (I guess somehow
internally docker containers are started below a service, and that
causes the problem).

I hate everything right now. So much.

I think it's quite nasty that postgres just silently starts to log to
the event log. Why on earth wasn't the solution instead to hardcode that
as a server parameter in pg_ctl register?

Not sure what a good fix is for this.



The second problem I saw was 001_initdb failing, which appears to have
been caused by some weird permission issue that I don't fully
understand. The directory with PG in it was created by user andres, an
administrator. But somehow the inherited permissions lead to the chmod()
that initdb does ("fixing permissions on existing directory %s ...")  to
fail.

c:\src\postgres>icacls c:\src\postgres
c:\src\postgres BUILTIN\Administrators:(F)
                BUILTIN\Administrators:(I)(OI)(CI)(F)
                NT AUTHORITY\SYSTEM:(I)(OI)(CI)(F)
                CREATOR OWNER:(I)(OI)(CI)(IO)(F)
                BUILTIN\Users:(I)(OI)(CI)(RX)
                BUILTIN\Users:(I)(CI)(AD)
                BUILTIN\Users:(I)(CI)(WD)
c:\src\postgres>whoami
andres-build-te\andres

c:\src\postgres>net user andres
User name                    andres
...
Local Group Memberships      *Administrators       *Users


Greetings,

Andres Freund

  9. Re: buildfarm windows checks / tap tests on windows

    Andres Freund <andres@anarazel.de> — 2021-03-03T05:47:18Z

    Hi,

On 2021-03-02 21:20:52 -0500, Andrew Dunstan wrote:
> On 3/2/21 8:27 PM, Michael Paquier wrote:
> > On Tue, Mar 02, 2021 at 04:54:57PM -0800, Andres Freund wrote:
> >> It still does, even after
> >>
> >> commit 1e6e40447115ca7b4749d7d117b81b016ee5e2c2 (upstream/master, master)
> >> Author: Andres Freund <andres@anarazel.de>
> >> Date:   2021-03-01 09:52:15 -0800
> >>
> >>     Fix recovery test hang in 021_row_visibility.pl on windows.
> >>
> >> ? I didn't see failures after that?
> > Yes.  Testing this morning on top of 5b2f2af, it fails for me with a
> > "Terminating on signal SIGBREAK".
> >
> 
> Yes, I saw similar this morning, which woud have been after that commit.

I can't reproduce that here - could either (or both) of you send

src/test/recovery/tmp_check/log/regress_log_021_row_visibility
src/test/recovery/tmp_check/log/021_row_visibility_standby.log
src/test/recovery/tmp_check/log/021_row_visibility_primary.log

of a failed run? And maybe how you're invoking it?

Does adding a

$psql_primary{run}->finish;
$psql_standby{run}->finish;
before
$psql_primary{run}->kill_kill;
$psql_standby{run}->kill_kill;

fix the issue?

Regards,

Andres

  10. Re: buildfarm windows checks / tap tests on windows

    Andres Freund <andres@anarazel.de> — 2021-03-03T05:56:06Z

    Hi,

On 2021-03-02 21:20:11 -0800, Andres Freund wrote:
> On 2021-03-02 12:57:57 -0800, Andres Freund wrote:
> > t/003_recovery_targets.pl ............ 7/9
> > #   Failed test 'multiple conflicting settings'
> > #   at t/003_recovery_targets.pl line 151.
> > 
> > #   Failed test 'recovery end before target reached is a fatal error'
> > #   at t/003_recovery_targets.pl line 177.
> > t/003_recovery_targets.pl ............ 9/9 # Looks like you failed 2 tests of 9.
> > t/003_recovery_targets.pl ............ Dubious, test returned 2 (wstat 512, 0x200)
> > Failed 2/9 subtests
> 
> This appears to be caused by stderr in windows docker containers to
> somehow not work quite right. cirrus-ci uses docker on windows.
> 
> If you look e.g. at https://cirrus-ci.com/task/6111560255930368, and
> specifically at the relevant log file:
> https://api.cirrus-ci.com/v1/artifact/task/6111560255930368/log/src/test/recovery/tmp_check/log/003_recovery_targets_primary.log
> you can see that it's, uh, less full than we normally expect:
>         1 file(s) copied.
>         1 file(s) copied.
>         1 file(s) copied.
>         1 file(s) copied.
> 
> As that test uses the log file to determine the state of servers:
> > my $logfile = slurp_file($node_standby->logfile());
> > ok($logfile =~ qr/multiple recovery targets specified/,
> >         'multiple conflicting settings');
> 
> that doesn't work.
> 
> 
> I was *very* confused by this for a while. But finally the cluebait hit
> when I discovered that stderr works just fine for *other*
> programs. Including the programs that evidently log into
> 003_recovery_targets_primary.log.  The problem is that
> pgwin32_is_service() somehow decides that postgres is running as a
> service. Despite that not really being the case (I guess somehow
> internally docker containers are started below a service, and that
> causes the problem).
> 
> I hate everything right now. So much.
> 
> I think it's quite nasty that postgres just silently starts to log to
> the event log. Why on earth wasn't the solution instead to hardcode that
> as a server parameter in pg_ctl register?
> 
> Not sure what a good fix is for this.

FWIW, just forcing pgwin32_is_service() to return false seems to get the
cirrus tests past 003_recovery_targets.pl. Possible it'll not finish due
to other problems (or too tight timeouts I set), but at least this one
can be considered diagnosed I think.

https://cirrus-ci.com/task/5049764917018624?command=windows_worker_buf#L132

Greetings,

Andres Freund

  11. Re: buildfarm windows checks / tap tests on windows

    Michael Paquier <michael@paquier.xyz> — 2021-03-03T06:57:47Z

    On Tue, Mar 02, 2021 at 09:47:18PM -0800, Andres Freund wrote:
> I can't reproduce that here - could either (or both) of you send
> 
> src/test/recovery/tmp_check/log/regress_log_021_row_visibility
> src/test/recovery/tmp_check/log/021_row_visibility_standby.log
> src/test/recovery/tmp_check/log/021_row_visibility_primary.log
> 
> of a failed run? And maybe how you're invoking it?

I have not checked this stuff in details, but here you go.  I have
simply invoked that with vcregress taptest src/test/recovery/,
speeding up the process by removing temporarily all the other
scripts.
--
Michael

  12. Re: buildfarm windows checks / tap tests on windows

    Andrew Dunstan <andrew@dunslane.net> — 2021-03-03T12:57:27Z

    On 3/3/21 12:47 AM, Andres Freund wrote:
> Hi,
>
> On 2021-03-02 21:20:52 -0500, Andrew Dunstan wrote:
>> On 3/2/21 8:27 PM, Michael Paquier wrote:
>>> On Tue, Mar 02, 2021 at 04:54:57PM -0800, Andres Freund wrote:
>>>> It still does, even after
>>>>
>>>> commit 1e6e40447115ca7b4749d7d117b81b016ee5e2c2 (upstream/master, master)
>>>> Author: Andres Freund <andres@anarazel.de>
>>>> Date:   2021-03-01 09:52:15 -0800
>>>>
>>>>     Fix recovery test hang in 021_row_visibility.pl on windows.
>>>>
>>>> ? I didn't see failures after that?
>>> Yes.  Testing this morning on top of 5b2f2af, it fails for me with a
>>> "Terminating on signal SIGBREAK".
>>>
>> Yes, I saw similar this morning, which woud have been after that commit.
> I can't reproduce that here - could either (or both) of you send
>
> src/test/recovery/tmp_check/log/regress_log_021_row_visibility
> src/test/recovery/tmp_check/log/021_row_visibility_standby.log
> src/test/recovery/tmp_check/log/021_row_visibility_primary.log
>
> of a failed run? And maybe how you're invoking it?
>
> Does adding a
>
> $psql_primary{run}->finish;
> $psql_standby{run}->finish;
> before
> $psql_primary{run}->kill_kill;
> $psql_standby{run}->kill_kill;
>
> fix the issue?
>




I will check later on. Note that IPC::Run's kill_kill is known to cause
problems on MSWin32 perl, see the other recovery checks where we skip
tests involving it - crash_recovery and logical_decoding.


Maybe we need a wrapper for it that dies if called on MSWin32 perl. That
at least would not crash the invoking service with a nasty signal, and
the buildfarm would actually tell us about the issue.


cheers


andrew

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

  13. Re: buildfarm windows checks / tap tests on windows

    Andrew Dunstan <andrew@dunslane.net> — 2021-03-03T15:18:44Z

    On 3/2/21 3:57 PM, Andres Freund wrote:
>
>>> This makes it even clearer to me that we really need a builtin
>>> testrunner that runs tests efficiently *and* debuggable on windows.
>>>
>> "show me the code" :-)
> The biggest obstacle on that front is perl. I started to write one, but
> hit several perl issues within an hour. I think I might write one in
> python, that'd be a lot less painful.



Without knowing details I'm skeptical. Over nearly three decades of
using perl I have found very little I wanted to do that I could not.



>
>
> One windows build question I have is why the msvc infrastructure doesn't
> accept msys perl in places like this:
> 		guid                  => $^O eq "MSWin32" ? Win32::GuidGen() : 'FAKE',
> If I change them to accept msys perl then the build ends up working.
>
> The reason it'd be nice to accept msys perl is that git for windows
> bundles that - and that's already installed on most CI projects...


Nice idea, but we can't run prove under Git's msys perl, because it's
missing some stuff, at least on drongo:

C:\prog>bin\prove --version

C:\prog>"c:\Program Files\Git\usr\bin\perl" "c:\Program
Files\Git\usr\bin\core_perl\prove" --version
Can't locate TAP/Harness/Env.pm in @INC (you may need to install the
TAP::Harness::Env module) (@INC contains: /usr/lib/perl5/site_perl
/usr/share/perl5/site_perl /usr/lib/perl5/vendor_perl
/usr/share/perl5/vendor_perl /usr/lib/perl5/core_perl
/usr/share/perl5/core_perl) at /usr/share/perl5/core_perl/App/Prove.pm
line 6.
BEGIN failed--compilation aborted at
/usr/share/perl5/core_perl/App/Prove.pm line 6.
Compilation failed in require at c:\Program
Files\Git\usr\bin\core_perl\prove line 9.
BEGIN failed--compilation aborted at c:\Program
Files\Git\usr\bin\core_perl\prove line 9.



cheers


andrew


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

  14. Re: buildfarm windows checks / tap tests on windows

    Andrew Dunstan <andrew@dunslane.net> — 2021-03-03T21:07:13Z

    On 3/3/21 7:57 AM, Andrew Dunstan wrote:
> On 3/3/21 12:47 AM, Andres Freund wrote:
>> Hi,
>>
>> On 2021-03-02 21:20:52 -0500, Andrew Dunstan wrote:
>>> On 3/2/21 8:27 PM, Michael Paquier wrote:
>>>> On Tue, Mar 02, 2021 at 04:54:57PM -0800, Andres Freund wrote:
>>>>> It still does, even after
>>>>>
>>>>> commit 1e6e40447115ca7b4749d7d117b81b016ee5e2c2 (upstream/master, master)
>>>>> Author: Andres Freund <andres@anarazel.de>
>>>>> Date:   2021-03-01 09:52:15 -0800
>>>>>
>>>>>     Fix recovery test hang in 021_row_visibility.pl on windows.
>>>>>
>>>>> ? I didn't see failures after that?
>>>> Yes.  Testing this morning on top of 5b2f2af, it fails for me with a
>>>> "Terminating on signal SIGBREAK".
>>>>
>>> Yes, I saw similar this morning, which woud have been after that commit.
>> I can't reproduce that here - could either (or both) of you send
>>
>> src/test/recovery/tmp_check/log/regress_log_021_row_visibility
>> src/test/recovery/tmp_check/log/021_row_visibility_standby.log
>> src/test/recovery/tmp_check/log/021_row_visibility_primary.log
>>
>> of a failed run? And maybe how you're invoking it?
>>
>> Does adding a
>>
>> $psql_primary{run}->finish;
>> $psql_standby{run}->finish;
>> before
>> $psql_primary{run}->kill_kill;
>> $psql_standby{run}->kill_kill;
>>
>> fix the issue?
>>
>
>
>
> I will check later on. Note that IPC::Run's kill_kill is known to cause
> problems on MSWin32 perl, see the other recovery checks where we skip
> tests involving it - crash_recovery and logical_decoding.


Here's what I actually got working. Rip out the calls to kill_kill and
replace them with:


    $psql_primary{stdin} .= "\\q\n";
    $psql_primary{run}->pump_nb();
    $psql_standby{stdin} .= "\\q\n";
    $psql_standby{run}->pump_nb();
    sleep 2; # give them time to quit


No hang or signal now.


cheers


andrew


-- 

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

  15. Re: buildfarm windows checks / tap tests on windows

    Andres Freund <andres@anarazel.de> — 2021-03-03T21:42:09Z

    Hi,

On 2021-03-03 16:07:13 -0500, Andrew Dunstan wrote:
> Here's what I actually got working. Rip out the calls to kill_kill and
> replace them with:
> 
> 
>     $psql_primary{stdin} .= "\\q\n";
>     $psql_primary{run}->pump_nb();
>     $psql_standby{stdin} .= "\\q\n";
>     $psql_standby{run}->pump_nb();
>     sleep 2; # give them time to quit
> 
> 
> No hang or signal now.

Hm. I wonder if we can avoid the sleep 2 by doing something like
->pump(); ->finish(); instead of one pump_nb()? One pump() is needed to
send the \q to psql, and then we need to wait for the process to finish?
I'll try that, but given that I can't reproduce any problems...

I suspect that at least the crash recovery test suffers from exactly the
same problem and that we can re-enable it on windows after doign an
equivalent change...

Greetings,

Andres Freund

  16. Re: buildfarm windows checks / tap tests on windows

    Andrew Dunstan <andrew@dunslane.net> — 2021-03-03T22:32:29Z

    On 3/3/21 4:42 PM, Andres Freund wrote:
> Hi,
>
> On 2021-03-03 16:07:13 -0500, Andrew Dunstan wrote:
>> Here's what I actually got working. Rip out the calls to kill_kill and
>> replace them with:
>>
>>
>>     $psql_primary{stdin} .= "\\q\n";
>>     $psql_primary{run}->pump_nb();
>>     $psql_standby{stdin} .= "\\q\n";
>>     $psql_standby{run}->pump_nb();
>>     sleep 2; # give them time to quit
>>
>>
>> No hang or signal now.
> Hm. I wonder if we can avoid the sleep 2 by doing something like
> ->pump(); ->finish(); instead of one pump_nb()? One pump() is needed to
> send the \q to psql, and then we need to wait for the process to finish?
> I'll try that, but given that I can't reproduce any problems...


Looking at the examples in the IPC:Run docco, it looks like we might
just be able to replace the pump_nb above with finish, and leave the
sleep out. I'll try that.


>
> I suspect that at least the crash recovery test suffers from exactly the
> same problem and that we can re-enable it on windows after doign an
> equivalent change...
>

Yes, possibly - it would be good to remove those skips.


cheers


andrew

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

  17. Re: buildfarm windows checks / tap tests on windows

    Andrew Dunstan <andrew@dunslane.net> — 2021-03-04T00:21:54Z

    On 3/3/21 5:32 PM, Andrew Dunstan wrote:
> On 3/3/21 4:42 PM, Andres Freund wrote:
>> Hi,
>>
>> On 2021-03-03 16:07:13 -0500, Andrew Dunstan wrote:
>>> Here's what I actually got working. Rip out the calls to kill_kill and
>>> replace them with:
>>>
>>>
>>>     $psql_primary{stdin} .= "\\q\n";
>>>     $psql_primary{run}->pump_nb();
>>>     $psql_standby{stdin} .= "\\q\n";
>>>     $psql_standby{run}->pump_nb();
>>>     sleep 2; # give them time to quit
>>>
>>>
>>> No hang or signal now.
>> Hm. I wonder if we can avoid the sleep 2 by doing something like
>> ->pump(); ->finish(); instead of one pump_nb()? One pump() is needed to
>> send the \q to psql, and then we need to wait for the process to finish?
>> I'll try that, but given that I can't reproduce any problems...
>
> Looking at the examples in the IPC:Run docco, it looks like we might
> just be able to replace the pump_nb above with finish, and leave the
> sleep out. I'll try that.


OK, this worked fine. I'll try it s a recipe in the other places where
kill_kill is forcing us to skip tests under MSwin32 perl, and see how we go.


cheers


andrew

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

  18. Re: buildfarm windows checks / tap tests on windows

    Andrew Dunstan <andrew@dunslane.net> — 2021-03-04T16:10:19Z

    On 3/3/21 7:21 PM, Andrew Dunstan wrote:
> On 3/3/21 5:32 PM, Andrew Dunstan wrote:
>> On 3/3/21 4:42 PM, Andres Freund wrote:
>>> Hi,
>>>
>>> On 2021-03-03 16:07:13 -0500, Andrew Dunstan wrote:
>>>> Here's what I actually got working. Rip out the calls to kill_kill and
>>>> replace them with:
>>>>
>>>>
>>>>     $psql_primary{stdin} .= "\\q\n";
>>>>     $psql_primary{run}->pump_nb();
>>>>     $psql_standby{stdin} .= "\\q\n";
>>>>     $psql_standby{run}->pump_nb();
>>>>     sleep 2; # give them time to quit
>>>>
>>>>
>>>> No hang or signal now.
>>> Hm. I wonder if we can avoid the sleep 2 by doing something like
>>> ->pump(); ->finish(); instead of one pump_nb()? One pump() is needed to
>>> send the \q to psql, and then we need to wait for the process to finish?
>>> I'll try that, but given that I can't reproduce any problems...
>> Looking at the examples in the IPC:Run docco, it looks like we might
>> just be able to replace the pump_nb above with finish, and leave the
>> sleep out. I'll try that.
>
> OK, this worked fine. I'll try it s a recipe in the other places where
> kill_kill is forcing us to skip tests under MSwin32 perl, and see how we go.
>
>


Here's the patch. I didn't see a convenient way of handling the
pg_recvlogical case, so that's unchanged.


cheers


andrew

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

  19. Re: buildfarm windows checks / tap tests on windows

    Andres Freund <andres@anarazel.de> — 2021-03-04T17:56:50Z

    Hi,

On 2021-03-04 11:10:19 -0500, Andrew Dunstan wrote:
> Here's the patch.

Awesome. Will you commit it?


> I didn't see a convenient way of handling the pg_recvlogical case, so
> that's unchanged.

Is the problem actually the kill_kill() itself, or just doing
->kill_kill() without a subsequent ->finish()?

But anyway, that seems like a less critical test...

Greetings,

Andres Freund

  20. Re: buildfarm windows checks / tap tests on windows

    Andrew Dunstan <andrew@dunslane.net> — 2021-03-04T18:31:39Z

    On 3/4/21 12:56 PM, Andres Freund wrote:
> Hi,
>
> On 2021-03-04 11:10:19 -0500, Andrew Dunstan wrote:
>> Here's the patch.
> Awesome. Will you commit it?



Done


>
>
>> I didn't see a convenient way of handling the pg_recvlogical case, so
>> that's unchanged.
> Is the problem actually the kill_kill() itself, or just doing
> ->kill_kill() without a subsequent ->finish()?


Pretty sure it's the kill_kill that causes the awful crash Michael and I
have seen.



>
> But anyway, that seems like a less critical test...



right


cheers


andrew

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