Bound memory usage during manual slot sync retries

On Fri, May 15, 2026 at 11:03 AM Xuneng Zhou <xunengzhou@gmail.com> wrote:

Hi Hackers,

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.
It also does not release unrelated per-cycle allocations made while
fetching and processing the remote slots.

This is probably modest in normal cases, as the retained memory is roughly
proportional to the number of retries times the number of remote slots.
Still, the function can wait for a long time on a lagging or
misconfigured standby, so the growth is unbounded for that call.

The attached patch runs each manual retry cycle in a short-lived memory
context and resets it before the next attempt. The slot name list needed
across retries is copied outside that context.

It also adds two local cleanups. Tuple slots created with
MakeSingleTupleTableSlot() are explicitly released with
ExecDropSingleTupleTableSlot() before clearing the walreceiver result.
The memory context would reclaim the storage eventually, but the slot also
holds a reference to the result tuple descriptor, so releasing it at the
matching ownership boundary seems clearer and follows nearby code.

drop_local_obsolete_slots() now frees the temporary list container it
builds. The retry-cycle context would reclaim this list too, but freeing
it locally would make the helper self-contained.

Politely CCed the original authors.

I like the core idea of this patch. It makes the flow cleaner and
protects the flow from memory-leaks when dealing with a high number of
slots. I think during implementation, we considered having a separate
memory context, but since we were freeing the remote_slots then, we
thought allocating a new memory context might not be required. But on
re-thinking and reading the details above, IMO, it is a good
improvement. But let's see what others have to say.

A couple of minor comments:

1. I think we need to delete the new memory context in
slotsync_failure_callback() as well.

2. Also, it will be good to add a comment before switiching to
old-context before extract_slot_names(). Or better we can move the
swicthing inside extract_slot_names() with a comment.

thanks
Shveta

On Friday, May 15, 2026 2:36 PM shveta malik <shveta.malik@gmail.com> wrote:

On Fri, May 15, 2026 at 11:03 AM Xuneng Zhou <xunengzhou@gmail.com>
wrote:

Hi Hackers,

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.
It also does not release unrelated per-cycle allocations made while
fetching and processing the remote slots.

This is probably modest in normal cases, as the retained memory is
roughly proportional to the number of retries times the number of remote

slots.

Still, the function can wait for a long time on a lagging or
misconfigured standby, so the growth is unbounded for that call.

The attached patch runs each manual retry cycle in a short-lived
memory context and resets it before the next attempt. The slot name
list needed across retries is copied outside that context.

It also adds two local cleanups. Tuple slots created with
MakeSingleTupleTableSlot() are explicitly released with
ExecDropSingleTupleTableSlot() before clearing the walreceiver result.
The memory context would reclaim the storage eventually, but the slot
also holds a reference to the result tuple descriptor, so releasing it
at the matching ownership boundary seems clearer and follows nearby

code.

drop_local_obsolete_slots() now frees the temporary list container it
builds. The retry-cycle context would reclaim this list too, but
freeing it locally would make the helper self-contained.

Politely CCed the original authors.

I like the core idea of this patch. It makes the flow cleaner and protects the
flow from memory-leaks when dealing with a high number of slots. I think
during implementation, we considered having a separate memory context,
but since we were freeing the remote_slots then, we thought allocating a new
memory context might not be required. But on re-thinking and reading the
details above, IMO, it is a good improvement.

+1 to the general idea of avoiding memory accumulation.

A couple of minor comments:

1. I think we need to delete the new memory context in
slotsync_failure_callback() as well.

I think we can avoid doing that, because the new memory context should be
destroyed along with its parent context on transaction abort (if any error
occurs).

~~~

Just one comment:

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

I think we prefer to avoid freeing memory explicitly, since this is a
static function and we already have a memory context in place to prevent leaks.
(We've seen this discussion about explicit freeing multiple times before, and
the previous conclusion was to rely on the memory context management rather than
adding more free code.)

Best Regards,
Hou zj

On Fri, May 15, 2026 at 11:02 AM Xuneng Zhou <xunengzhou@gmail.com> 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.

--
With Regards,
Amit Kapila.

Hi Shveta, Zhijie,

