Subject: Re: Bound memory usage during manual slot sync retries

From: Amit Kapila <[email-masked]>
Date: Mon, 25 May 2026 19:03:48 -0700

On Sat, May 16, 2026 at 4:35 AM Xuneng Zhou <[email-masked]> wrote:
>
> On Fri, May 15, 2026 at 9:20 PM Xuneng Zhou <[email-masked]> wrote:
> >
> > Hi Amit,
> >
> > On Fri, May 15, 2026 at 5:23 PM Amit Kapila <[email-masked]> wrote:
> > >
> > > On Fri, May 15, 2026 at 11:02 AM Xuneng Zhou <[email-masked]> wrote:
> > > >
> > > > pg_sync_replication_slots() now retries inside a single SQL function
> > > > call.  Unlike the slotsync worker, it does not get a transaction boundary
> > > > between retry cycles, so allocations made while fetching and synchronizing
> > > > remote slots can accumulate until the function returns.
> > > >
> > > > The existing list_free_deep(remote_slots) is not enough to bound this.
> > > > It frees the List cells and the RemoteSlot structs stored as list
> > > > elements, but it does not recursively free allocations owned by those
> > > > structs, such as the copied slot name, plugin name, and database name.
> > > >
> > >
> > > Right.
> > >
> > > > It also does not release unrelated per-cycle allocations made while
> > > > fetching and processing the remote slots.
> > > >
> > >
> > > BTW, did you notice via test or otherwise, what and how much other
> > > unrelated memory is being allocated during each sync cycle if we
> > > manually free the allocations you observed? This is mainly to learn
> > > the impact of not doing all these allocations in some short-duration
> > > memory context.
> >
> > I noticed this by reading the feature code while walking through the
> > PG 19 release notes, not by observing actual memory growth in a
> > running system. Besides the RemoteSlot field strings, there seems to
> > be a few smaller per-cycle allocations that also accumulate:
> > quote_literal_cstr() strings used to build the filtered query, a
> > temporary TextDatumGetCString() result for invalidation_reason, the
> > standalone TupleTableSlot in fetch_remote_slots(), and the list
> > container built by get_local_synced_slots(). I chose a per-cycle
> > memory context over manual pfrees to make the retry-cycle lifetime
> > explicit and avoid maintaining a destructor for every current and
> > future allocation in this path. It may be worth measuring how much
> > extra memory actually accumulates during an extended wait to confirm
> > the practical impact.
>
> I did some measurements for the memory growth in the manual
> pg_sync_replication_slots() retry path.
>
> The test used 100 failover logical slots and forced
> pg_sync_replication_slots() to keep retrying on the standby. Memory
> was sampled with pg_log_backend_memory_contexts() on the backend
> running the function.
>
> On HEAD, the short run showed:
> timestamp total_bytes delta
> 2026-05-16T10:47:54Z,63422,1339920,
> 2026-05-16T10:47:56Z,63422,1405456,65536
> 2026-05-16T10:47:59Z,63422,1405456,0
> 2026-05-16T10:48:01Z,63422,1405456,0
> 2026-05-16T10:48:03Z,63422,1536528,131072
> 2026-05-16T10:48:05Z,63422,1536528,0
>
> So the total increase was about 192 KiB.
>
> After adding a targeted cleanup for the copied RemoteSlot strings, the
> same test showed:
>
> timestamp total_bytes delta
> 2026-05-16T11:04:58Z,77000,1339920,
> 2026-05-16T11:05:00Z,77000,1339920,0
> 2026-05-16T11:05:02Z,77000,1405456,65536
> 2026-05-16T11:05:04Z,77000,1405456,0
>
> So the increase dropped to about 64 KiB.
>
> With a per-retry memory context around the fetch/synchronize cycle,
> the same test stayed flat:
>
> 2026-05-16T11:09:10Z,84600,1315344,
> 2026-05-16T11:09:12Z,84600,1315344,0
> 2026-05-16T11:09:14Z,84600,1315344,0
> 2026-05-16T11:09:16Z,84600,1315344,0
> 2026-05-16T11:09:18Z,84600,1315344,0
> 2026-05-16T11:09:20Z,84600,1315344,0
>
> Looking at the memory-context dumps, the growth on HEAD was visible
> under ExprContext. The grand-total increase matched the ExprContex
> increase, which is consistent with retry-cycle allocations surviving
> for the duration of the single SQL function call.
>
> That said, the practical severity looks small. This is mainly because
> the retry loop is not a tight loop. With no slot activity,
> wait_for_slot_activity() doubles the sleep time up to 30 seconds.
>
> So after about 51 seconds it retries only about twice per minute. For
> 100 slots and no slot activity, a rough 1-hour test from the short
> runs is on the order of a few MB on HEAD, and around 1 MB with the
> manual RemoteSlot cleanup. For installations with fewer slots, it
> should be smaller.
>
> So my read is:
> the accumulation is real;
> it is modest because of the retry backoff;
> manually freeing RemoteSlot’s copied strings removes most of the
> observed growth;
> a per-retry memory context fully bounds the retry-cycle lifetime
>

Okay, then let's go with a per-retry memory context approach.

