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

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.