Received: from malur.postgresql.org ([217.196.149.56]) by arkaria.postgresql.org with esmtps (TLS1.3:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.92) (envelope-from ) id 1ncvC7-0001Vj-KL for pgsql-hackers@arkaria.postgresql.org; Fri, 08 Apr 2022 20:26:59 +0000 Received: from localhost ([127.0.0.1] helo=malur.postgresql.org) by malur.postgresql.org with esmtp (Exim 4.92) (envelope-from ) id 1ncvC6-0000oR-GE for pgsql-hackers@arkaria.postgresql.org; Fri, 08 Apr 2022 20:26:58 +0000 Received: from makus.postgresql.org ([2001:4800:3e1:1::229]) by malur.postgresql.org with esmtps (TLS1.3:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.92) (envelope-from ) id 1ncvC6-0000oB-6k for pgsql-hackers@lists.postgresql.org; Fri, 08 Apr 2022 20:26:58 +0000 Received: from sss.pgh.pa.us ([66.207.139.130]) by makus.postgresql.org with esmtps (TLS1.3:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.92) (envelope-from ) id 1ncvC3-0005jn-NH for pgsql-hackers@lists.postgresql.org; Fri, 08 Apr 2022 20:26:57 +0000 Received: from sss1.sss.pgh.pa.us (localhost [127.0.0.1]) by sss.pgh.pa.us (8.15.2/8.15.2) with ESMTP id 238KQnm73993550; Fri, 8 Apr 2022 16:26:50 -0400 From: Tom Lane To: Peter Eisentraut cc: Daniel Gustafsson , Andres Freund , Robert Haas , Michael Paquier , PostgreSQL Hackers Subject: Re: Frontend error logging style In-reply-to: <3949909.1649444725@sss.pgh.pa.us> References: <913687.1645572199@sss.pgh.pa.us> <6fdb3ab3-f093-27b9-dfdc-c391e69163fc@enterprisedb.com> <1346360.1645715441@sss.pgh.pa.us> <1895619.1645809325@sss.pgh.pa.us> <20220322015135.k4tcd3tsjq5xydqa@alap3.anarazel.de> <2446238.1647914437@sss.pgh.pa.us> <20220322021348.pvtvgrseyfohkxob@alap3.anarazel.de> <471299.1648412393@sss.pgh.pa.us> <191BF874-CB92-4F0A-9F0B-414465DE35CB@yesql.se> <116100.1648593510@sss.pgh.pa.us> <61E10C38-D278-4ADC-AD36-4F69CF077957@yesql.se> <3949909.1649444725@sss.pgh.pa.us> Comments: In-reply-to Tom Lane message dated "Fri, 08 Apr 2022 15:05:25 -0400" MIME-Version: 1.0 Content-Type: multipart/mixed; boundary="----- =_aaaaaaaaaa0" Content-ID: <3993482.1649449560.0@sss.pgh.pa.us> Date: Fri, 08 Apr 2022 16:26:49 -0400 Message-ID: <3993549.1649449609@sss.pgh.pa.us> List-Id: List-Help: List-Subscribe: List-Post: List-Owner: List-Archive: Archived-At: Precedence: bulk ------- =_aaaaaaaaaa0 Content-Type: text/plain; charset="us-ascii" Content-ID: <3993482.1649449560.1@sss.pgh.pa.us> 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 ------- =_aaaaaaaaaa0 Content-Type: text/x-diff; name="remove-likely-from-logging-macros.patch"; charset="us-ascii" Content-ID: <3993482.1649449560.2@sss.pgh.pa.us> Content-Description: remove-likely-from-logging-macros.patch Content-Transfer-Encoding: quoted-printable 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 <=3D 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] !=3D '\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 <=3D 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 <=3D 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 <=3D 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 <=3D 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 <=3D 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 <=3D 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 <=3D 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 <=3D 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 <=3D 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 p= g_log_part part, * A common shortcut: pg_log_error() and immediately exit(1). */ #define pg_fatal(...) do { \ - if (likely(__pg_log_level <=3D 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) = ------- =_aaaaaaaaaa0--