Re: pgbench - use pg logging capabilities - Mailing list pgsql-hackers

Michaël,

>> So I think that the current situation is a good thing at least for debug.
>
> If you look at some of my messages on other threads, you would likely
> notice that my mood of the day is to not design things which try to
> outsmart a user's expectations :)

I'm not following you.

ISTM that user expectations is that the message is printed when the level 
requires it, and that the performance impact is minimal otherwise.

I'm not aiming at anything different.

> So I would stand on the position to just remove those likely/unlikely
> parts if we want this logging to be generic.

It is unclear to me whether your point is about the whole "if", or only 
the compiler directive itself (i.e. "likely" and "unlikely").

I'll assume the former. I do not think we should "want" logging to be 
generic per se, but only if it makes sense from a performance and feature 
point of view.

For the normal case (standard level, no debug), there is basically no 
difference because the message is going to be printed anyway: either it is 
check+call+work, or call+check+work. Anything is fine. The directive would 
help the compiler reorder instructions so that usual case does not inccur 
a jump.

For debug messages, things are different: removing the external test & 
unlikely would have a detrimental effect on performance when not 
debugging, which is most of the time, because you would pay the cost of 
evaluating arguments and call/return cycle on each message anyway. That 
can be bad if a debug message is place in some critical place.

So the right place of the the debug check is early. Once this is done, 
then why not doing that for all other level for consistency? This is the 
current situation.

If the check is moved inside the call, then there is a performance benefit 
to repeat it for debug, which is a pain because then it would be there 
twice in that case, and it creates an exception. The fact that some macro 
are simplified is not very useful because this is not really user visible.

So IMHO the current situation is fine, but the __variable name. So ISTM 
that the attached is the simplest and more reasonable option to fix this.

>> For other levels, they are on by default AND would not be placed at critical
>> performance points, so the whole effort of avoiding the call are moot.
>>
>> I agree with Tom that __pg_log_level variable name violates usages.
>
> My own taste would be to still keep the variable local to logging.c,
> and use a "get"-like routine to be consistent with the "set" part.  I
> don't have to be right, let's see where this discussion leads us.

This would defeat the point of avoiding a function call, if a function 
call is needed to check whether the other function call is needed:-)

Hence the macro.

> (I mentioned that upthread, but I don't think it is a good idea to
> discuss about a redesign of those routines on a thread about pgbench
> based on $subject.  All the main players are here so it likely does
> not matter, but..)

Yep. I hesitated to be the one to do it, and ISTM that the problem is 
small enough so that it can be resolved without a new thread. I may be 
naïvely wrong:-)

-- 
Fabien.