On Fri, May 15, 2026 at 4:41 PM Zhijie Hou (Fujitsu)
<houzj.fnst@fujitsu.com> wrote:

On Friday, May 15, 2026 2:36 PM shveta malik <shveta.malik@gmail.com> wrote:

On Fri, May 15, 2026 at 11:03 AM Xuneng Zhou <xunengzhou@gmail.com>
wrote:

Hi Hackers,

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.
It also does not release unrelated per-cycle allocations made while
fetching and processing the remote slots.

This is probably modest in normal cases, as the retained memory is
roughly proportional to the number of retries times the number of remote

slots.

Still, the function can wait for a long time on a lagging or
misconfigured standby, so the growth is unbounded for that call.

The attached patch runs each manual retry cycle in a short-lived
memory context and resets it before the next attempt. The slot name
list needed across retries is copied outside that context.

It also adds two local cleanups. Tuple slots created with
MakeSingleTupleTableSlot() are explicitly released with
ExecDropSingleTupleTableSlot() before clearing the walreceiver result.
The memory context would reclaim the storage eventually, but the slot
also holds a reference to the result tuple descriptor, so releasing it
at the matching ownership boundary seems clearer and follows nearby

code.

drop_local_obsolete_slots() now frees the temporary list container it
builds. The retry-cycle context would reclaim this list too, but
freeing it locally would make the helper self-contained.

Politely CCed the original authors.

I like the core idea of this patch. It makes the flow cleaner and protects the
flow from memory-leaks when dealing with a high number of slots. I think
during implementation, we considered having a separate memory context,
but since we were freeing the remote_slots then, we thought allocating a new
memory context might not be required. But on re-thinking and reading the
details above, IMO, it is a good improvement.

Yeah, we need to stop the memory accumulation, either by using a
dedicated memory context or by freeing the memory manually.

+1 to the general idea of avoiding memory accumulation.

A couple of minor comments:

1. I think we need to delete the new memory context in
slotsync_failure_callback() as well.

I think we can avoid doing that, because the new memory context should be
destroyed along with its parent context on transaction abort (if any error
occurs).

Yeah, normal error/transaction memory context cleanup seems enough. It
avoids adding callback state only to delete cycle_ctx.

Just one comment:

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

I think we prefer to avoid freeing memory explicitly, since this is a
static function and we already have a memory context in place to prevent leaks.
(We've seen this discussion about explicit freeing multiple times before, and
the previous conclusion was to rely on the memory context management rather than
adding more free code.)

Thanks for pointing out. I wasn't aware of the prior discussion. I
mainly wanted to avoid potential issues in case future callers invoke
it without handling the cleanup properly.

--
Regards,
Xuneng Zhou
HighGo Software Co., Ltd.

Hi Amit,

On Fri, May 15, 2026 at 5:23 PM Amit Kapila <amit.kapila16@gmail.com> wrote:

On Fri, May 15, 2026 at 11:02 AM Xuneng Zhou <xunengzhou@gmail.com> 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.

--
Regards,
Xuneng Zhou
HighGo Software Co., Ltd.

On Fri, May 15, 2026 at 9:20 PM Xuneng Zhou <xunengzhou@gmail.com> wrote:

Hi Amit,

On Fri, May 15, 2026 at 5:23 PM Amit Kapila <amit.kapila16@gmail.com> wrote:

On Fri, May 15, 2026 at 11:02 AM Xuneng Zhou <xunengzhou@gmail.com> 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

--
Regards,
Xuneng Zhou
HighGo Software Co., Ltd.

On Sat, May 16, 2026 at 7:35 PM Xuneng Zhou <xunengzhou@gmail.com> wrote:

On Fri, May 15, 2026 at 9:20 PM Xuneng Zhou <xunengzhou@gmail.com> wrote:

Hi Amit,

On Fri, May 15, 2026 at 5:23 PM Amit Kapila <amit.kapila16@gmail.com> wrote:

On Fri, May 15, 2026 at 11:02 AM Xuneng Zhou <xunengzhou@gmail.com> 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

Expectedly, the memory accumulation is much more pronounced with a
churning slot to disablet the backoff. I'll share the findings later.

