Re: [HACKERS] WIP Patch: Pgbench Serialization and deadlock errors

Hello Marina,

> v9-0003-Pgbench-errors-use-the-ereport-macro-to-report-de.patch
> - a patch for the ereport() macro (this is used to report client failures
> that do not cause an aborts and this depends on the level of debugging).

ISTM that abort() is called under FATAL.

> - implementation: if possible, use the local ErrorData structure during the
> errstart()/errmsg()/errfinish() calls. Otherwise use a static variable
> protected by a mutex if necessary. To do all of this export the function
> appendPQExpBufferVA from libpq.

This patch applies cleanly on top of the other ones (there are minimal
interactions), compiles cleanly, global & pgbench "make check" are ok.

IMO this patch is more controversial than the other ones.

It is not really related to the aim of the patch series, which could do
without, couldn't it? Moreover, it changes pgbench current behavior, which
might be admissible, but should be discussed clearly.

I'd suggest that it should be an independent submission, unrelated to the
pgbench error management patch.

The code adapts/duplicates existing server-side "ereport" stuff and brings
it to the frontend, where the logging needs are somehow quite different.

I'd prefer to avoid duplication and/or have some code sharing. If it
really needs to be duplicated, I'd suggest to put all this stuff in
separated files. If we want to do that, I think that it would belong to
fe_utils, and where it could/should be used by all front-end programs.

I do not understand why names are changed, eg ELEVEL_FATAL instead of
FATAL. ISTM that part of the point of the move would be to be homogeneous,
which suggests that the same names should be reused.

For logging purposes, ISTM that the "elog" macro interface is nicer,
closer to the existing "fprintf(stderr", as it would not introduce the
additional parentheses hack for "rest".

I see no actual value in creating on the fly a dynamic buffer through
plenty macros and functions as the end result is just to print the message
out to stderr in the end.

errfinishImpl: fprintf(stderr, "%s", error->message.data);

This looks like overkill. From reading the code, this does not look
like an improvement:

fprintf(stderr, "invalid socket: %s", PQerrorMessage(st->con));

vs

ereport(ELEVEL_LOG, (errmsg("invalid socket: %s", PQerrorMessage(st->con))));

The whole complexity of the server-side interface only make sense because
TRY/CATCH stuff and complex logging requirements (eg several outputs) in
the backend. The patch adds quite some code and complexity without clear
added value that I can see.

The semantics of the existing code is changed, the FATAL levels calls
abort() and replace existing exit(1) calls. Maybe you want an ERROR level
as well.

My 0.02€: maybe you just want to turn

fprintf(stderr, format, ...);
// then possibly exit or abort depending...

into

elog(level, format, ...);

which maybe would exit or abort depending on level, and possibly not
actually report under some levels and/or some conditions. For that, it
could enough to just provide an nice "elog" function.

In conclusion, which you can disagree with because maybe I have missed
something... anyway I currently think that:

- it should be an independent submission

- possibly at "fe_utils" level

- possibly just a nice "elog" function is enough, if so just do that.

--
Fabien.