Hello Marina,
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);
I'm not sure that debug messages needs to be kept after debug, if it is
about debugging pgbench itself. That is debatable.
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).
Yep. I did not wrote that, but I agree with an "elog" suggestion to switch
if (...) { fprintf(...); exit/abort/continue/... }
to a simpler:
elog(level, ...)
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?
AFAICR I meant switching exit to abort in some cases.
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.
The "elog" interface already exists, it is not an invention. "ereport" is
a hack which is somehow necessary in some cases. I prefer a simple
function call if possible for the purpose, and ISTM that this is the case.
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..
Dunno. If you only need one "elog" function which prints a message to
stderr and decides whether to abort/exit/whatevrer, maybe it can just be
kept in pgbench. If there are are several complicated functions and
macros, better with a file. So I'd say it depends.
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]:
Probably. Are you hoping that advises from different reviewers should be
consistent? That seems optimistic:-)
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.
See commit 8a07ebb3c172 which turns some ereport into elog...
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.
Yep. That is a lot of complication which are justified server side where
logging requirements are special, but in this case I see it as overkill.
So my current view is that if you only need an "elog" function, it is
simpler to add it to "pgbench.c".
--
Fabien.
