Memory leak in WAL sender with pgoutput (v10~)

On Fri, Nov 29, 2024 at 11:05:51AM +0900, Michael Paquier wrote:

This is a follow-up of ea792bfd93ab and this thread where I've noticed
that some memory was still leaking when running sysbench with a
logical replication setup:
/messages/by-id/Zz7SRcBXxhOYngOr@paquier.xyz

Adding Amit in CC in case, as we've talked about this topic offlist.

This problem exists since the introduction of logical replication,
down to v10. It would be sad to have an extra loop on publications to
free explicitely the publication names, and the issue would still be
hidden to the existing callers of GetPublication(), so I'd suggest to
change the Publication structure to use a NAMEDATALEN string to force
the free to happen, as in the attached.

That means an ABI break. I've looked around and did not notice any
out-of-core code relying on sizeof(Publication). That does not mean
that nothing would break, of course, but the odds should be pretty
good as this is a rather internal structure. One way would be to just
fix that on HEAD and call it a day, but I'm aware of heavy logirep
users whose workloads would be able to see memory bloating because
they maintain WAL senders around.

After sleeping on that, and because the leak is minimal, I'm thinking
about just applying the fix only on HEAD and call it a day. This
changes the structure of Publication so as we use a char[NAMEDATALEN]
rather than a char*, avoiding the pstrdup(), for the publication name
and free it in the list_free_deep() when reloading the list of
publications.

If there are any objections or comments, feel free. I'll revisit that
again around the middle of next week.
--
Michael

On 2024-Nov-30, Michael Paquier wrote:

After sleeping on that, and because the leak is minimal, I'm thinking
about just applying the fix only on HEAD and call it a day. This
changes the structure of Publication so as we use a char[NAMEDATALEN]
rather than a char*, avoiding the pstrdup(), for the publication name
and free it in the list_free_deep() when reloading the list of
publications.

I'm not sure about your proposed fix. Isn't it simpler to have another
memory context which we can reset instead of doing list_free_deep()? It
doesn't have to be a global memory context -- since this is not
reentrant and not referenced anywhere else, it can be a simple static
variable in that block, as in the attached. I ran the stock tests (no
sysbench) and at least it doesn't crash.

This should be easily backpatchable also, since there's no ABI change.

--
Álvaro Herrera Breisgau, Deutschland — https://www.EnterpriseDB.com/

On Sat, Nov 30, 2024 at 09:28:31AM +0100, Alvaro Herrera wrote:

I'm not sure about your proposed fix. Isn't it simpler to have another
memory context which we can reset instead of doing list_free_deep()? It
doesn't have to be a global memory context -- since this is not
reentrant and not referenced anywhere else, it can be a simple static
variable in that block, as in the attached. I ran the stock tests (no
sysbench) and at least it doesn't crash.

This should be easily backpatchable also, since there's no ABI change.

Yeah. Using more memory contexts around these API calls done without
the cache manipulation is something I've also mentioned to Amit a few
days ago, and I'm feeling that this concept should be applied to a
broader area than just the cached publication list in light of this
thread and ea792bfd93ab. That's a discussion for later, likely. I'm
not sure how broad this should be and if this should be redesign to
make the whole context tracking easier. What I am sure of at this
stage is that for this workload we don't have any garbage left behind.

+			static MemoryContext pubmemcxt = NULL;
+
+			if (pubmemcxt == NULL)
+				pubmemcxt = AllocSetContextCreate(CacheMemoryContext,
+												  "publication list context",
+												  ALLOCSET_DEFAULT_SIZES);
+			else
+				MemoryContextReset(pubmemcxt);
+			oldctx = MemoryContextSwitchTo(pubmemcxt);
+
data->publications = LoadPublications(data->publication_names);
MemoryContextSwitchTo(oldctx);
publications_valid = true;

