Re: Frontend error logging style - Mailing list pgsql-hackers

I wrote:
> One other loose end is bothering me: I stuck with logging.h's
> original choice to put "if (likely())" or "if (unlikely())"
> conditionals into the macros, but I rather suspect that that's
> just a waste.  I think we should put a centralized level check
> into logging.c, and get rid of at least the "if (likely())"
> checks, because those are going to succeed approximately 100.0%
> of the time.  Maybe there's an argument for keeping the unlikely()
> ones.

Concretely, something like the attached.  As a simple check,
I looked at the compiled size of pg_dump.  It went from

  text    data     bss     dec     hex filename
 380298    4008    1384  385690   5e29a /home/postgres/testversion/bin/pg_dump

to

   text    data     bss     dec     hex filename
 374954    4008    1384  380346   5cdba src/bin/pg_dump/pg_dump

for a savings of about 5K or 1.5%.  Not a huge amount, but
not nothing either, especially considering that the existing
coding isn't buying us anything.

            regards, tom lane

diff --git a/src/bin/pg_dump/pg_backup_utils.h b/src/bin/pg_dump/pg_backup_utils.h
index 5b1c51554d..8173bb93cf 100644
--- a/src/bin/pg_dump/pg_backup_utils.h
+++ b/src/bin/pg_dump/pg_backup_utils.h
@@ -34,8 +34,7 @@ extern void exit_nicely(int code) pg_attribute_noreturn();
 /* In pg_dump, we modify pg_fatal to call exit_nicely instead of exit */
 #undef pg_fatal
 #define pg_fatal(...) do { \
-        if (likely(__pg_log_level <= PG_LOG_ERROR)) \
-            pg_log_generic(PG_LOG_ERROR, PG_LOG_PRIMARY, __VA_ARGS__); \
+        pg_log_generic(PG_LOG_ERROR, PG_LOG_PRIMARY, __VA_ARGS__); \
         exit_nicely(1); \
     } while(0)

diff --git a/src/common/logging.c b/src/common/logging.c
index 18d6669f27..8a061f46b4 100644
--- a/src/common/logging.c
+++ b/src/common/logging.c
@@ -223,6 +223,10 @@ pg_log_generic_v(enum pg_log_level level, enum pg_log_part part,
     Assert(fmt);
     Assert(fmt[strlen(fmt) - 1] != '\n');

+    /* Do nothing if log level is too low. */
+    if (level < __pg_log_level)
+        return;
+
     /*
      * Flush stdout before output to stderr, to ensure sync even when stdout
      * is buffered.
diff --git a/src/include/common/logging.h b/src/include/common/logging.h
index e213bb70d0..35c7c7b976 100644
--- a/src/include/common/logging.h
+++ b/src/include/common/logging.h
@@ -104,48 +104,39 @@ void        pg_log_generic_v(enum pg_log_level level, enum pg_log_part part,
  * pg_log_generic[_v] directly, except perhaps in error interface code.
  */
 #define pg_log_error(...) do { \
-        if (likely(__pg_log_level <= PG_LOG_ERROR)) \
-            pg_log_generic(PG_LOG_ERROR, PG_LOG_PRIMARY, __VA_ARGS__); \
+        pg_log_generic(PG_LOG_ERROR, PG_LOG_PRIMARY, __VA_ARGS__); \
     } while(0)

 #define pg_log_error_detail(...) do { \
-        if (likely(__pg_log_level <= PG_LOG_ERROR)) \
-            pg_log_generic(PG_LOG_ERROR, PG_LOG_DETAIL, __VA_ARGS__); \
+        pg_log_generic(PG_LOG_ERROR, PG_LOG_DETAIL, __VA_ARGS__); \
     } while(0)

 #define pg_log_error_hint(...) do { \
-        if (likely(__pg_log_level <= PG_LOG_ERROR)) \
-            pg_log_generic(PG_LOG_ERROR, PG_LOG_HINT, __VA_ARGS__); \
+        pg_log_generic(PG_LOG_ERROR, PG_LOG_HINT, __VA_ARGS__); \
     } while(0)

 #define pg_log_warning(...) do { \
-        if (likely(__pg_log_level <= PG_LOG_WARNING)) \
-            pg_log_generic(PG_LOG_WARNING, PG_LOG_PRIMARY, __VA_ARGS__); \
+        pg_log_generic(PG_LOG_WARNING, PG_LOG_PRIMARY, __VA_ARGS__); \
     } while(0)

 #define pg_log_warning_detail(...) do { \
-        if (likely(__pg_log_level <= PG_LOG_WARNING)) \
-            pg_log_generic(PG_LOG_WARNING, PG_LOG_DETAIL, __VA_ARGS__); \
+        pg_log_generic(PG_LOG_WARNING, PG_LOG_DETAIL, __VA_ARGS__); \
     } while(0)

 #define pg_log_warning_hint(...) do { \
-        if (likely(__pg_log_level <= PG_LOG_WARNING)) \
-            pg_log_generic(PG_LOG_WARNING, PG_LOG_HINT, __VA_ARGS__); \
+        pg_log_generic(PG_LOG_WARNING, PG_LOG_HINT, __VA_ARGS__); \
     } while(0)

 #define pg_log_info(...) do { \
-        if (likely(__pg_log_level <= PG_LOG_INFO)) \
-            pg_log_generic(PG_LOG_INFO, PG_LOG_PRIMARY, __VA_ARGS__); \
+        pg_log_generic(PG_LOG_INFO, PG_LOG_PRIMARY, __VA_ARGS__); \
     } while(0)

 #define pg_log_info_detail(...) do { \
-        if (likely(__pg_log_level <= PG_LOG_INFO)) \
-            pg_log_generic(PG_LOG_INFO, PG_LOG_DETAIL, __VA_ARGS__); \
+        pg_log_generic(PG_LOG_INFO, PG_LOG_DETAIL, __VA_ARGS__); \
     } while(0)

 #define pg_log_info_hint(...) do { \
-        if (likely(__pg_log_level <= PG_LOG_INFO)) \
-            pg_log_generic(PG_LOG_INFO, PG_LOG_HINT, __VA_ARGS__); \
+        pg_log_generic(PG_LOG_INFO, PG_LOG_HINT, __VA_ARGS__); \
     } while(0)

 #define pg_log_debug(...) do { \
@@ -167,8 +158,7 @@ void        pg_log_generic_v(enum pg_log_level level, enum pg_log_part part,
  * A common shortcut: pg_log_error() and immediately exit(1).
  */
 #define pg_fatal(...) do { \
-        if (likely(__pg_log_level <= PG_LOG_ERROR)) \
-            pg_log_generic(PG_LOG_ERROR, PG_LOG_PRIMARY, __VA_ARGS__); \
+        pg_log_generic(PG_LOG_ERROR, PG_LOG_PRIMARY, __VA_ARGS__); \
         exit(1); \
     } while(0)