--
Regards,
Xuneng Zhou
HighGo Software Co., Ltd.

On Sat, May 16, 2026 at 4:35 AM Xuneng Zhou <xunengzhou@gmail.com> wrote:

On Fri, May 15, 2026 at 9:20 PM Xuneng Zhou <xunengzhou@gmail.com> wrote:

Hi Amit,

On Fri, May 15, 2026 at 5:23 PM Amit Kapila <amit.kapila16@gmail.com> wrote:

On Fri, May 15, 2026 at 11:02 AM Xuneng Zhou <xunengzhou@gmail.com> 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.

On Mon, May 25, 2026 at 7:03 PM Amit Kapila <amit.kapila16@gmail.com> wrote:

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?

I see that the latest patch in email [1]/messages/by-id/CABPTF7WB4Z62sPoZkhSygOCAo3OiTDLpMELxZDuwCb3HYgM_pQ@mail.gmail.com has already addressed this
point. So, I'll push the v2 version.

[1]: /messages/by-id/CABPTF7WB4Z62sPoZkhSygOCAo3OiTDLpMELxZDuwCb3HYgM_pQ@mail.gmail.com

--
With Regards,
Amit Kapila.

On Mon, May 25, 2026 at 7:03 PM Amit Kapila <amit.kapila16@gmail.com>
wrote:

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?

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

[1] -
/messages/by-id/CABPTF7WB4Z62sPoZkhSygOCAo3OiTDLpMELxZDuwCb3HYgM_pQ@mail.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.

On Tue, May 26, 2026 at 1:01 PM Xuneng Zhou <xunengzhou@gmail.com> wrote:

On Mon, May 25, 2026 at 7:03 PM Amit Kapila <amit.kapila16@gmail.com> wrote:

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?

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

[1] - /messages/by-id/CABPTF7WB4Z62sPoZkhSygOCAo3OiTDLpMELxZDuwCb3HYgM_pQ@mail.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.

Pushed.

--
With Regards,
Amit Kapila.

On Tue, May 26, 2026 at 4:39 PM Amit Kapila <amit.kapila16@gmail.com> wrote:

On Tue, May 26, 2026 at 1:01 PM Xuneng Zhou <xunengzhou@gmail.com> wrote:

On Mon, May 25, 2026 at 7:03 PM Amit Kapila <amit.kapila16@gmail.com> wrote:

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?

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

[1] - /messages/by-id/CABPTF7WB4Z62sPoZkhSygOCAo3OiTDLpMELxZDuwCb3HYgM_pQ@mail.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.

Pushed.

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.

Hi Khoa,

On Fri, May 29, 2026 at 12:43 AM Khoa Nguyen <kdnguyen9.oss@gmail.com> wrote:

On Tue, May 26, 2026 at 4:39 PM Amit Kapila <amit.kapila16@gmail.com> wrote:

On Tue, May 26, 2026 at 1:01 PM Xuneng Zhou <xunengzhou@gmail.com> wrote:

On Mon, May 25, 2026 at 7:03 PM Amit Kapila <amit.kapila16@gmail.com> wrote:

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?

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

[1] - /messages/by-id/CABPTF7WB4Z62sPoZkhSygOCAo3OiTDLpMELxZDuwCb3HYgM_pQ@mail.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.

Pushed.

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.

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.

--
Regards,
Xuneng Zhou
HighGo Software Co., Ltd.

On Friday, May 29, 2026 10:31 AM Xuneng Zhou <xunengzhou@gmail.com> wrote:

On Fri, May 29, 2026 at 12:43 AM Khoa Nguyen <kdnguyen9.oss@gmail.com>
wrote:

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.

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.

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

On Fri, May 29, 2026 at 10:59 AM Zhijie Hou (Fujitsu)
<houzj.fnst@fujitsu.com> wrote:

On Friday, May 29, 2026 10:31 AM Xuneng Zhou <xunengzhou@gmail.com> wrote:

On Fri, May 29, 2026 at 12:43 AM Khoa Nguyen <kdnguyen9.oss@gmail.com>
wrote:

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.

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.

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.

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.

--
Regards,
Xuneng Zhou
HighGo Software Co., Ltd.