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

On 13-06-2018 22:44, Fabien COELHO wrote:
> 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.

AFAICS it is not about debugging pgbench itself, but about more detailed 
information that can be used to understand what exactly happened during 
its launch. In the case of errors this helps to distinguish between 
failures or errors by type (including which limit for retries was 
violated and how far it was exceeded for the serialization/deadlock 
errors).

>>> 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.

> 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.

I think we need ereport() if we want to make detailed error messages 
(see examples in [1])..

>>> 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.

> So my current view is that if you only need an "elog" function, it is
> simpler to add it to "pgbench.c".

Thank you!

>>> 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:-)

To make the patch committable there should be no objection to it..

[1] 
https://www.postgresql.org/message-id/c89fcc380a19380260b5ea463efc1416%40postgrespro.ru

-- 
Marina Polyakova
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company