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.