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

On 10-08-2018 15:53, Arthur Zakirov wrote:
> On Thu, Aug 09, 2018 at 06:17:22PM +0300, Marina Polyakova wrote:
>> > * ErrorLevel
>> >
>> > If ErrorLevel is used for things which are not errors, its name should
>> > not include "Error"? Maybe "LogLevel"?
>> 
>> On the one hand, this sounds better for me too. On the other hand, 
>> will not
>> this be in some kind of conflict with error level codes in elog.h?..
> 
> I think it shouldn't because those error levels are backends levels.
> pgbench is a client side utility with its own code, it shares some code
> with libpq and other utilities, but elog.h isn't one of them.

I agree with you on this :) I just meant that maybe it would be better 
to call this group in the same way because they are used in general for 
the same purpose?..

>> > This point also suggest that maybe "pgbench_error" is misnamed as well
>> > (ok, I know I suggested it in place of ereport, but e stands for error
>> > there), as it is called on errors, but is also on other things. Maybe
>> > "pgbench_log"? Or just simply "log" or "report", as it is really an
>> > local function, which does not need a prefix? That would mean that
>> > "pgbench_simple_error", which is indeed called on errors, could keep
>> > its initial name "pgbench_error", and be called on errors.
>> 
>> About the name "log" - we already have the function doLog, so perhaps 
>> the
>> name "report" will be better.. But like with ErrorLevel will not this 
>> be in
>> some kind of conflict with ereport which is also used for the levels
>> DEBUG... / LOG / INFO?
> 
> +1 from me to keep initial name "pgbench_error". "pgbench_log" for new
> function looks nice to me. I think it is better than just "log",
> because "log" may conflict with natural logarithmic function (see "man 
> 3
> log").

Do you think that pgbench_log (or another whose name speaks only about 
logging) will look good, for example, with FATAL? Because this means 
that the logging function also processes errors and calls exit(1) if 
necessary..

>> > pgbench_error calls pgbench_error. Hmmm, why not.
> 
> I agree with Fabien. Calling pgbench_error() inside pgbench_error()
> could be dangerous. I think "fmt" checking could be removed, or we may
> use Assert()

I would like not to use Assert in this case because IIUC they are mostly 
used for testing.

> or fprintf()+exit(1) at least.

Ok!

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