Re: Improve error detections in TAP tests by spreading safe_psql

On Wed, Aug 28, 2019 at 12:19:08PM -0400, Tom Lane wrote:
> The attached patch just follows a mechanical rule of "set on_error_die
> to 0 in exactly those calls where the surrounding code verifies the
> exit code is what it expects". That leads to a lot of code that
> looks like, say,
>
> # Insert should work on standby
> is( $node_standby->psql(
> 'postgres',
> - qq{insert into testtab select generate_series(1,1000), 'foo';}),
> + qq{insert into testtab select generate_series(1,1000), 'foo';},
> + on_error_die => 0),
> 0,
> 'INSERT succeeds with truncated relation FSM');

I am fine with that approach. There is an argument for dropping
safe_psql then?

> I have to wonder if it isn't better to just drop the is() test
> and let PostgresNode::psql issue the failure. The only thing
> the is() is really buying us is the ability to label the test
> with an ID string, which is helpful, but um ... couldn't that
> just be a comment?

I got the same thought as you on this point about why the is() is
actually necessary if we know that it would fail. An advantage of the
current code is that we are able to catch all errors in a given run at
once. If the test dies immediately, you cannot catch that and this
needs repetitive retries if there is no dependency between each step
of the test. An argument against back-patching the stuff changing the
default flag are tests which rely on the current behavior. This could
be annoying for some people doing advanced testing.
--
Michael