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.