Re: pgbench - use pg logging capabilities

Michael Paquier <[email protected]> Fri, 10 Jan 2020 13:08:44 +0900

Newsgroups	gmane.comp.db.postgresql.devel.general
Message-ID	<[email protected]>

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

log-level-stuff.patch (text/x-diff, 3.1 KB)

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 (application/pgp-signature, 833 B)