Hi, On Mon, Mar 30, 2026 at 7:52 PM Andres Freund <[email protected]> wrote: > > I don't see when the overhead of creating an populating the string info ever > matters in these cases. This is optimizing something that never can matter for > real world performance. Even if it were worth optimizing them, I doubt that > the log level check is useful here, because most if not all of these are > logged with a level that's logged in nearly all installations. >
Thank you for the review! I have a few arguments to defend this patch : 1) It is normal practice in the code base to check log level before allocating memory for StringInfo or some other structure. For example, I found the log level check even before a very lightweight piece of code (see xlogutils.c) and even if we are going to emit log with DEBUG3 level (see postmaster.c). 2) Originally, I created this patch in order to avoid spending time during acquiring LWLock before entering the GetLockHoldersAndWaiters function. The log_lock_waits parameter is "true" by default, so even if log level is high, we will *waste* time on the lock acquiring. I.e. this patch is not only about getting rid of redundant memory allocation - it is also about reducing waiting time on the locks. I think that it may be noticeable in conditions of a huge number of parallel processes. 3) I don't think that all other places (except lock.c and proc.c) where I have added log level check are really matter for real world performance. This is more about consistent approach : if we check log level in lock.c, then we should check it everywhere (if it makes sense). Again, it is normal practice for the postgres' code. Am I missing something? BTW, during writing it I noticed that I forgot to add log level check for pretty important code path (proc.c). Please, see it in the v3 patch. -- Best regards, Daniil Davydov
From 5dcfd5b0960fce1f0422a4f73f8b672b69596413 Mon Sep 17 00:00:00 2001 From: Daniil Davidov <[email protected]> Date: Mon, 30 Mar 2026 13:24:03 +0700 Subject: [PATCH v3] Get rid of redundant calculations --- src/backend/replication/logical/conflict.c | 7 +++++-- src/backend/storage/ipc/procarray.c | 3 +++ src/backend/storage/ipc/standby.c | 3 +++ src/backend/storage/lmgr/lock.c | 2 +- src/backend/storage/lmgr/proc.c | 2 +- src/backend/utils/init/postinit.c | 3 ++- 6 files changed, 15 insertions(+), 5 deletions(-) diff --git a/src/backend/replication/logical/conflict.c b/src/backend/replication/logical/conflict.c index ca71a81c7bf..7b676ce1c69 100644 --- a/src/backend/replication/logical/conflict.c +++ b/src/backend/replication/logical/conflict.c @@ -108,6 +108,11 @@ ReportApplyConflict(EState *estate, ResultRelInfo *relinfo, int elevel, Relation localrel = relinfo->ri_RelationDesc; StringInfoData err_detail; + pgstat_report_subscription_conflict(MySubscription->oid, type); + + if (!message_level_is_interesting(elevel)) + return; + initStringInfo(&err_detail); /* Form errdetail message by combining conflicting tuples information. */ @@ -120,8 +125,6 @@ ReportApplyConflict(EState *estate, ResultRelInfo *relinfo, int elevel, conflicttuple->ts, &err_detail); - pgstat_report_subscription_conflict(MySubscription->oid, type); - ereport(elevel, errcode_apply_conflict(type), errmsg("conflict detected on relation \"%s.%s\": conflict=%s", diff --git a/src/backend/storage/ipc/procarray.c b/src/backend/storage/ipc/procarray.c index cc207cb56e3..7e9bfac634f 100644 --- a/src/backend/storage/ipc/procarray.c +++ b/src/backend/storage/ipc/procarray.c @@ -5267,6 +5267,9 @@ KnownAssignedXidsDisplay(int trace_level) tail = pArray->tailKnownAssignedXids; head = pArray->headKnownAssignedXids; + if (!message_level_is_interesting(trace_level)) + return; + initStringInfo(&buf); for (i = tail; i < head; i++) diff --git a/src/backend/storage/ipc/standby.c b/src/backend/storage/ipc/standby.c index de9092fdf5b..2f2c0df7b74 100644 --- a/src/backend/storage/ipc/standby.c +++ b/src/backend/storage/ipc/standby.c @@ -282,6 +282,9 @@ LogRecoveryConflict(RecoveryConflictReason reason, TimestampTz wait_start, StringInfoData buf; int nprocs = 0; + if (!message_level_is_interesting(LOG)) + return; + /* * There must be no conflicting processes when the recovery conflict has * already been resolved. diff --git a/src/backend/storage/lmgr/lock.c b/src/backend/storage/lmgr/lock.c index 234643e4dd7..69dd21f178b 100644 --- a/src/backend/storage/lmgr/lock.c +++ b/src/backend/storage/lmgr/lock.c @@ -1172,7 +1172,7 @@ LockAcquireExtended(const LOCKTAG *locktag, * logLockFailure = true and lock acquisition fails with dontWait * = true */ - if (logLockFailure) + if (logLockFailure && message_level_is_interesting(LOG)) { StringInfoData buf, lock_waiters_sbuf, diff --git a/src/backend/storage/lmgr/proc.c b/src/backend/storage/lmgr/proc.c index 5c47cf13473..67511e8a6fb 100644 --- a/src/backend/storage/lmgr/proc.c +++ b/src/backend/storage/lmgr/proc.c @@ -1570,7 +1570,7 @@ ProcSleep(LOCALLOCK *locallock) if (myWaitStatus == PROC_WAIT_STATUS_OK) pgstat_count_lock_waits(locallock->tag.lock.locktag_type, msecs); - if (log_lock_waits) + if (log_lock_waits && message_level_is_interesting(LOG)) { StringInfoData buf, lock_waiters_sbuf, diff --git a/src/backend/utils/init/postinit.c b/src/backend/utils/init/postinit.c index 26118661f07..1fb00faa978 100644 --- a/src/backend/utils/init/postinit.c +++ b/src/backend/utils/init/postinit.c @@ -269,7 +269,8 @@ PerformAuthentication(Port *port) /* Capture authentication end time for logging */ conn_timing.auth_end = GetCurrentTimestamp(); - if (log_connections & LOG_CONNECTION_AUTHORIZATION) + if ((log_connections & LOG_CONNECTION_AUTHORIZATION) && + message_level_is_interesting(LOG)) { StringInfoData logmsg; -- 2.43.0
