public inbox for [email protected]
help / color / mirror / Atom feed[PATCH] Fix memory leak in pgstat_progress_parallel_incr_param()
6+ messages / 4 participants
[nested] [flat]
* [PATCH] Fix memory leak in pgstat_progress_parallel_incr_param()
@ 2026-06-05 17:44 Baji Shaik <[email protected]>
0 siblings, 1 reply; 6+ messages in thread
From: Baji Shaik @ 2026-06-05 17:44 UTC (permalink / raw)
To: [email protected]; +Cc: [email protected]
Hi,
While running parallel vacuum with track_cost_delay_timing=on, I
noticed memory in the parallel worker process keeps growing
proportionally to vacuum runtime, and is never reclaimed until the
worker exits.
I think pgstat_progress_parallel_incr_param() (backend_progress.c)
leaks memory on every call from a parallel worker.
The suspected block:
static StringInfoData progress_message;
initStringInfo(&progress_message); /* palloc -> A */
pq_beginmessage(&progress_message, PqMsg_Progress);
/* pq_beginmessage internally calls initStringInfo again ->
palloc -> B, A is orphaned */
pq_sendint32(&progress_message, index);
pq_sendint64(&progress_message, incr);
pq_endmessage(&progress_message); /* pfree(B), A leaked
*/
So one palloc(~1 kB) leaks per call, into the per-worker context.
This is an oversight of f1889729dd3 ("Add new parallel message type
to progress reporting"); track_cost_delay_timing just makes it more
visible. With that GUC enabled, a long-running parallel vacuum leaks
megabytes per worker (~232 MB observed in a 43-min vacuum at default
settings on a 15M-row, 30-index workload).
The proposed fix is in the attached patch which does a one-time init of the
static
buffer, then pq_beginmessage_reuse() / pq_endmessage_reuse() so the
buffer is allocated once and reused.
All 245 regression tests pass.
Thanks,
Baji Shaik.
Attachments:
[application/octet-stream] 0001-Fix-memory-leak-in-pgstat_progress_parallel_incr_par.patch (2.4K, 3-0001-Fix-memory-leak-in-pgstat_progress_parallel_incr_par.patch)
download | inline diff:
From 590712b4fea381a82cfaa9aa17bbe0f07ca0c16a Mon Sep 17 00:00:00 2001
From: Baji Shaik <[email protected]>
Date: Fri, 5 Jun 2026 12:40:18 -0500
Subject: [PATCH] Fix memory leak in pgstat_progress_parallel_incr_param()
When called from a parallel worker, pgstat_progress_parallel_incr_param()
calls initStringInfo() on a static StringInfoData and then immediately
calls pq_beginmessage(), which calls initStringInfo() again. The second
call overwrites buf->data with a freshly palloc'd buffer, orphaning the
first one. pq_endmessage() then frees only the second buffer, so each
call leaks ~1 kB into the per-worker memory context.
The leak is negligible by default, but with track_cost_delay_timing
enabled the parallel cost-delay reporting path fires once per second per
worker, so a long-running parallel vacuum leaks megabytes per worker.
Fix by initializing the static buffer once per process and using
pq_beginmessage_reuse()/pq_endmessage_reuse(), so the buffer is allocated
once and reused.
This is an oversight of f1889729dd3 ("Add new parallel message type to
progress reporting"); track_cost_delay_timing just makes it more visible.
Author: Baji Shaik <[email protected]>
---
src/backend/utils/activity/backend_progress.c | 20 ++++++++++++++-----
1 file changed, 15 insertions(+), 5 deletions(-)
diff --git a/src/backend/utils/activity/backend_progress.c b/src/backend/utils/activity/backend_progress.c
index b0359771de5..0483741a80e 100644
--- a/src/backend/utils/activity/backend_progress.c
+++ b/src/backend/utils/activity/backend_progress.c
@@ -99,13 +99,23 @@ pgstat_progress_parallel_incr_param(int index, int64 incr)
if (IsParallelWorker())
{
static StringInfoData progress_message;
-
- initStringInfo(&progress_message);
-
- pq_beginmessage(&progress_message, PqMsg_Progress);
+ static bool progress_message_initialized = false;
+
+ /*
+ * Initialize the message buffer once per process; pq_beginmessage_reuse()
+ * and pq_endmessage_reuse() reset and reuse it on each call to avoid
+ * palloc overhead.
+ */
+ if (!progress_message_initialized)
+ {
+ initStringInfo(&progress_message);
+ progress_message_initialized = true;
+ }
+
+ pq_beginmessage_reuse(&progress_message, PqMsg_Progress);
pq_sendint32(&progress_message, index);
pq_sendint64(&progress_message, incr);
- pq_endmessage(&progress_message);
+ pq_endmessage_reuse(&progress_message);
}
else
pgstat_progress_incr_param(index, incr);
--
2.50.1 (Apple Git-155)
^ permalink raw reply [nested|flat] 6+ messages in thread
* Re: [PATCH] Fix memory leak in pgstat_progress_parallel_incr_param()
@ 2026-06-05 21:29 Tristan Partin <[email protected]>
parent: Baji Shaik <[email protected]>
0 siblings, 1 reply; 6+ messages in thread
From: Tristan Partin @ 2026-06-05 21:29 UTC (permalink / raw)
To: Baji Shaik <[email protected]>; +Cc: [email protected]; [email protected]
On Fri Jun 5, 2026 at 5:44 PM UTC, Baji Shaik wrote:
> Hi,
>
> While running parallel vacuum with track_cost_delay_timing=on, I
> noticed memory in the parallel worker process keeps growing
> proportionally to vacuum runtime, and is never reclaimed until the
> worker exits.
>
> I think pgstat_progress_parallel_incr_param() (backend_progress.c)
> leaks memory on every call from a parallel worker.
>
> The suspected block:
>
> static StringInfoData progress_message;
> initStringInfo(&progress_message); /* palloc -> A */
> pq_beginmessage(&progress_message, PqMsg_Progress);
> /* pq_beginmessage internally calls initStringInfo again ->
> palloc -> B, A is orphaned */
> pq_sendint32(&progress_message, index);
> pq_sendint64(&progress_message, incr);
> pq_endmessage(&progress_message); /* pfree(B), A leaked
> */
>
> So one palloc(~1 kB) leaks per call, into the per-worker context.
>
> This is an oversight of f1889729dd3 ("Add new parallel message type
> to progress reporting"); track_cost_delay_timing just makes it more
> visible. With that GUC enabled, a long-running parallel vacuum leaks
> megabytes per worker (~232 MB observed in a 43-min vacuum at default
> settings on a 15M-row, 30-index workload).
>
> The proposed fix is in the attached patch which does a one-time init of the
> static
> buffer, then pq_beginmessage_reuse() / pq_endmessage_reuse() so the
> buffer is allocated once and reused.
Hey Baji,
This looks pretty reasonable to me. Nice find. Did you think about
keeping the code path as is and just removing the first initStringInfo()
call? Removing the allocation per progress message seems like a good
idea to me. Maybe you could separate this change into two patches. One
to fix the memory leak and another to remove the allocation per message.
A committer could then decide for themselves if the second patch is
worth committing.
--
Tristan Partin
PostgreSQL Contributors Team
AWS (https://aws.amazon.com)
^ permalink raw reply [nested|flat] 6+ messages in thread
* Re: [PATCH] Fix memory leak in pgstat_progress_parallel_incr_param()
@ 2026-06-05 23:44 Baji Shaik <[email protected]>
parent: Tristan Partin <[email protected]>
0 siblings, 1 reply; 6+ messages in thread
From: Baji Shaik @ 2026-06-05 23:44 UTC (permalink / raw)
To: Tristan Partin <[email protected]>; +Cc: [email protected]; [email protected]
On Fri, Jun 5, 2026 at 4:29 PM Tristan Partin <[email protected]> wrote:
> This looks pretty reasonable to me. Nice find. Did you think about
> keeping the code path as is and just removing the first initStringInfo()
> call? Removing the allocation per progress message seems like a good
> idea to me. Maybe you could separate this change into two patches. One
> to fix the memory leak and another to remove the allocation per message.
> A committer could then decide for themselves if the second patch is
> worth committing.
>
Thank you for the review. I hadn't thought of splitting it, but it's
a good idea. I see f1889729dd3 itself is in PG17+, so the bug fix is
a backport candidate independently of the PG19 caller bb8dff9995f.
Patches attached:
0001: drop the redundant initStringInfo() call (backport candidate)
0002: allocate the static buffer once per process via
pq_beginmessage_reuse / pq_endmessage_reuse, to avoid the
per-call allocation (master only)
Thanks,
Baji Shaik
Attachments:
[application/octet-stream] 0001-Fix-memory-leak-in-pgstat_progress_parallel_incr_par.patch (1.5K, 3-0001-Fix-memory-leak-in-pgstat_progress_parallel_incr_par.patch)
download | inline diff:
From c7972bc8ee34ec111440bf948c0cebe476fd8cdb Mon Sep 17 00:00:00 2001
From: Baji Shaik <[email protected]>
Date: Fri, 5 Jun 2026 18:41:47 -0500
Subject: [PATCH 1/2] Fix memory leak in pgstat_progress_parallel_incr_param()
When called from a parallel worker, pgstat_progress_parallel_incr_param()
calls initStringInfo() on a static StringInfoData and then immediately
calls pq_beginmessage(), which calls initStringInfo() again. The second
call overwrites buf->data with a freshly palloc'd buffer, orphaning the
first one. pq_endmessage() then frees only the second buffer, so each
call leaks ~1 kB into the per-worker memory context.
Fix by removing the redundant initStringInfo() call.
Oversight of f1889729dd3 ("Add new parallel message type to progress
reporting").
Author: Baji Shaik <[email protected]>
---
src/backend/utils/activity/backend_progress.c | 2 --
1 file changed, 2 deletions(-)
diff --git a/src/backend/utils/activity/backend_progress.c b/src/backend/utils/activity/backend_progress.c
index b0359771de5..6d2049105ab 100644
--- a/src/backend/utils/activity/backend_progress.c
+++ b/src/backend/utils/activity/backend_progress.c
@@ -100,8 +100,6 @@ pgstat_progress_parallel_incr_param(int index, int64 incr)
{
static StringInfoData progress_message;
- initStringInfo(&progress_message);
-
pq_beginmessage(&progress_message, PqMsg_Progress);
pq_sendint32(&progress_message, index);
pq_sendint64(&progress_message, incr);
--
2.50.1 (Apple Git-155)
[application/octet-stream] 0002-Allocate-progress-message-buffer-once-per-parallel-w.patch (2.1K, 4-0002-Allocate-progress-message-buffer-once-per-parallel-w.patch)
download | inline diff:
From 94e1cd63954616f81953950aaf712fa2acf78913 Mon Sep 17 00:00:00 2001
From: Baji Shaik <[email protected]>
Date: Fri, 5 Jun 2026 18:42:08 -0500
Subject: [PATCH 2/2] Allocate progress message buffer once per parallel worker
The static StringInfoData declared in pgstat_progress_parallel_incr_param()
was clearly intended to be allocated once per process and reused across
calls, but the buggy double-initStringInfo() pattern (fixed in the
previous commit) defeated that intent: each call still allocated and
freed a fresh buffer via pq_beginmessage() / pq_endmessage().
Restore the original intent by initializing the static buffer only on
the first call and switching to pq_beginmessage_reuse() and
pq_endmessage_reuse(), which reset and reuse the existing buffer
without (re)allocating.
Author: Baji Shaik <[email protected]>
---
src/backend/utils/activity/backend_progress.c | 18 +++++++++++++++---
1 file changed, 15 insertions(+), 3 deletions(-)
diff --git a/src/backend/utils/activity/backend_progress.c b/src/backend/utils/activity/backend_progress.c
index 6d2049105ab..0483741a80e 100644
--- a/src/backend/utils/activity/backend_progress.c
+++ b/src/backend/utils/activity/backend_progress.c
@@ -99,11 +99,23 @@ pgstat_progress_parallel_incr_param(int index, int64 incr)
if (IsParallelWorker())
{
static StringInfoData progress_message;
-
- pq_beginmessage(&progress_message, PqMsg_Progress);
+ static bool progress_message_initialized = false;
+
+ /*
+ * Initialize the message buffer once per process; pq_beginmessage_reuse()
+ * and pq_endmessage_reuse() reset and reuse it on each call to avoid
+ * palloc overhead.
+ */
+ if (!progress_message_initialized)
+ {
+ initStringInfo(&progress_message);
+ progress_message_initialized = true;
+ }
+
+ pq_beginmessage_reuse(&progress_message, PqMsg_Progress);
pq_sendint32(&progress_message, index);
pq_sendint64(&progress_message, incr);
- pq_endmessage(&progress_message);
+ pq_endmessage_reuse(&progress_message);
}
else
pgstat_progress_incr_param(index, incr);
--
2.50.1 (Apple Git-155)
^ permalink raw reply [nested|flat] 6+ messages in thread
* Re: [PATCH] Fix memory leak in pgstat_progress_parallel_incr_param()
@ 2026-06-06 14:55 Sami Imseih <[email protected]>
parent: Baji Shaik <[email protected]>
0 siblings, 2 replies; 6+ messages in thread
From: Sami Imseih @ 2026-06-06 14:55 UTC (permalink / raw)
To: Baji Shaik <[email protected]>; +Cc: Tristan Partin <[email protected]>; [email protected]; [email protected]
Hi,
good find, and thanks for the patches!
> 0001: drop the redundant initStringInfo() call (backport candidate)
This one looks like an obvious fix to me.
> 0002: allocate the static buffer once per process via
> pq_beginmessage_reuse / pq_endmessage_reuse, to avoid the
> per-call allocation (master only)
I am less convinced this will have any benefits for the additional complexity.
The callers of pgstat_progress_parallel_incr_param() are not frequent enough
to make a measurable difference here. cost delay reporting for parallel workers
is throttled by PARALLEL_VACUUM_DELAY_REPORT_INTERVAL_NS and
index progress reporting does not happen very frequently either.
--
Sami Imseih
Amazon Web Services (AWS)
^ permalink raw reply [nested|flat] 6+ messages in thread
* Re: [PATCH] Fix memory leak in pgstat_progress_parallel_incr_param()
@ 2026-06-06 16:04 Baji Shaik <[email protected]>
parent: Sami Imseih <[email protected]>
1 sibling, 0 replies; 6+ messages in thread
From: Baji Shaik @ 2026-06-06 16:04 UTC (permalink / raw)
To: Sami Imseih <[email protected]>; +Cc: Tristan Partin <[email protected]>; [email protected]; [email protected]
On Sat, Jun 6, 2026 at 9:55 AM Sami Imseih <[email protected]> wrote:
> > 0002: allocate the static buffer once per process via
> > pq_beginmessage_reuse / pq_endmessage_reuse, to avoid the
> > per-call allocation (master only)
>
> I am less convinced this will have any benefits for the additional
> complexity.
> The callers of pgstat_progress_parallel_incr_param() are not frequent
> enough
> to make a measurable difference here. cost delay reporting for parallel
> workers
> is throttled by PARALLEL_VACUUM_DELAY_REPORT_INTERVAL_NS and
> index progress reporting does not happen very frequently either.
Thanks for the review, Sami.
Agreed, at those call frequencies the per-call palloc is not worth
the added complexity. Dropping 0002.
v3 attached (just 0001, unchanged from previous).
Thanks,
Baji Shaik.
Attachments:
[application/octet-stream] v3-0001-Fix-memory-leak-in-pgstat_progress_parallel_incr_.patch (1.5K, 3-v3-0001-Fix-memory-leak-in-pgstat_progress_parallel_incr_.patch)
download | inline diff:
From 507f4fc9030bcb4708ff29d04fcd715463396ccb Mon Sep 17 00:00:00 2001
From: Baji Shaik <[email protected]>
Date: Sat, 6 Jun 2026 10:57:54 -0500
Subject: [PATCH v3] Fix memory leak in pgstat_progress_parallel_incr_param()
When called from a parallel worker, pgstat_progress_parallel_incr_param()
calls initStringInfo() on a static StringInfoData and then immediately
calls pq_beginmessage(), which calls initStringInfo() again. The second
call overwrites buf->data with a freshly palloc'd buffer, orphaning the
first one. pq_endmessage() then frees only the second buffer, so each
call leaks ~1 kB into the per-worker memory context.
Fix by removing the redundant initStringInfo() call.
Oversight of f1889729dd3 ("Add new parallel message type to progress
reporting").
Author: Baji Shaik <[email protected]>
---
src/backend/utils/activity/backend_progress.c | 2 --
1 file changed, 2 deletions(-)
diff --git a/src/backend/utils/activity/backend_progress.c b/src/backend/utils/activity/backend_progress.c
index b0359771de5..6d2049105ab 100644
--- a/src/backend/utils/activity/backend_progress.c
+++ b/src/backend/utils/activity/backend_progress.c
@@ -100,8 +100,6 @@ pgstat_progress_parallel_incr_param(int index, int64 incr)
{
static StringInfoData progress_message;
- initStringInfo(&progress_message);
-
pq_beginmessage(&progress_message, PqMsg_Progress);
pq_sendint32(&progress_message, index);
pq_sendint64(&progress_message, incr);
--
2.50.1 (Apple Git-155)
^ permalink raw reply [nested|flat] 6+ messages in thread
* Re: [PATCH] Fix memory leak in pgstat_progress_parallel_incr_param()
@ 2026-06-08 03:44 Michael Paquier <[email protected]>
parent: Sami Imseih <[email protected]>
1 sibling, 0 replies; 6+ messages in thread
From: Michael Paquier @ 2026-06-08 03:44 UTC (permalink / raw)
To: Sami Imseih <[email protected]>; +Cc: Baji Shaik <[email protected]>; Tristan Partin <[email protected]>; [email protected]; [email protected]
On Sat, Jun 06, 2026 at 09:55:05AM -0500, Sami Imseih wrote:
> Hi,
>
> good find, and thanks for the patches!
>
> > 0001: drop the redundant initStringInfo() call (backport candidate)
>
> This one looks like an obvious fix to me.
And clearly something that should be backpatched down to v17, or we
could pile a lot of memory depending on how many calls we do in a
worker, with more piling over time.
Will process, thanks!
>> 0002: allocate the static buffer once per process via
>> pq_beginmessage_reuse / pq_endmessage_reuse, to avoid the
>> per-call allocation (master only)
>
> I am less convinced this will have any benefits for the additional complexity.
> The callers of pgstat_progress_parallel_incr_param() are not frequent enough
> to make a measurable difference here. cost delay reporting for parallel workers
> is throttled by PARALLEL_VACUUM_DELAY_REPORT_INTERVAL_NS and
> index progress reporting does not happen very frequently either.
I doubt that 0002 is worth doing, particularly seeing the code paths
where this is called.
--
Michael
Attachments:
[application/pgp-signature] signature.asc (833B, 2-signature.asc)
download
^ permalink raw reply [nested|flat] 6+ messages in thread
end of thread, other threads:[~2026-06-08 03:44 UTC | newest]
Thread overview: 6+ messages (download: mbox mbox.gz follow: Atom feed)
-- links below jump to the message on this page --
2026-06-05 17:44 [PATCH] Fix memory leak in pgstat_progress_parallel_incr_param() Baji Shaik <[email protected]>
2026-06-05 21:29 ` Tristan Partin <[email protected]>
2026-06-05 23:44 ` Baji Shaik <[email protected]>
2026-06-06 14:55 ` Sami Imseih <[email protected]>
2026-06-06 16:04 ` Baji Shaik <[email protected]>
2026-06-08 03:44 ` Michael Paquier <[email protected]>
This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox