Re: TAP output format in pg_regress - Mailing list pgsql-hackers

> On 24 Nov 2022, at 18:07, Nikolay Shaplov <dhyan@nataraj.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/