Michael Paquier <michael@paquier.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