Something like that works for me in stable branches, removing the need
for the list free as there is only one caller of LoadPublications()
currently. I've checked with my previous script, with the aggressive
invalidation tweak, in the case I'm missing something, and the memory
numbers are stable.

I am slightly concerned about the current design of GetPublication()
in the long-term, TBH. LoadPublications() has hidden the leak behind
two layers of routines in the WAL sender, and that's easy to miss once
you call anything that loads a Publication depending on how the caller
caches its data. So I would still choose for modifying the structure
on HEAD removing the pstrdup() for the publication name. Anyway, your
suggestion is also OK by me on top of that, that's less conflicts in
all the branches.

May I suggest the attached then? 0001 is your backpatch, 0002 would
be my HEAD-only suggestion, if that's OK for you and others of course.
--
Michael

On Mon, Dec 2, 2024 at 7:19 AM Michael Paquier <michael@paquier.xyz> wrote:

On Sat, Nov 30, 2024 at 09:28:31AM +0100, Alvaro Herrera wrote:

I'm not sure about your proposed fix. Isn't it simpler to have another
memory context which we can reset instead of doing list_free_deep()? It
doesn't have to be a global memory context -- since this is not
reentrant and not referenced anywhere else, it can be a simple static
variable in that block, as in the attached. I ran the stock tests (no
sysbench) and at least it doesn't crash.

This should be easily backpatchable also, since there's no ABI change.

Yeah. Using more memory contexts around these API calls done without
the cache manipulation is something I've also mentioned to Amit a few
days ago, and I'm feeling that this concept should be applied to a
broader area than just the cached publication list in light of this
thread and ea792bfd93ab.

We already have PGOutputData->cachectx which could be used for it. I
think we should be able to reset such a context when we are
revalidating the publications. Even, if we want a new context for some
localized handling, we should add that in PGOutputData rather than a
local context as the proposed patch is doing at the very least for
HEAD.

Can't we consider freeing the publication names individually that can
be backpatchable and have no or minimal risk of breaking anything?

I am slightly concerned about the current design of GetPublication()
in the long-term, TBH. LoadPublications() has hidden the leak behind
two layers of routines in the WAL sender, and that's easy to miss once
you call anything that loads a Publication depending on how the caller
caches its data. So I would still choose for modifying the structure
on HEAD removing the pstrdup() for the publication name.

BTW, the subscription structure also used the name in a similar way.
This will make the publication/subscription names handled differently.

--
With Regards,
Amit Kapila.

On Mon, Dec 02, 2024 at 11:47:09AM +0530, Amit Kapila wrote:

We already have PGOutputData->cachectx which could be used for it. I
think we should be able to reset such a context when we are
revalidating the publications. Even, if we want a new context for some
localized handling, we should add that in PGOutputData rather than a
local context as the proposed patch is doing at the very least for
HEAD.

cachectx is used for the publications and the hash table holding
all the RelationSyncEntry entries, but we lack control of individual
parts within it. So you cannot reset the whole context when
processing a publication invalication. Perhaps adding that to
PGOutputData would be better, but that would be inconsistent with
RelationSyncCache.

Can't we consider freeing the publication names individually that can
be backpatchable and have no or minimal risk of breaking anything?

Sure. The first thing I did was a loop that goes through the
publication list and does individual pfree() for the publication
names. That works, but IMO that's weird as we rely on the internals
of GetPublication() hidden two levels down in pg_publication.c.

I am slightly concerned about the current design of GetPublication()
in the long-term, TBH. LoadPublications() has hidden the leak behind
two layers of routines in the WAL sender, and that's easy to miss once
you call anything that loads a Publication depending on how the caller
caches its data. So I would still choose for modifying the structure
on HEAD removing the pstrdup() for the publication name.

BTW, the subscription structure also used the name in a similar way.
This will make the publication/subscription names handled differently.

