public inbox for [email protected]
help / color / mirror / Atom feedFrom: Tom Lane <[email protected]>
To: Peter Eisentraut <[email protected]>
Cc: Daniel Gustafsson <[email protected]>
Cc: Andres Freund <[email protected]>
Cc: Robert Haas <[email protected]>
Cc: Michael Paquier <[email protected]>
Cc: PostgreSQL Hackers <[email protected]>
Subject: Re: Frontend error logging style
Date: Fri, 08 Apr 2022 16:26:49 -0400
Message-ID: <[email protected]> (raw)
In-Reply-To: <[email protected]>
References: <YZMfMI3kTDw/[email protected]>
<CA+TgmoZy84orCfUNipEp+QzWwRuR2yrjsdcJN9ROHzPkXDRbxQ@mail.gmail.com>
<[email protected]>
<CA+TgmoYaA1wtU91yNg1_C6oS8azAfp5dQ1YrfuNAm2=Yefrn1A@mail.gmail.com>
<[email protected]>
<[email protected]>
<[email protected]>
<[email protected]>
<[email protected]>
<[email protected]>
<[email protected]>
<[email protected]>
<[email protected]>
<[email protected]>
<[email protected]>
<[email protected]>
<[email protected]>
<[email protected]>
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
Attachments:
[text/x-diff] remove-likely-from-logging-macros.patch (3.8K, ../[email protected]/2-remove-likely-from-logging-macros.patch)
download | inline diff:
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)
view thread (23+ messages) latest in thread
reply
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: [email protected]
Cc: [email protected], [email protected], [email protected], [email protected], [email protected], [email protected], [email protected]
Subject: Re: Frontend error logging style
In-Reply-To: <[email protected]>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox