Re: PATCH: Unlogged tables re-initialization tests

On 3/13/18 12:46, Alvaro Herrera wrote:
> Dagfinn Ilmari Mannsåker wrote:
>
>> $SIG{__DIE__} gets called even for exceptions that would be caught by a
>> surrunding eval block, so this should at the very least be:
>>
>> $SIG{__DIE__} = sub { BAIL_OUT(@_) unless $^S };
>>
>> However, that is still wrong, because die() and BAIL_OUT() mean
>> different things: die() aborts the current test script, while BAIL_OUT()
>> aborts the entire 'prove' run, i.e. all subsequent scripts in the same
>> test directory.
>
> Sounds like 'die' is the correct thing, then, and that BAIL_OUT should
> be called sparingly ... for example this one in PostgresNode::start
> seems like an overreaction:
> BAIL_OUT("node \"$name\" is already running") if defined $self->{_pid};

It does sound like BAIL_OUT should be used more sparingly. However, if
you use die, then you don't get a good error message printed, just
something like

t/020_pg_receivewal.pl ... 9/18 # Looks like you planned 18 tests but
ran 12.
# Looks like your test exited with 25 just after 12.
t/020_pg_receivewal.pl ... Dubious, test returned 25 (wstat 6400, 0x1900)
Failed 6/18 subtests

whereas with BAIL_OUT you get

t/020_pg_receivewal.pl ... 9/18 Bailout called. Further testing
stopped: foobar
FAILED--Further testing stopped: foobar

The functional difference is significant however: BAIL_OUT prevents the
030 test file to be called, die does not.

Could use more research into best practices ....

--
Peter Eisentraut http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services