Corey Huinker <corey.huinker@gmail.com> writes:
> [ v9-0003-Add-psql-variables-SHELL_ERROR-and-SHELL_EXIT_COD.patch ]
I took a brief look through this. I'm on board with the general
concept, but I think you've spent too much time on an ultimately
futile attempt to get a committable test case, and not enough time
I didn't want to give the impression that I was avoiding/dismissing the test case, and at some point I got curious how far we could push pg_regress.
on making the behavior correct/usable. In particular, it bothers
me that there doesn't appear to be any way to distinguish whether
a command failed on nonzero exit code versus a signal. Also,
That's an intriguing distinction, and one I hadn't considered, mostly because I assumed that any command with a set of exit codes rich enough to warrant inspection would have a separate exit code for i-was-terminated.
It would certainly be possible to have two independent booleans, SHELL_ERROR and SHELL_SIGNAL.
I see that you're willing to report whatever ferror returns
(something quite unspecified in relevant standards, beyond
zero/nonzero) as an equally-non-distinguishable integer.
I had imagined users of this feature falling into two camps:
1. Users writing scripts for their specific environment, with the ability to know what exit codes are possible and different desired courses of action based on those codes.
2. Users in no specific environment, content with SHELL_ERROR boolean, who are probably just going to test for failure, and if so either \set a default or \echo a message and \quit.
I'm tempted to suggest that we ought to be returning a string,
along the lines of "OK" or "exit code N" or "signal N" or
"I/O error". This though brings up the question of exactly
what you expect scripts to use the variable for. Maybe such
a definition would be unhelpful, but if so why? Maybe we
should content ourselves with adding the SHELL_ERROR boolean, and
leave the details to whatever we print in the error message?
Having a curated list of responses like "OK" and "exit code N" is also intriguing, but could be hard to unpack given psql's limited string manipulation capabilities.
I think the SHELL_ERROR boolean solves most cases, but it was suggested in
https://www.postgresql.org/message-id/20221102115801.GG16921@telsasoft.com that users might want more detail than that, even if that detail is unspecified and highly system dependent.
As for the test case, I'm unimpressed with it because it's so
weak as to be nearly useless; plus it seems like a potential
security issue (what if "nosuchcommand" exists?). It's fine
to have it during development, I guess, but I'm inclined to
leave it out of the eventual commit.
I have no objection to leaving it out. I think it proves that writing os-specific pg_regress tests is hard, and little else.
