On Thu, Jan 09, 2020 at 08:09:29PM -0500, Tom Lane wrote: > TBH, my recommendation would be to drop *all* of these likely() > and unlikely() calls. What evidence have you got that those are > meaningfully improving the quality of the generated code? And if > they're buried inside macros, they certainly aren't doing anything > useful in terms of documenting the code.
Yes. I am wondering if we should not rework this part of the logging with something like the attached. My 2c, thoughts welcome. -- Michael
diff --git a/src/include/common/logging.h b/src/include/common/logging.h index 028149c7a1..dd54dc57d0 100644 --- a/src/include/common/logging.h +++ b/src/include/common/logging.h @@ -55,8 +55,6 @@ enum pg_log_level PG_LOG_OFF, }; -extern enum pg_log_level __pg_log_level; - /* * Kind of a hack to be able to produce the psql output exactly as required by * the regression tests. @@ -66,6 +64,7 @@ extern enum pg_log_level __pg_log_level; void pg_logging_init(const char *argv0); void pg_logging_config(int new_flags); void pg_logging_set_level(enum pg_log_level new_level); +enum pg_log_level pg_logging_get_level(void); void pg_logging_set_pre_callback(void (*cb) (void)); void pg_logging_set_locus_callback(void (*cb) (const char **filename, uint64 *lineno)); @@ -73,23 +72,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__); \ + 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__); \ + 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__); \ + 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__); \ + 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__); \ + pg_log_generic(PG_LOG_DEBUG, __VA_ARGS__); \ } while(0) #endif /* COMMON_LOGGING_H */ diff --git a/src/common/logging.c b/src/common/logging.c index c78ae793b8..18b3c94fe2 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; +static enum pg_log_level __pg_log_level; static const char *progname; static int log_flags; @@ -110,6 +110,12 @@ pg_logging_set_level(enum pg_log_level new_level) __pg_log_level = new_level; } +enum pg_log_level +pg_logging_get_level(void) +{ + return __pg_log_level; +} + void pg_logging_set_pre_callback(void (*cb) (void)) { @@ -127,6 +133,9 @@ pg_log_generic(enum pg_log_level level, const char *pg_restrict fmt,...) { va_list ap; + if (level < __pg_log_level) + return; + va_start(ap, fmt); pg_log_generic_v(level, fmt, ap); va_end(ap); diff --git a/src/bin/pgbench/pgbench.c b/src/bin/pgbench/pgbench.c index 39c1a243d5..5e63e1f51a 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 (pg_logging_get_level() <= PG_LOG_DEBUG) { PQExpBufferData buf;
signature.asc
Description: PGP signature