@@ -579,6 +579,8 @@ drop_local_obsolete_slots(List *remote_slot_list)
     local_slot->data.database));
  }
  }
+
+ list_free(local_slots);
 }

Why do we need this retail pfree if the caller is using memory context?

-- 
With Regards,
Amit Kapila.

---

From: Amit Kapila <[email-masked]>
Date: Tue, 26 May 2026 09:40:19 -0700

> Why do we need this retail pfree if the caller is using memory context?

I see that the latest patch in email [1] has already addressed this
point. So, I'll push the v2 version.

[1] - https://www.postgresql.org/message-id/CABPTF7WB4Z62sPoZkhSygOCAo3OiTDLpMELxZDuwCb3HYgM_pQ%40mail.gmail.com

-- 
With Regards,
Amit Kapila.

---

From: Xuneng Zhou <[email-masked]>
Date: Wed, 27 May 2026 04:01:09 +0800

> [1] -
> https://www.postgresql.org/message-id/CABPTF7WB4Z62sPoZkhSygOCAo3OiTDLpMELxZDuwCb3HYgM_pQ%40mail.gmail.com

Thanks. My original reasoning for adding the pfree here was to act as a
safety guard in case other future callers might not manage the memory
properly. But Zhijie pointed out that this double-free pattern is not
favored in previous community discussions. I'll post the  worst-case test
and its results later.

Regards,
Xuneng Zhou
HighGo Software Co., Ltd.

---

From: Amit Kapila <[email-masked]>
Date: Tue, 26 May 2026 16:39:34 -0700

On Tue, May 26, 2026 at 1:01 PM Xuneng Zhou <[email-masked]> wrote:
> Thanks. My original reasoning for adding the pfree here was to act as a safety guard in case other future callers might not manage the memory properly. But Zhijie pointed out that this double-free pattern is not favored in previous community discussions. I'll post the  worst-case test and its results later.
>

Pushed.

-- 
With Regards,
Amit Kapila.

---

From: Khoa Nguyen <[email-masked]>
Date: Thu, 28 May 2026 09:42:54 -0700

> > Thanks. My original reasoning for adding the pfree here was to act as a safety guard in case other future callers might not manage the memory properly. But Zhijie pointed out that this double-fre…

This patch is already pushed by Amit but I wanted to add my review
from a validation standpoint.  I was able to use the
measure_slotsync_memory.sh to verify the leak that Xuneng reported.
pre-patch
timestamp,backend_pid,total_bytes,delta_bytes
2026-05-27T16:52:20Z,78800,1339920,
2026-05-27T16:52:22Z,78800,1405456,65536
2026-05-27T16:52:24Z,78800,1405456,0
2026-05-27T16:52:26Z,78800,1405456,0
2026-05-27T16:52:28Z,78800,1536528,131072
2026-05-27T16:52:30Z,78800,1536528,0
2026-05-27T16:52:32Z,78800,1536528,0

patched
timestamp,backend_pid,total_bytes,delta_bytes
2026-05-27T01:17:50Z,66917,1315344,
2026-05-27T01:17:52Z,66917,1315344,0
2026-05-27T01:17:54Z,66917,1315344,0
2026-05-27T01:17:56Z,66917,1315344,0
2026-05-27T01:17:58Z,66917,1315344,0
2026-05-27T01:18:00Z,66917,1315344,0
2026-05-27T01:18:02Z,66917,1315344,0

I also reviewed the script and it is well done.  The script setup and
capture data points showing memory leak within the session over time.
The patch looks fine though I think that oldctx should be captured
once outside the retry loop since the current logic is more about
parent and child memory context rather than previous and current
context.  Nevertheless, the current code works as well.

---

From: Xuneng Zhou <[email-masked]>
Date: Fri, 29 May 2026 10:30:57 +0800

Hi Khoa,

> context.  Nevertheless, the current code works as well.

Thanks for your review and verification. I agree with your suggestion
that capturing the outer memory context once outside the retry loop
looks cleaner and reflect the lifetime model better. I am also
wondering whether renaming the oldctx to outerctx could express the
parent/child relationship more clearly. That said, I am ok with the
status quo, if Amit thinks no further modification is needed.

---

From: "Zhijie Hou (Fujitsu)" <[email-masked]>
Date: Fri, 29 May 2026 02:59:54 +0000

> status quo, if Amit thinks no further modification is needed.

I am personally -1 on this change. The patch makes the logic somewhat harder for
me to follow and understand. IIUC, the patch assumes that all code executed
after creating sync_retry_ctx should fall under the 'outerctx'. This assumption
seems fragile as it could easily be broken if someone creates another temp
context and places the entire loop logic inside that context.

Best Regards,
Hou zj

---

From: Xuneng Zhou <[email-masked]>
Date: Fri, 29 May 2026 15:29:07 +0800

> context and places the entire loop logic inside that context.

Both patterns exist in the tree. I've studied the use of them a bit.
My current takeaway is that the fixed-context is preferred when the
saved context has a named semantic role like result, caller context
and the code switches to it only for allocations that must live there.
The slotsyc case does not meet these conditions. Its semantic rule is
that slot_names must survive sysnc_retry_ctx resets. It does not
specifically need to live in "the context active before sync_retry_ctx
was created." So I am also inclined to keep the status quo.