public inbox for [email protected]
help / color / mirror / Atom feedFrom: Daniil Davydov <[email protected]>
To: Andres Freund <[email protected]>
Cc: Postgres hackers <[email protected]>
Subject: Re: Get rid of redundant StringInfo accumulation
Date: Mon, 30 Mar 2026 21:24:01 +0700
Message-ID: <CAJDiXgjPGu_cW9nsvciqdgAkqY5o5ugaxDvo3S-rOcqBPcHCPw@mail.gmail.com> (raw)
In-Reply-To: <eok6kgntbec2vo35xp4eky5ef52ttb7trislyev63ty3ltu533@je4bpifl6spb>
References: <CAJDiXgi=Vcp1nh2UoZ5=7BCoR+uXoYzqMsfOfga-XxSUHjFj5Q@mail.gmail.com>
<eok6kgntbec2vo35xp4eky5ef52ttb7trislyev63ty3ltu533@je4bpifl6spb>
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
Attachments:
[text/x-patch] v3-0001-Get-rid-of-redundant-calculations.patch (4.0K, 2-v3-0001-Get-rid-of-redundant-calculations.patch)
download | inline diff:
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
view thread (9+ 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]
Subject: Re: Get rid of redundant StringInfo accumulation
In-Reply-To: <CAJDiXgjPGu_cW9nsvciqdgAkqY5o5ugaxDvo3S-rOcqBPcHCPw@mail.gmail.com>
* 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