On 10-06-2018 10:38, Fabien COELHO wrote:
Hello Marina,
Hello!
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.
If you mean abortion of the client, this is not an abortion of the main
program.
- 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?
I'd suggest that it should be an independent submission, unrelated to
the pgbench error management patch.
I suppose that this is related; because of my patch there may be a lot
of such code (see v7 in [1]):
- fprintf(stderr,
- "malformed variable \"%s\" value:
\"%s\"\n",
- var->name, var->svalue);
+ if (debug_level >= DEBUG_FAILS)
+ {
+ fprintf(stderr,
+ "malformed variable \"%s\" value:
\"%s\"\n",
+ var->name, var->svalue);
+ }
- if (debug)
+ if (debug_level >= DEBUG_ALL)
fprintf(stderr, "client %d sending %s\n", st->id, sql);
That's why it was suggested to make the error function which hides all
these things (see [2]):
There is a lot of checks like "if (debug_level >= DEBUG_FAILS)" with
corresponding fprintf(stderr..) I think it's time to do it like in the
main code, wrap with some function like log(level, msg).
Moreover, it changes pgbench current
behavior, which might be admissible, but should be discussed clearly.
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.
Oh, thanks, I agree with you. And I do not want to change the program
exit code without good reasons, but I'm sorry I may not know all pros
and cons in this matter..
Or did you also mean other changes?
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.
I was recommended to use the same interface in [3]:
On elog/errstart: we already have a convention for what ereport() calls
look like; I suggest to use that instead of inventing your own.
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'll try to do it..
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.
Ok!
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 was also recommended to use ereport() instead of elog() in [3]:
With that, is there a need for elog()? In the backend we have it
because $HISTORY but there's no need for that here -- I propose to lose
elog() and use only ereport everywhere.
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.
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.
I agree that elog() can be coded in this way. To use ereport() I need a
structure to store the error level as a condition to exit.
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.
I hope I answered all this above..
[1]
https://www.postgresql.org/message-id/453fa52de88477df2c4a2d82e09e461c%40postgrespro.ru
[2]
https://www.postgresql.org/message-id/20180405180807.0bc1114f%40wp.localdomain
[3]
https://www.postgresql.org/message-id/20180508105832.6o3uf3npfpjgk5m7%40alvherre.pgsql
--
Marina Polyakova
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company