Good point about the inconsistency, so the name could also be switched
to a fixed-NAMEDATALEN there if we were to do that. The subscription
has much more pstrdup() fields, though.. How about having some Free()
routines instead that deal with the whole cleanup of a single list
entry? If that's kept close to the GetPublication() and
GetSubscription() routines, a refresh when changing these structures
would be hard to miss.
--
Michael

On Mon, Dec 2, 2024 at 12:49 PM Michael Paquier <michael@paquier.xyz> wrote:

On Mon, Dec 02, 2024 at 11:47:09AM +0530, Amit Kapila wrote:

We already have PGOutputData->cachectx which could be used for it. I
think we should be able to reset such a context when we are
revalidating the publications. Even, if we want a new context for some
localized handling, we should add that in PGOutputData rather than a
local context as the proposed patch is doing at the very least for
HEAD.

cachectx is used for the publications and the hash table holding
all the RelationSyncEntry entries, but we lack control of individual
parts within it. So you cannot reset the whole context when
processing a publication invalication. Perhaps adding that to
PGOutputData would be better, but that would be inconsistent with
RelationSyncCache.

AFAICS, RelationSyncCache is not allocated in PGOutputData->cachectx.
It is allocated in CacheMemoryContext, see caller of
init_rel_sync_cache(). I think you are talking about individual hash
entries. Ideally, we can free all entries together and reset cachectx
but right now, we are freeing the allocated memory in those entries,
if required, at the next access. So, resetting the entire
PGOutputData->cachectx won't be possible. But, I don't get why adding
new context in PGOutputData for publications would be inconsistent
with RelationSyncCache? Anyway, I think it would be okay to
retail-free in this case, see the below responses.

Can't we consider freeing the publication names individually that can
be backpatchable and have no or minimal risk of breaking anything?

Sure. The first thing I did was a loop that goes through the
publication list and does individual pfree() for the publication
names. That works, but IMO that's weird as we rely on the internals
of GetPublication() hidden two levels down in pg_publication.c.

We can look at it from a different angle which is that the
FreePublication(s) relies on how the knowledge of Publication
structure is built. So, it doesn't look weird if we think from that
angle.

I am slightly concerned about the current design of GetPublication()
in the long-term, TBH. LoadPublications() has hidden the leak behind
two layers of routines in the WAL sender, and that's easy to miss once
you call anything that loads a Publication depending on how the caller
caches its data. So I would still choose for modifying the structure
on HEAD removing the pstrdup() for the publication name.

BTW, the subscription structure also used the name in a similar way.
This will make the publication/subscription names handled differently.

Good point about the inconsistency, so the name could also be switched
to a fixed-NAMEDATALEN there if we were to do that. The subscription
has much more pstrdup() fields, though.. How about having some Free()
routines instead that deal with the whole cleanup of a single list
entry? If that's kept close to the GetPublication() and
GetSubscription() routines, a refresh when changing these structures
would be hard to miss.

We already have FreeSubscription() which free name and other things
before calling list_free_deep. So, I thought a call on those lines for
publications wouldn't be a bad idea.

--
With Regards,
Amit Kapila.

On 2024-Dec-02, Michael Paquier wrote:

I am slightly concerned about the current design of GetPublication()
in the long-term, TBH. LoadPublications() has hidden the leak behind
two layers of routines in the WAL sender, and that's easy to miss once
you call anything that loads a Publication depending on how the caller
caches its data. So I would still choose for modifying the structure
on HEAD removing the pstrdup() for the publication name. Anyway, your
suggestion is also OK by me on top of that, that's less conflicts in
all the branches.

TBH I'm not sure that wastefully allocating NAMEDATALEN for each
relation is so great. Our strategy for easing memory management is to
use appropriately timed contexts.

