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.
diff --git a/src/bin/pgbench/pgbench.c b/src/bin/pgbench/pgbench.c
index 39c1a243d5..e7b33bb8e0 100644
--- a/src/bin/pgbench/pgbench.c
+++ b/src/bin/pgbench/pgbench.c
@@ -3295,7 +3295,7 @@ executeMetaCommand(CState *st, instr_time *now)
argc = command->argc;
argv = command->argv;
- if (unlikely(__pg_log_level <= PG_LOG_DEBUG))
+ if (unlikely(pg_log_current_level <= PG_LOG_DEBUG))
{
PQExpBufferData buf;
diff --git a/src/common/logging.c b/src/common/logging.c
index c78ae793b8..05827c52cc 100644
--- a/src/common/logging.c
+++ b/src/common/logging.c
@@ -13,7 +13,7 @@
#include "common/logging.h"
-enum pg_log_level __pg_log_level;
+enum pg_log_level pg_log_current_level;
static const char *progname;
static int log_flags;
@@ -45,7 +45,7 @@ pg_logging_init(const char *argv0)
setvbuf(stderr, NULL, _IONBF, 0);
progname = get_progname(argv0);
- __pg_log_level = PG_LOG_INFO;
+ pg_log_current_level = PG_LOG_INFO;
if (pg_color_env)
{
@@ -107,7 +107,7 @@ pg_logging_config(int new_flags)
void
pg_logging_set_level(enum pg_log_level new_level)
{
- __pg_log_level = new_level;
+ pg_log_current_level = new_level;
}
void
diff --git a/src/include/common/logging.h b/src/include/common/logging.h
index 028149c7a1..487ce21cf7 100644
--- a/src/include/common/logging.h
+++ b/src/include/common/logging.h
@@ -55,7 +55,7 @@ enum pg_log_level
PG_LOG_OFF,
};
-extern enum pg_log_level __pg_log_level;
+extern enum pg_log_level pg_log_current_level;
/*
* Kind of a hack to be able to produce the psql output exactly as required by
@@ -73,23 +73,23 @@ void pg_log_generic(enum pg_log_level level, const char *pg_restrict fmt,...) p
void pg_log_generic_v(enum pg_log_level level, const char *pg_restrict fmt, va_list ap) pg_attribute_printf(2, 0);
#define pg_log_fatal(...) do { \
- if (likely(__pg_log_level <= PG_LOG_FATAL)) pg_log_generic(PG_LOG_FATAL, __VA_ARGS__); \
+ if (likely(pg_log_current_level <= PG_LOG_FATAL)) pg_log_generic(PG_LOG_FATAL, __VA_ARGS__); \
} while(0)
#define pg_log_error(...) do { \
- if (likely(__pg_log_level <= PG_LOG_ERROR)) pg_log_generic(PG_LOG_ERROR, __VA_ARGS__); \
+ if (likely(pg_log_current_level <= PG_LOG_ERROR)) pg_log_generic(PG_LOG_ERROR, __VA_ARGS__); \
} while(0)
#define pg_log_warning(...) do { \
- if (likely(__pg_log_level <= PG_LOG_WARNING)) pg_log_generic(PG_LOG_WARNING, __VA_ARGS__); \
+ if (likely(pg_log_current_level <= PG_LOG_WARNING)) pg_log_generic(PG_LOG_WARNING, __VA_ARGS__); \
} while(0)
#define pg_log_info(...) do { \
- if (likely(__pg_log_level <= PG_LOG_INFO)) pg_log_generic(PG_LOG_INFO, __VA_ARGS__); \
+ if (likely(pg_log_current_level <= PG_LOG_INFO)) pg_log_generic(PG_LOG_INFO, __VA_ARGS__); \
} while(0)
#define pg_log_debug(...) do { \
- if (unlikely(__pg_log_level <= PG_LOG_DEBUG)) pg_log_generic(PG_LOG_DEBUG, __VA_ARGS__); \
+ if (unlikely(pg_log_current_level <= PG_LOG_DEBUG)) pg_log_generic(PG_LOG_DEBUG, __VA_ARGS__); \
} while(0)
#endif /* COMMON_LOGGING_H */
