Re: Improve error detections in TAP tests by spreading safe_psql

Michael Paquier <michael(at)paquier(dot)xyz> writes:
> 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".

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

Well, it's useful if you just want the stdout back. But its name
is a bit misleading if the default behavior of psql is just as
safe. Not sure whether renaming it is worthwhile.

>> I have to wonder if it isn't better to just drop the is() test
>> and let PostgresNode::psql issue the failure.

> 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.

Yeah, but only if the test cases are independent, which I think is
mostly not true in our TAP scripts. Otherwise you're just looking
at cascading errors.

> 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.

Yup, the tradeoff is people possibly having test scripts outside
our tree that would break, versus the possibility that we'll back-patch
test changes that don't behave as expected in back branches. I don't
know if the former is true, but I'm afraid the latter is a certainty
if we don't back-patch.

regards, tom lane