Re: TAP output format in pg_regress

> On 24 Nov 2022, at 18:07, Nikolay Shaplov <dhyan(at)nataraj(dot)su> wrote:
>
> You guys are really fast... I only think about problem, it is already
> mentioned here... Most issues I've noticed are already fixed before I was able
> to say something.

Thanks for looking and reviewing!

> /*
> * Bailing out is for unrecoverable errors which prevents further testing to
> * occur and after which the test run should be aborted. By passing non_rec
> * as true the process will exit with _exit(2) and skipping registered exit
> * handlers.
> */
> static void
> bail_out(bool non_rec, const char *fmt,...)
> {
>
> In original code, where _exit were called, there were mention about recursion
> (whatever it is) as a reason for using _exit() instead of exit(). Now this
> mention is gone:
>
> - _exit(2); /* not exit(), that could be recursive */
> + /* Not using the normal bail() as we want _exit */
> + _bail(_("could not stop postmaster: exit code was %d\n"), r);
>
> The only remaining part of recursion is _rec suffix.
>
> I guess we should keep mention of recursion in comments, and for me _rec
> stands for "record", not "recursion", I will not guess original meaning by
> _rec suffix. I would suggest to change it to _recurs or _recursive to keep
> things clear

The other original comment on _exit usage reads:

/* not exit() here... */

I do think that the longer explanation I added in the comment you quoted above
is more explanatory than those. I can however add a small note on why skipping
registered exit handlers is useful (ie, not risk recursive calls to exit()).

> +#define _bail(...) bail_out(true, __VA_ARGS__)
> +#define bail(...) bail_out(false, __VA_ARGS__)
>
> I will never guess what the difference between bail, _bail and bail_out. We
> need really good explanation here, or better to use self-explanatory names
> here...
>
> I would start bail_out with _ as it is "internal", not used in the code.
> And for _bail I would try to find some name that shows it's nature. Like
> bail_in_recursion or something.

One option could be to redefine bail() to take the exit function as a parameter
and have the caller pass the preferred exit handler.

-bail_out(bool non_rec, const char *fmt,...)
+bail(void (*exit_func)(int), const char *fmt,...)

The callsites would then look like the below, which puts a reference to the
actual exit handler used in the code where it is called. I find it a bit
uglier, but also quite self-explanatory:

@@ -409,10 +403,7 @@ stop_postmaster(void)
fflush(NULL);
r = system(buf);
if (r != 0)
- {
- /* Not using the normal bail() as we want _exit */
- _bail(_("could not stop postmaster: exit code was %d\n"), r);
- }
+ bail(_exit, _("could not stop postmaster: exit code was %d\n"), r);

postmaster_running = false;
}
@@ -469,7 +460,7 @@ make_temp_sockdir(void)
temp_sockdir = mkdtemp(template);
if (temp_sockdir == NULL)
{
- bail(_("could not create directory \"%s\": %s\n"),
+ bail(exit, _("could not create directory \"%s\": %s\n"),
template, strerror(errno));
}

Not sure what is the best option, but I've been unable to think of a name which
is documenting the code well enough that a comment explaining why isn't
required.

> This magic spell "...%-5i %s%-*s %8.0f ms\n" is too dark to repeat it even two
> times. I understand problems with spaces... But may be it would be better
> somehow narrow it to one ugly print... Print "ok %-5i "|"not ok %-5i" to
> buffer first, and then have one "%s%-*s %8.0f ms%s\n" print or something like
> that...

I'm not convinced that this printf format is that hard to read (which may well
be attributed to Stockholm Syndrome), and I do think that breaking it up and
adding more code to print the line will make it less readable instead.

--
Daniel Gustafsson https://vmware.com/