I guess if you wanted to make a publication a single palloc block (so
that it's easy to free) and not waste so much memory, you could stash
the name string at the end of the struct. I think that'd be a change
wholly contained in GetPublication.

--
Álvaro Herrera Breisgau, Deutschland — https://www.EnterpriseDB.com/
Are you not unsure you want to delete Firefox?
[Not unsure] [Not not unsure] [Cancel]
http://smylers.hates-software.com/2008/01/03/566e45b2.html

On 2024-Dec-02, Amit Kapila wrote:

We already have PGOutputData->cachectx which could be used for it.
I think we should be able to reset such a context when we are
revalidating the publications.

I don't see that context being reset anywhere, so I have a hard time
imagining that this would work without subtle risk of breakage
elsewhere.

Even, if we want a new context for some localized handling, we should
add that in PGOutputData rather than a local context as the proposed
patch is doing at the very least for HEAD.

I don't necessarily agree, given that this context is not needed
anywhere else.

Can't we consider freeing the publication names individually that can
be backpatchable and have no or minimal risk of breaking anything?

That was my first thought, but then it occurred to me that such a thing
would be totally pointless.

you call anything that loads a Publication depending on how the caller
caches its data. So I would still choose for modifying the structure
on HEAD removing the pstrdup() for the publication name.

BTW, the subscription structure also used the name in a similar way.
This will make the publication/subscription names handled differently.

True (with conninfo, slotname, synccommit, and origin).

FWIW it seems FreeSubscription is incomplete, and not only because it
fails to free the publication names ...

(Why are we storing a string in Subscription->synccommit?)

--
Álvaro Herrera PostgreSQL Developer — https://www.EnterpriseDB.com/
"Los trabajadores menos efectivos son sistematicamente llevados al lugar
donde pueden hacer el menor daño posible: gerencia." (El principio Dilbert)

On Tue, Dec 3, 2024 at 2:12 AM Alvaro Herrera <alvherre@alvh.no-ip.org> wrote:

On 2024-Dec-02, Amit Kapila wrote:

Even, if we want a new context for some localized handling, we should
add that in PGOutputData rather than a local context as the proposed
patch is doing at the very least for HEAD.

I don't necessarily agree, given that this context is not needed
anywhere else.

But that suits the current design more. We allocate PGOutputData and
other contexts in that structure in a "Logical decoding context". A
few of its members (publications, publication_names) residing in
totally unrelated contexts sounds odd. In the first place, we don't
need to allocate publications under CacheMemoryContext, they should be
allocated in PGOutputData->cachectx. However, because we need to free
those entirely at one-shot during invalidation processing, we could
use a new context as a child context of PGOutputData->cachectx. Unless
I am missing something, the current memory context usage appears more
like a coding convenience than a thoughtful design decision.

--
With Regards,
Amit Kapila.

On Tue, Dec 03, 2024 at 09:50:42AM +0530, Amit Kapila wrote:

But that suits the current design more. We allocate PGOutputData and
other contexts in that structure in a "Logical decoding context". A
few of its members (publications, publication_names) residing in
totally unrelated contexts sounds odd. In the first place, we don't
need to allocate publications under CacheMemoryContext, they should be
allocated in PGOutputData->cachectx. However, because we need to free
those entirely at one-shot during invalidation processing, we could
use a new context as a child context of PGOutputData->cachectx. Unless
I am missing something, the current memory context usage appears more
like a coding convenience than a thoughtful design decision.

PGOutputData->cachectx has been introduced in 2022 in commit 52e4f0cd4,
while the decision to have RelationSyncEntry and the publication list
in CacheMemoryContext gets down to v10 where this logical replication
has been introduced. This was a carefully-thought choice back then
because this is data that belongs to the process cache, so yes, this
choice makes sense to me. Even today this choice makes sense: this is
still data cached in the process, and it's stored in the memory
context for cached data.

The problem is that we have two locations where the cached data is
stored, and I'd agree to make the whole leaner by relying on one or
the other entirely, but not both. If you want to move all that to the
single memory context in PGOutputData, perhaps that makes sense in the
long-run and the code gets simpler. Perhaps also it could be simpler
to get everything under CacheMemoryContext and remove
PGOutputData->cachectx. However, if you do so, I'd suggest to use the
same parent context for RelationSyncData as well rather than have two
concepts, not only the publication list.

That's digressing from the original subject a bit, btw..
--
Michael

On Mon, Dec 02, 2024 at 03:29:56PM +0530, Amit Kapila wrote:

We can look at it from a different angle which is that the
FreePublication(s) relies on how the knowledge of Publication
structure is built. So, it doesn't look weird if we think from that
angle.

OK, I can live with that on all the stable branches with an extra
list free rather than a deep list free.

I agree that the memory handling of this whole area needs some rework
to make such leaks harder to introduce in the WAL sender. Still,
let's first solve the problem at hand :)

So how about the attached that introduces a FreePublication() matching
with GetPublication(), used to do the cleanup? Feel free to comment.
--
Michael

On Tue, Dec 3, 2024 at 5:47 PM Michael Paquier <michael@paquier.xyz> wrote:

On Mon, Dec 02, 2024 at 03:29:56PM +0530, Amit Kapila wrote:

We can look at it from a different angle which is that the
FreePublication(s) relies on how the knowledge of Publication
structure is built. So, it doesn't look weird if we think from that
angle.

OK, I can live with that on all the stable branches with an extra
list free rather than a deep list free.

I agree that the memory handling of this whole area needs some rework
to make such leaks harder to introduce in the WAL sender. Still,
let's first solve the problem at hand :)

So how about the attached that introduces a FreePublication() matching
with GetPublication(), used to do the cleanup? Feel free to comment.
--

Perhaps the patch can use foreach_ptr macro instead of foreach?

======
Kind Regards,
Peter Smith.
Fujitsu Australia

On 2024-Dec-03, Peter Smith wrote:

Perhaps the patch can use foreach_ptr macro instead of foreach?

That cannot be backpatched, though.

--
Álvaro Herrera 48°01'N 7°57'E — https://www.EnterpriseDB.com/
"Thou shalt not follow the NULL pointer, for chaos and madness await
thee at its end." (2nd Commandment for C programmers)

On Tue, Dec 03, 2024 at 08:49:41AM +0100, Alvaro Herrera wrote:

That cannot be backpatched, though.

Extra code churn is always annoying across stable branches, so I'd
rather avoid it if we can.
--
Michael

On Tue, Dec 3, 2024 at 11:57 AM Michael Paquier <michael@paquier.xyz> wrote:

On Tue, Dec 03, 2024 at 09:50:42AM +0530, Amit Kapila wrote:

But that suits the current design more. We allocate PGOutputData and
other contexts in that structure in a "Logical decoding context". A
few of its members (publications, publication_names) residing in
totally unrelated contexts sounds odd. In the first place, we don't
need to allocate publications under CacheMemoryContext, they should be
allocated in PGOutputData->cachectx. However, because we need to free
those entirely at one-shot during invalidation processing, we could
use a new context as a child context of PGOutputData->cachectx. Unless
I am missing something, the current memory context usage appears more
like a coding convenience than a thoughtful design decision.

PGOutputData->cachectx has been introduced in 2022 in commit 52e4f0cd4,
while the decision to have RelationSyncEntry and the publication list
in CacheMemoryContext gets down to v10 where this logical replication
has been introduced. This was a carefully-thought choice back then
because this is data that belongs to the process cache, so yes, this
choice makes sense to me.

The parent structure (PGOutputData) was stored in the "Logical
decoding context" even in v11. So, how does storing its member
'publications' in CacheMemoryContext a good idea? It is possible that
we are leaking memory while doing decoding via SQL APIs where we free
decoding context after getting changes though I haven't tested the
same.

--
With Regards,
Amit Kapila.

On Tue, Dec 3, 2024 at 12:17 PM Michael Paquier <michael@paquier.xyz> wrote:

So how about the attached that introduces a FreePublication() matching
with GetPublication(), used to do the cleanup? Feel free to comment.

As you would have noted I am fine with the fix on these lines but I
suggest holding it till we conclude the memory context point raised by
me today. It is possible that we are still leaking some memory in
other related scenarios.

--
With Regards,
Amit Kapila.

On Tuesday, December 3, 2024 4:28 PM Amit Kapila <amit.kapila16@gmail.com> wrote:

On Tue, Dec 3, 2024 at 11:57 AM Michael Paquier <michael@paquier.xyz>
wrote:

On Tue, Dec 03, 2024 at 09:50:42AM +0530, Amit Kapila wrote:

But that suits the current design more. We allocate PGOutputData and
other contexts in that structure in a "Logical decoding context". A
few of its members (publications, publication_names) residing in
totally unrelated contexts sounds odd. In the first place, we don't
need to allocate publications under CacheMemoryContext, they should
be allocated in PGOutputData->cachectx. However, because we need to
free those entirely at one-shot during invalidation processing, we
could use a new context as a child context of
PGOutputData->cachectx. Unless I am missing something, the current
memory context usage appears more like a coding convenience than a

thoughtful design decision.

PGOutputData->cachectx has been introduced in 2022 in commit
PGOutputData->52e4f0cd4,
while the decision to have RelationSyncEntry and the publication list
in CacheMemoryContext gets down to v10 where this logical replication
has been introduced. This was a carefully-thought choice back then
because this is data that belongs to the process cache, so yes, this
choice makes sense to me.

The parent structure (PGOutputData) was stored in the "Logical decoding
context" even in v11. So, how does storing its member 'publications' in
CacheMemoryContext a good idea? It is possible that we are leaking memory
while doing decoding via SQL APIs where we free decoding context after
getting changes though I haven't tested the same.

Right. I think I have faced this memory leak recently. It might be true for
walsender that 'publications' is a per-process content. But SQL APIs might use
different publication names each time during execution.

I can reproduce the memory leak due to allocating the publication
names under CacheMemoryContext like the following:

--
CREATE PUBLICATION pub FOR ALL TABLES;
CREATE TABLE stream_test(a int);
SELECT 'init' FROM pg_create_logical_replication_slot('isolation_slot', 'pgoutput');
INSERT INTO stream_test SELECT generate_series(1, 2, 1);

- he backend's memory usage increases with each execution of the following function
SELECT count(*) FROM pg_logical_slot_peek_binary_changes('isolation_slot', NULL, NULL, 'proto_version', '4', 'publication_names', 'pub,pub,........ <lots of pub names>');

Best Regards,
Hou zj

On Tuesday, December 3, 2024 4:43 AM Alvaro Herrera <alvherre@alvh.no-ip.org> wrote:

On 2024-Dec-02, Amit Kapila wrote:

you call anything that loads a Publication depending on how the
caller caches its data. So I would still choose for modifying the
structure on HEAD removing the pstrdup() for the publication name.

BTW, the subscription structure also used the name in a similar way.
This will make the publication/subscription names handled differently.

True (with conninfo, slotname, synccommit, and origin).

...

(Why are we storing a string in Subscription->synccommit?)

I think it's because the primary purpose of sub->synccommit is to serve as a
parameter for SetConfigOption() in the apply worker, which requires a string
value. Additionally, the existing function set_config_option() that validates
this option only accepts a string input. Although we could convert
sub->synccommit to an integer, this would necessitate additional conversion
code before passing it to these functions.

Best Regards,
Hou zj

On Tue, Dec 03, 2024 at 02:01:00PM +0530, Amit Kapila wrote:

As you would have noted I am fine with the fix on these lines but I
suggest holding it till we conclude the memory context point raised by
me today. It is possible that we are still leaking some memory in
other related scenarios.

Sure. I've not seen anything else, but things are complicated enough
in this code that a path could always have been missed.
--
Michael