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

On 2024-Dec-03, Michael Paquier wrote:

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

I think this doubles down on bad design in the logical replication code,
or at least it goes against what we do almost everywhere else in backend
code. We should do less freeing, more context deleting/resetting.
(Storing stuff in CacheMemoryContext was surely a mistake.)

If you don't like the idea of a static memcxt in the one block where
it's needed, I propose to store a new memcxt in PGOutputData, to be used
exclusively for publications, with a well defined lifetime. I'm against
reusing data->cachecxt, because the lifetime of that is 100% unclear.

--
Álvaro Herrera PostgreSQL Developer — https://www.EnterpriseDB.com/

On Tue, Dec 3, 2024 at 4:03 PM Alvaro Herrera <alvherre@alvh.no-ip.org> wrote:

If you don't like the idea of a static memcxt in the one block where
it's needed, I propose to store a new memcxt in PGOutputData, to be used
exclusively for publications, with a well defined lifetime.

+1. This sounds like a way to proceed at least for HEAD. For
back-branches, it is less clear whether changing PGOutputData is a
good idea. Can such a change in back branches break any existing
non-core code (extensions)?

--
With Regards,
Amit Kapila.

On 2024-Dec-03, Amit Kapila wrote:

On Tue, Dec 3, 2024 at 4:03 PM Alvaro Herrera <alvherre@alvh.no-ip.org> wrote:

If you don't like the idea of a static memcxt in the one block where
it's needed, I propose to store a new memcxt in PGOutputData, to be used
exclusively for publications, with a well defined lifetime.

+1. This sounds like a way to proceed at least for HEAD. For
back-branches, it is less clear whether changing PGOutputData is a
good idea. Can such a change in back branches break any existing
non-core code (extensions)?

We can put the new member at the end of the struct, it shouldn't damage
anything even if they're using this struct -- which I find pretty
unlikely. The only way that could break anything is if somebody is
allocating/using arrays of it, which sounds even more unlikely.

If we don't want to accept that risk (for which I see no argument, but
happy to be proven wrong), I would suggest to use the foreach-pfree
pattern Michael first proposed for the backbranches, and the new memory
context in master. I think this is conducive to better coding overall
as we clean things up in this area.

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

On Tue, Dec 03, 2024 at 02:45:22PM +0100, Alvaro Herrera wrote:

We can put the new member at the end of the struct, it shouldn't damage
anything even if they're using this struct -- which I find pretty
unlikely. The only way that could break anything is if somebody is
allocating/using arrays of it, which sounds even more unlikely.

Yes, that sounds unlikely.

If we don't want to accept that risk (for which I see no argument, but
happy to be proven wrong), I would suggest to use the foreach-pfree
pattern Michael first proposed for the backbranches, and the new memory
context in master. I think this is conducive to better coding overall
as we clean things up in this area.

Is it really worth betting on nobody doing something that does a
sizeof(PGOutputData) for the stable branches? People like doing fancy
things, and we would not hear about such problems except if we push
the button making it a possibility because compiled code suddenly
breaks after a minor release update of the core engine.
--
Michael

On Tue, Dec 3, 2024, at 7:41 PM, Michael Paquier wrote:

On Tue, Dec 03, 2024 at 02:45:22PM +0100, Alvaro Herrera wrote:

If we don't want to accept that risk (for which I see no argument, but
happy to be proven wrong), I would suggest to use the foreach-pfree
pattern Michael first proposed for the backbranches, and the new memory
context in master. I think this is conducive to better coding overall
as we clean things up in this area.

Is it really worth betting on nobody doing something that does a
sizeof(PGOutputData) for the stable branches? People like doing fancy
things, and we would not hear about such problems except if we push
the button making it a possibility because compiled code suddenly
breaks after a minor release update of the core engine.

Although, Debian code search [1]https://codesearch.debian.net/search?q=PGOutputData&amp;literal=0 says this data structure is not used outside
PostgreSQL, I wouldn't risk breaking third-party extensions during a minor
upgrade (even if it is known that such data structure is from that particular
output plugin -- pgoutput -- and other output plugins generally have its own
data structure). +1 from Alvaro's proposal.

[1]: https://codesearch.debian.net/search?q=PGOutputData&amp;literal=0

--
Euler Taveira
EDB https://www.enterprisedb.com/

On Tue, Dec 03, 2024 at 09:46:06PM -0300, Euler Taveira wrote:

Although, Debian code search [1] says this data structure is not used outside
PostgreSQL, I wouldn't risk breaking third-party extensions during a minor
upgrade (even if it is known that such data structure is from that particular
output plugin -- pgoutput -- and other output plugins generally have its own
data structure). +1 from Alvaro's proposal.

A lookup of the public repos of github did not show fancy with the
manipulation of the structure for peoject related to Postgres, either.

FWIW, I'm OK with the memory context reset solution as much as the
direct free calls as we are sure that they will be safe. And at the
end of the day, the problem would be solved with any of these
solutions. My votes would be +0.6 for the free and +0.5 for the mcxt
manipulation, so let's say that they are tied.

As Alvaro and yourself are in favor of the mcxt approach, then let's
go for it. Amit has concerns with other code paths that could be
similarly leaking. I'm not sure if this is worth waiting too long
based on how local the fix for the existing leak is with any of these
solutions.
--
Michael

On Wednesday, December 4, 2024 8:55 AM Michael Paquier <michael@paquier.xyz> wrote:

On Tue, Dec 03, 2024 at 09:46:06PM -0300, Euler Taveira wrote:

Although, Debian code search [1] says this data structure is not used

outside

PostgreSQL, I wouldn't risk breaking third-party extensions during a minor
upgrade (even if it is known that such data structure is from that particular
output plugin -- pgoutput -- and other output plugins generally have its own
data structure). +1 from Alvaro's proposal.

A lookup of the public repos of github did not show fancy with the
manipulation of the structure for peoject related to Postgres, either.

FWIW, I'm OK with the memory context reset solution as much as the
direct free calls as we are sure that they will be safe. And at the
end of the day, the problem would be solved with any of these
solutions. My votes would be +0.6 for the free and +0.5 for the mcxt
manipulation, so let's say that they are tied.

As Alvaro and yourself are in favor of the mcxt approach, then let's
go for it.

+1

Amit has concerns with other code paths that could be
similarly leaking. I'm not sure if this is worth waiting too long
based on how local the fix for the existing leak is with any of these
solutions.

It appears there is an additional memory leak caused by allocating publication
names within the CacheMemoryContext, as noted in [1]/messages/by-id/OS0PR01MB57166A4DA0ABBB94F2FBB28694362@OS0PR01MB5716.jpnprd01.prod.outlook.com. And it can also be fixed by
creating a separate memctx for publication names under the logical decoding
context. I think the approach makes sense since the lifespan of publication
names should ideally align with that of the logical decoding context.

[1]: /messages/by-id/OS0PR01MB57166A4DA0ABBB94F2FBB28694362@OS0PR01MB5716.jpnprd01.prod.outlook.com

Best Regards,
Hou zj

On Wed, Dec 4, 2024 at 7:39 AM Zhijie Hou (Fujitsu)
<houzj.fnst@fujitsu.com> wrote:

On Wednesday, December 4, 2024 8:55 AM Michael Paquier <michael@paquier.xyz> wrote:

Amit has concerns with other code paths that could be
similarly leaking. I'm not sure if this is worth waiting too long
based on how local the fix for the existing leak is with any of these
solutions.

It appears there is an additional memory leak caused by allocating publication
names within the CacheMemoryContext, as noted in [1]. And it can also be fixed by
creating a separate memctx for publication names under the logical decoding
context. I think the approach makes sense since the lifespan of publication
names should ideally align with that of the logical decoding context.

Yeah, I don't think we can go with the proposed patch for the local
memory context as it is.

--
With Regards,
Amit Kapila.

On Wed, Dec 04, 2024 at 11:05:43AM +0530, Amit Kapila wrote:

On Wed, Dec 4, 2024 at 7:39 AM Zhijie Hou (Fujitsu) <houzj.fnst@fujitsu.com> wrote:

It appears there is an additional memory leak caused by allocating publication
names within the CacheMemoryContext, as noted in [1]. And it can also be fixed by
creating a separate memctx for publication names under the logical decoding
context. I think the approach makes sense since the lifespan of publication
names should ideally align with that of the logical decoding context.

Yeah, I don't think we can go with the proposed patch for the local
memory context as it is.

Ah, indeed. I was missing your point. Would any of you like to write
a patch to achieve that?
--
Michael

On Wednesday, December 4, 2024 2:22 PM Michael Paquier <michael@paquier.xyz> wrote:

On Wed, Dec 04, 2024 at 11:05:43AM +0530, Amit Kapila wrote:

On Wed, Dec 4, 2024 at 7:39 AM Zhijie Hou (Fujitsu)

<houzj.fnst@fujitsu.com> wrote:

It appears there is an additional memory leak caused by allocating
publication names within the CacheMemoryContext, as noted in [1]. And
it can also be fixed by creating a separate memctx for publication
names under the logical decoding context. I think the approach makes
sense since the lifespan of publication names should ideally align with that

of the logical decoding context.

Yeah, I don't think we can go with the proposed patch for the local
memory context as it is.

Ah, indeed. I was missing your point. Would any of you like to write a patch
to achieve that?

I can try to write a patch if no one else is working on this.

Best Regards,
Hou zj

On Wed, Dec 04, 2024 at 06:42:55AM +0000, Zhijie Hou (Fujitsu) wrote:

I can try to write a patch if no one else is working on this.

If you have some room to write a patch, that would be really nice.
Thanks.
--
Michael

On Wednesday, December 4, 2024 7:39 PM Michael Paquier <michael@paquier.xyz> wrote:

On Wed, Dec 04, 2024 at 06:42:55AM +0000, Zhijie Hou (Fujitsu) wrote:

I can try to write a patch if no one else is working on this.

If you have some room to write a patch, that would be really nice.
Thanks.

No problem. Here is the patch for the HEAD. This patch introduces a new memory
context within PGOutputData, specifically for allocating memory for
publication_names. The new memory context is nested under the logical decoding
context, ensuring it is freed at the end of decoding through
FreeDecodingContext.

I realized that this patch cannot be backpatched because it introduces a new
field into the public PGOutputData structure. Therefore, I think we may need to
use Alvaro's version [1]/messages/by-id/202411300828.hwe55pzx5a4x@alvherre.pgsql for the back branches.

[1]: /messages/by-id/202411300828.hwe55pzx5a4x@alvherre.pgsql

Best Regards,
Hou zj

On Thu, Dec 05, 2024 at 04:31:56AM +0000, Zhijie Hou (Fujitsu) wrote:

I realized that this patch cannot be backpatched because it introduces a new
field into the public PGOutputData structure. Therefore, I think we may need to
use Alvaro's version [1] for the back branches.

[1] /messages/by-id/202411300828.hwe55pzx5a4x@alvherre.pgsql

Thanks for the patch.

For HEAD it should be as good as it can be as it avoids the problem of
CacheMemoryContext bloating for your case and my case. Alvaro's patch
would not take care of your case, unfortunately, but I'm less worried
about this case in the back branches and we don't track the parent
context where StartupDecodingContext() has begun its work when
building PGOutputData. Thoughts?
--
Michael

On Thu, Dec 5, 2024 at 1:32 PM Zhijie Hou (Fujitsu)
<houzj.fnst@fujitsu.com> wrote:

On Wednesday, December 4, 2024 7:39 PM Michael Paquier <michael@paquier.xyz> wrote:

On Wed, Dec 04, 2024 at 06:42:55AM +0000, Zhijie Hou (Fujitsu) wrote:

I can try to write a patch if no one else is working on this.

If you have some room to write a patch, that would be really nice.
Thanks.

No problem. Here is the patch for the HEAD. This patch introduces a new memory
context within PGOutputData, specifically for allocating memory for
publication_names. The new memory context is nested under the logical decoding
context, ensuring it is freed at the end of decoding through
FreeDecodingContext.

+1 for using new memory context to fix the issue for HEAD.

I realized that this patch cannot be backpatched because it introduces a new
field into the public PGOutputData structure. Therefore, I think we may need to
use Alvaro's version [1] for the back branches.

FWIW for back branches, I prefer using the foreach-pfree pattern
Michael first proposed, just in case. It's not elegant but it can
solve the problem while there is no risk of breaking non-core
extensions. I think we can live with such (a bit of) ugliness on back
branches.

Regards,

--
Masahiko Sawada
Amazon Web Services: https://aws.amazon.com

On Thu, Dec 5, 2024, at 1:31 AM, Zhijie Hou (Fujitsu) wrote:

No problem. Here is the patch for the HEAD. This patch introduces a new memory
context within PGOutputData, specifically for allocating memory for
publication_names. The new memory context is nested under the logical decoding
context, ensuring it is freed at the end of decoding through
FreeDecodingContext.

Thanks for taking care of it. I suggest 2 small adjustments: (a) use
ALLOCSET_SMALL_SIZES instead of ALLOCSET_DEFAULT_SIZES and (b) replace
pubmemcxt with pubmemctx (that's the same abbreviation used by
cachectx). I think you could remove 'mem' from this variable. My
suggestions are pubcxt or pubnamescxt. Although, I prefer the former, if
other publication elements are added to this context in the future.

--
Euler Taveira
EDB https://www.enterprisedb.com/

On Thursday, December 5, 2024 11:39 PM Euler Taveira <euler@eulerto.com> wrote:

Thanks for taking care of it. I suggest 2 small adjustments: (a) use
ALLOCSET_SMALL_SIZES instead of ALLOCSET_DEFAULT_SIZES and (b) replace
pubmemcxt with pubmemctx (that's the same abbreviation used by
cachectx). I think you could remove 'mem' from this variable. My
suggestions are pubcxt or pubnamescxt. Although, I prefer the former, if
other publication elements are added to this context in the future.

Thanks for the suggestions. They make sense to me.

Please see the updated version as attached.

Best Regards,
Hou zj

On Thursday, December 5, 2024 12:52 PM Michael Paquier <michael@paquier.xyz> wrote:

Hi,

On Thu, Dec 05, 2024 at 04:31:56AM +0000, Zhijie Hou (Fujitsu) wrote:

I realized that this patch cannot be backpatched because it introduces
a new field into the public PGOutputData structure. Therefore, I think
we may need to use Alvaro's version [1] for the back branches.

Thanks for the patch.

For HEAD it should be as good as it can be as it avoids the problem of
CacheMemoryContext bloating for your case and my case. Alvaro's patch
would not take care of your case, unfortunately, but I'm less worried about this
case in the back branches and we don't track the parent context where
StartupDecodingContext() has begun its work when building PGOutputData.
Thoughts?

I am fine with the plan. Thanks.

Best Regards,
Hou zj

On Fri, Dec 06, 2024 at 08:23:00AM +0000, Zhijie Hou (Fujitsu) wrote:

Thanks for the suggestions. They make sense to me.

Please see the updated version as attached.

It sounds to me that we are in agreement for HEAD, so I've moved ahead
and fixed this issue on HEAD using your patch that adds a dedicated
memory context in PGOutputData as that's the cleanest way to address
things in a single execution context of pgoutput.

For stable branches, let's see.. I need to reply to the latest
message.
--
Michael

On Thu, Dec 5, 2024 at 2:56 PM Masahiko Sawada <sawada.mshk@gmail.com> wrote:

I realized that this patch cannot be backpatched because it introduces a new
field into the public PGOutputData structure. Therefore, I think we may need to
use Alvaro's version [1] for the back branches.

FWIW for back branches, I prefer using the foreach-pfree pattern
Michael first proposed, just in case. It's not elegant but it can
solve the problem while there is no risk of breaking non-core
extensions.

It couldn't solve the problem completely even in back-branches. The
SQL API case I mentioned and tested by Hou-San in the email [1]/messages/by-id/OS0PR01MB57166A4DA0ABBB94F2FBB28694362@OS0PR01MB5716.jpnprd01.prod.outlook.com won't
be solved.

[1]: /messages/by-id/OS0PR01MB57166A4DA0ABBB94F2FBB28694362@OS0PR01MB5716.jpnprd01.prod.outlook.com

--
With Regards,
Amit Kapila.

On Mon, Dec 9, 2024 at 2:06 AM Amit Kapila <amit.kapila16@gmail.com> wrote:

On Thu, Dec 5, 2024 at 2:56 PM Masahiko Sawada <sawada.mshk@gmail.com> wrote:

I realized that this patch cannot be backpatched because it introduces a new
field into the public PGOutputData structure. Therefore, I think we may need to
use Alvaro's version [1] for the back branches.

FWIW for back branches, I prefer using the foreach-pfree pattern
Michael first proposed, just in case. It's not elegant but it can
solve the problem while there is no risk of breaking non-core
extensions.

It couldn't solve the problem completely even in back-branches. The
SQL API case I mentioned and tested by Hou-San in the email [1] won't
be solved.

True. There seems another place where we possibly leak memory on
CacheMemoryContext when using pgoutput via SQL APIs:

/* Map must live as long as the session does. */
oldctx = MemoryContextSwitchTo(CacheMemoryContext);

entry->attrmap = build_attrmap_by_name_if_req(indesc, outdesc, false);

MemoryContextSwitchTo(oldctx);
RelationClose(ancestor);

entry->attrmap is pfree'd only when validating the RelationSyncEntry
so remains even after logical decoding API calls.

Regards,

--
Masahiko Sawada
Amazon Web Services: https://aws.amazon.com

On Mon, Dec 09, 2024 at 03:36:15PM +0530, Amit Kapila wrote:

It couldn't solve the problem completely even in back-branches. The
SQL API case I mentioned and tested by Hou-San in the email [1] won't
be solved.

[1] - /messages/by-id/OS0PR01MB57166A4DA0ABBB94F2FBB28694362@OS0PR01MB5716.jpnprd01.prod.outlook.com

Yeah, exactly (wanted to reply exactly that yesterday but lacked time,
thanks!).

Alvaro's solution is not perfect either as we would still bloat some
memory in the CacheMemoryContext when a single execution of the
logical decoding context finishes, but the static context approach
proposed at [2]/messages/by-id/202411300828.hwe55pzx5a4x@alvherre.pgsql -- Michael has the merit to limit the damage on repetitive calls
when an invalidation can happen as well as in WAL senders with
periodic cleanups. The free APIs would just lose track of these
pointers: good for WAL senders, not for repetitive pgoutput calls.

Instead of ALLOCSET_DEFAULT_SIZES, it seems to me that we should
switch to ALLOCSET_SMALL_SIZES for consistency with HEAD in Alvaro's
patch. I would also switch "publication list context" to "logical
replication publication list context" to match with HEAD, while on it.

Would all that be OK for everybody in terms of what to do in stable
branches down to 13?

[2]: /messages/by-id/202411300828.hwe55pzx5a4x@alvherre.pgsql -- Michael
--
Michael

On Mon, Dec 09, 2024 at 12:46:35PM -0800, Masahiko Sawada wrote:

True. There seems another place where we possibly leak memory on
CacheMemoryContext when using pgoutput via SQL APIs:

/* Map must live as long as the session does. */
oldctx = MemoryContextSwitchTo(CacheMemoryContext);

entry->attrmap = build_attrmap_by_name_if_req(indesc, outdesc, false);

MemoryContextSwitchTo(oldctx);
RelationClose(ancestor);

entry->attrmap is pfree'd only when validating the RelationSyncEntry
so remains even after logical decoding API calls.

Right. I'm also slightly worried about how we handle streamed_txns in
set_schema_sent_in_streamed_txn() through CacheMemoryContext. It
feels like this could be made more robust without relying on an
explicit list_free() in get_rel_sync_entry(), including the fact that
some cleanup also relies on pgoutput_stream_abort() being taken.

As a whole, thinking long-term, it seems to me that we'd live better
if we remove all direct dependencies to CacheMemoryContext in this
code.
--
Michael

On Tue, Dec 10, 2024 at 2:17 AM Masahiko Sawada <sawada.mshk@gmail.com> wrote:

On Mon, Dec 9, 2024 at 2:06 AM Amit Kapila <amit.kapila16@gmail.com> wrote:

On Thu, Dec 5, 2024 at 2:56 PM Masahiko Sawada <sawada.mshk@gmail.com> wrote:

I realized that this patch cannot be backpatched because it introduces a new
field into the public PGOutputData structure. Therefore, I think we may need to
use Alvaro's version [1] for the back branches.

FWIW for back branches, I prefer using the foreach-pfree pattern
Michael first proposed, just in case. It's not elegant but it can
solve the problem while there is no risk of breaking non-core
extensions.

It couldn't solve the problem completely even in back-branches. The
SQL API case I mentioned and tested by Hou-San in the email [1] won't
be solved.

True. There seems another place where we possibly leak memory on
CacheMemoryContext when using pgoutput via SQL APIs:

/* Map must live as long as the session does. */
oldctx = MemoryContextSwitchTo(CacheMemoryContext);

entry->attrmap = build_attrmap_by_name_if_req(indesc, outdesc, false);

MemoryContextSwitchTo(oldctx);
RelationClose(ancestor);

entry->attrmap is pfree'd only when validating the RelationSyncEntry
so remains even after logical decoding API calls.

We have also noticed this but it needs more analysis on the fix which
one of my colleagues is doing. I think we can fix this as a separate
issue unless you think otherwise.

--
With Regards,
Amit Kapila.

On Tue, 10 Dec 2024 at 04:56, Michael Paquier <michael@paquier.xyz> wrote:

On Mon, Dec 09, 2024 at 03:36:15PM +0530, Amit Kapila wrote:

It couldn't solve the problem completely even in back-branches. The
SQL API case I mentioned and tested by Hou-San in the email [1] won't
be solved.

[1] - /messages/by-id/OS0PR01MB57166A4DA0ABBB94F2FBB28694362@OS0PR01MB5716.jpnprd01.prod.outlook.com

Yeah, exactly (wanted to reply exactly that yesterday but lacked time,
thanks!).

Alvaro's solution is not perfect either as we would still bloat some
memory in the CacheMemoryContext when a single execution of the
logical decoding context finishes, but the static context approach
proposed at [2] has the merit to limit the damage on repetitive calls
when an invalidation can happen as well as in WAL senders with
periodic cleanups. The free APIs would just lose track of these
pointers: good for WAL senders, not for repetitive pgoutput calls.

Instead of ALLOCSET_DEFAULT_SIZES, it seems to me that we should
switch to ALLOCSET_SMALL_SIZES for consistency with HEAD in Alvaro's
patch. I would also switch "publication list context" to "logical
replication publication list context" to match with HEAD, while on it.

Yes, that makes sense. How about something like the attached patch.

Regards,
Vignesh

On Tue, Dec 10, 2024 at 03:24:19AM +0000, vignesh C wrote:

Yes, that makes sense. How about something like the attached patch.

So you have this bit hidden in 7f481b8d3884, causing a small conflict
when cherry-picking the change from 15 to 14:

-            oldctx = MemoryContextSwitchTo(CacheMemoryContext);
-            if (data->publications)
-                list_free_deep(data->publications);
+            static MemoryContext pubctx = NULL;
[...]
-            if (data->publications)
-            {
-                list_free_deep(data->publications);
-                data->publications = NIL;
-            }
+            static MemoryContext pubctx = NULL;

Your versions look OK at quick glance, for the publication list
caching.
--
Michael

On Tue, Dec 10, 2024 at 8:54 AM vignesh C <vignesh21@gmail.com> wrote:

On Tue, 10 Dec 2024 at 04:56, Michael Paquier <michael@paquier.xyz> wrote:

On Mon, Dec 09, 2024 at 03:36:15PM +0530, Amit Kapila wrote:

It couldn't solve the problem completely even in back-branches. The
SQL API case I mentioned and tested by Hou-San in the email [1] won't
be solved.

[1] - /messages/by-id/OS0PR01MB57166A4DA0ABBB94F2FBB28694362@OS0PR01MB5716.jpnprd01.prod.outlook.com

Yeah, exactly (wanted to reply exactly that yesterday but lacked time,
thanks!).

Yes, that makes sense. How about something like the attached patch.

- oldctx = MemoryContextSwitchTo(CacheMemoryContext);
- if (data->publications)
- {
- list_free_deep(data->publications);
- data->publications = NIL;
- }
+ static MemoryContext pubctx = NULL;
+
+ if (pubctx == NULL)
+ pubctx = AllocSetContextCreate(CacheMemoryContext,
+    "logical replication publication list context",
+    ALLOCSET_SMALL_SIZES);
+ else
+ MemoryContextReset(pubctx);
+
+ oldctx = MemoryContextSwitchTo(pubctx);

Considering the SQL API case, why is it okay to allocate this context
under CacheMemoryContext?

--
With Regards,
Amit Kapila.

On Tue, Dec 10, 2024 at 11:24 AM Amit Kapila <amit.kapila16@gmail.com> wrote:

On Tue, Dec 10, 2024 at 8:54 AM vignesh C <vignesh21@gmail.com> wrote:

On Tue, 10 Dec 2024 at 04:56, Michael Paquier <michael@paquier.xyz> wrote:

On Mon, Dec 09, 2024 at 03:36:15PM +0530, Amit Kapila wrote:

It couldn't solve the problem completely even in back-branches. The
SQL API case I mentioned and tested by Hou-San in the email [1] won't
be solved.

[1] - /messages/by-id/OS0PR01MB57166A4DA0ABBB94F2FBB28694362@OS0PR01MB5716.jpnprd01.prod.outlook.com

Yeah, exactly (wanted to reply exactly that yesterday but lacked time,
thanks!).

Yes, that makes sense. How about something like the attached patch.

- oldctx = MemoryContextSwitchTo(CacheMemoryContext);
- if (data->publications)
- {
- list_free_deep(data->publications);
- data->publications = NIL;
- }
+ static MemoryContext pubctx = NULL;
+
+ if (pubctx == NULL)
+ pubctx = AllocSetContextCreate(CacheMemoryContext,
+    "logical replication publication list context",
+    ALLOCSET_SMALL_SIZES);
+ else
+ MemoryContextReset(pubctx);
+
+ oldctx = MemoryContextSwitchTo(pubctx);

Considering the SQL API case, why is it okay to allocate this context
under CacheMemoryContext?

On further thinking, we can't allocate it under
LogicalDecodingContext->context because once that is freed at the end
of SQL API pg_logical_slot_get_changes(), pubctx will be pointing to a
dangling memory. One idea is that we use
MemoryContextRegisterResetCallback() to invoke a reset callback
function where we can reset pubctx but not sure if we want to go there
in back branches. OTOH, the currently proposed fix won't leak memory
on repeated calls to pg_logical_slot_get_changes(), so that might be
okay as well.

Thoughts?

--
With Regards,
Amit Kapila.

On Mon, Dec 9, 2024 at 6:52 PM Amit Kapila <amit.kapila16@gmail.com> wrote:

On Tue, Dec 10, 2024 at 2:17 AM Masahiko Sawada <sawada.mshk@gmail.com> wrote:

On Mon, Dec 9, 2024 at 2:06 AM Amit Kapila <amit.kapila16@gmail.com> wrote:

On Thu, Dec 5, 2024 at 2:56 PM Masahiko Sawada <sawada.mshk@gmail.com> wrote:

I realized that this patch cannot be backpatched because it introduces a new
field into the public PGOutputData structure. Therefore, I think we may need to
use Alvaro's version [1] for the back branches.

FWIW for back branches, I prefer using the foreach-pfree pattern
Michael first proposed, just in case. It's not elegant but it can
solve the problem while there is no risk of breaking non-core
extensions.

It couldn't solve the problem completely even in back-branches. The
SQL API case I mentioned and tested by Hou-San in the email [1] won't
be solved.

True. There seems another place where we possibly leak memory on
CacheMemoryContext when using pgoutput via SQL APIs:

/* Map must live as long as the session does. */
oldctx = MemoryContextSwitchTo(CacheMemoryContext);

entry->attrmap = build_attrmap_by_name_if_req(indesc, outdesc, false);

MemoryContextSwitchTo(oldctx);
RelationClose(ancestor);

entry->attrmap is pfree'd only when validating the RelationSyncEntry
so remains even after logical decoding API calls.

We have also noticed this but it needs more analysis on the fix which
one of my colleagues is doing. I think we can fix this as a separate
issue unless you think otherwise.

I agree to fix this as a separate patch.

Regards,

--
Masahiko Sawada
Amazon Web Services: https://aws.amazon.com

On Tue, Dec 10, 2024 at 1:16 AM Amit Kapila <amit.kapila16@gmail.com> wrote:

On Tue, Dec 10, 2024 at 11:24 AM Amit Kapila <amit.kapila16@gmail.com> wrote:

On Tue, Dec 10, 2024 at 8:54 AM vignesh C <vignesh21@gmail.com> wrote:

On Tue, 10 Dec 2024 at 04:56, Michael Paquier <michael@paquier.xyz> wrote:

On Mon, Dec 09, 2024 at 03:36:15PM +0530, Amit Kapila wrote:

It couldn't solve the problem completely even in back-branches. The
SQL API case I mentioned and tested by Hou-San in the email [1] won't
be solved.

[1] - /messages/by-id/OS0PR01MB57166A4DA0ABBB94F2FBB28694362@OS0PR01MB5716.jpnprd01.prod.outlook.com

Yeah, exactly (wanted to reply exactly that yesterday but lacked time,
thanks!).

Yes, that makes sense. How about something like the attached patch.

- oldctx = MemoryContextSwitchTo(CacheMemoryContext);
- if (data->publications)
- {
- list_free_deep(data->publications);
- data->publications = NIL;
- }
+ static MemoryContext pubctx = NULL;
+
+ if (pubctx == NULL)
+ pubctx = AllocSetContextCreate(CacheMemoryContext,
+    "logical replication publication list context",
+    ALLOCSET_SMALL_SIZES);
+ else
+ MemoryContextReset(pubctx);
+
+ oldctx = MemoryContextSwitchTo(pubctx);

Considering the SQL API case, why is it okay to allocate this context
under CacheMemoryContext?

On further thinking, we can't allocate it under
LogicalDecodingContext->context because once that is freed at the end
of SQL API pg_logical_slot_get_changes(), pubctx will be pointing to a
dangling memory. One idea is that we use
MemoryContextRegisterResetCallback() to invoke a reset callback
function where we can reset pubctx but not sure if we want to go there
in back branches. OTOH, the currently proposed fix won't leak memory
on repeated calls to pg_logical_slot_get_changes(), so that might be
okay as well.

Thoughts?

Alternative idea is to declare pubctx as a file static variable. And
we create the memory context under LogicalDecodingContext->context in
the startup callback and free it in the shutdown callback.

Regards,

--
Masahiko Sawada
Amazon Web Services: https://aws.amazon.com

On Wednesday, December 11, 2024 2:14 AM Masahiko Sawada <sawada.mshk@gmail.com> wrote:

On Tue, Dec 10, 2024 at 1:16 AM Amit Kapila <amit.kapila16@gmail.com>
wrote:

On Tue, Dec 10, 2024 at 11:24 AM Amit Kapila <amit.kapila16@gmail.com>

wrote:

On Tue, Dec 10, 2024 at 8:54 AM vignesh C <vignesh21@gmail.com>

wrote:

On Tue, 10 Dec 2024 at 04:56, Michael Paquier <michael@paquier.xyz>

wrote:

On Mon, Dec 09, 2024 at 03:36:15PM +0530, Amit Kapila wrote:

It couldn't solve the problem completely even in back-branches. The
SQL API case I mentioned and tested by Hou-San in the email [1]

won't

be solved.

[1] -

/messages/by-id/OS0PR01MB57166A4DA0ABBB94F
2FBB28694362%40OS0PR01MB5716.jpnprd01.prod.outlook.com

Yeah, exactly (wanted to reply exactly that yesterday but lacked time,
thanks!).

Yes, that makes sense. How about something like the attached patch.

- oldctx = MemoryContextSwitchTo(CacheMemoryContext);
- if (data->publications)
- {
- list_free_deep(data->publications);
- data->publications = NIL;
- }
+ static MemoryContext pubctx = NULL;
+
+ if (pubctx == NULL)
+ pubctx = AllocSetContextCreate(CacheMemoryContext,
+    "logical replication publication list context",
+    ALLOCSET_SMALL_SIZES);
+ else
+ MemoryContextReset(pubctx);
+
+ oldctx = MemoryContextSwitchTo(pubctx);

Considering the SQL API case, why is it okay to allocate this context
under CacheMemoryContext?

On further thinking, we can't allocate it under
LogicalDecodingContext->context because once that is freed at the end
of SQL API pg_logical_slot_get_changes(), pubctx will be pointing to a
dangling memory. One idea is that we use
MemoryContextRegisterResetCallback() to invoke a reset callback
function where we can reset pubctx but not sure if we want to go there
in back branches. OTOH, the currently proposed fix won't leak memory
on repeated calls to pg_logical_slot_get_changes(), so that might be
okay as well.

Thoughts?

Alternative idea is to declare pubctx as a file static variable. And
we create the memory context under LogicalDecodingContext->context in
the startup callback and free it in the shutdown callback.

I think when an ERROR occurs during the execution of the pg_logical_slot_xx()
API, the shutdown callback function is not invoked. This would result in the
static variable not being reset, which, I think, is why Amit mentioned the use
of MemoryContextRegisterResetCallback().

Best Regards,
Hou zj

On Tue, 10 Dec 2024 at 23:36, Masahiko Sawada <sawada.mshk@gmail.com> wrote:

On Mon, Dec 9, 2024 at 6:52 PM Amit Kapila <amit.kapila16@gmail.com> wrote:

On Tue, Dec 10, 2024 at 2:17 AM Masahiko Sawada <sawada.mshk@gmail.com> wrote:

On Mon, Dec 9, 2024 at 2:06 AM Amit Kapila <amit.kapila16@gmail.com> wrote:

On Thu, Dec 5, 2024 at 2:56 PM Masahiko Sawada <sawada.mshk@gmail.com> wrote:

I realized that this patch cannot be backpatched because it introduces a new
field into the public PGOutputData structure. Therefore, I think we may need to
use Alvaro's version [1] for the back branches.

FWIW for back branches, I prefer using the foreach-pfree pattern
Michael first proposed, just in case. It's not elegant but it can
solve the problem while there is no risk of breaking non-core
extensions.

It couldn't solve the problem completely even in back-branches. The
SQL API case I mentioned and tested by Hou-San in the email [1] won't
be solved.

True. There seems another place where we possibly leak memory on
CacheMemoryContext when using pgoutput via SQL APIs:

/* Map must live as long as the session does. */
oldctx = MemoryContextSwitchTo(CacheMemoryContext);

entry->attrmap = build_attrmap_by_name_if_req(indesc, outdesc, false);

MemoryContextSwitchTo(oldctx);
RelationClose(ancestor);

entry->attrmap is pfree'd only when validating the RelationSyncEntry
so remains even after logical decoding API calls.

We have also noticed this but it needs more analysis on the fix which
one of my colleagues is doing. I think we can fix this as a separate
issue unless you think otherwise.

I agree to fix this as a separate patch.

Thanks Sawada-san, I have started a new thread with a test case which
can reproduce this issue at [1]/messages/by-id/CALDaNm1hewNAsZ_e6FF52a=9drmkRJxtEPrzCB6-9mkJyeBBqA@mail.gmail.com:
[1]: /messages/by-id/CALDaNm1hewNAsZ_e6FF52a=9drmkRJxtEPrzCB6-9mkJyeBBqA@mail.gmail.com

Regards,
Vignesh

On Tue, Dec 10, 2024 at 6:13 PM Zhijie Hou (Fujitsu)
<houzj.fnst@fujitsu.com> wrote:

On Wednesday, December 11, 2024 2:14 AM Masahiko Sawada <sawada.mshk@gmail.com> wrote:

On Tue, Dec 10, 2024 at 1:16 AM Amit Kapila <amit.kapila16@gmail.com>
wrote:

On Tue, Dec 10, 2024 at 11:24 AM Amit Kapila <amit.kapila16@gmail.com>

wrote:

On Tue, Dec 10, 2024 at 8:54 AM vignesh C <vignesh21@gmail.com>

wrote:

On Tue, 10 Dec 2024 at 04:56, Michael Paquier <michael@paquier.xyz>

wrote:

On Mon, Dec 09, 2024 at 03:36:15PM +0530, Amit Kapila wrote:

It couldn't solve the problem completely even in back-branches. The
SQL API case I mentioned and tested by Hou-San in the email [1]

won't

be solved.

[1] -

/messages/by-id/OS0PR01MB57166A4DA0ABBB94F
2FBB28694362%40OS0PR01MB5716.jpnprd01.prod.outlook.com

Yeah, exactly (wanted to reply exactly that yesterday but lacked time,
thanks!).

Yes, that makes sense. How about something like the attached patch.

- oldctx = MemoryContextSwitchTo(CacheMemoryContext);
- if (data->publications)
- {
- list_free_deep(data->publications);
- data->publications = NIL;
- }
+ static MemoryContext pubctx = NULL;
+
+ if (pubctx == NULL)
+ pubctx = AllocSetContextCreate(CacheMemoryContext,
+    "logical replication publication list context",
+    ALLOCSET_SMALL_SIZES);
+ else
+ MemoryContextReset(pubctx);
+
+ oldctx = MemoryContextSwitchTo(pubctx);

Considering the SQL API case, why is it okay to allocate this context
under CacheMemoryContext?

On further thinking, we can't allocate it under
LogicalDecodingContext->context because once that is freed at the end
of SQL API pg_logical_slot_get_changes(), pubctx will be pointing to a
dangling memory. One idea is that we use
MemoryContextRegisterResetCallback() to invoke a reset callback
function where we can reset pubctx but not sure if we want to go there
in back branches. OTOH, the currently proposed fix won't leak memory
on repeated calls to pg_logical_slot_get_changes(), so that might be
okay as well.

Thoughts?

Alternative idea is to declare pubctx as a file static variable. And
we create the memory context under LogicalDecodingContext->context in
the startup callback and free it in the shutdown callback.

I think when an ERROR occurs during the execution of the pg_logical_slot_xx()
API, the shutdown callback function is not invoked. This would result in the
static variable not being reset, which, I think, is why Amit mentioned the use
of MemoryContextRegisterResetCallback().

My idea is that since that new context is cleaned up together with its
parent context (LogicalDecodingContext->context), we unconditionally
set that new context to the static variable at the startup callback.
That being said, Amit's idea would be cleaner.

Regards,

--
Masahiko Sawada
Amazon Web Services: https://aws.amazon.com

On Wed, Dec 11, 2024 at 11:09 AM Masahiko Sawada <sawada.mshk@gmail.com> wrote:

On Tue, Dec 10, 2024 at 6:13 PM Zhijie Hou (Fujitsu)
<houzj.fnst@fujitsu.com> wrote:

On Wednesday, December 11, 2024 2:14 AM Masahiko Sawada <sawada.mshk@gmail.com> wrote:

On Tue, Dec 10, 2024 at 1:16 AM Amit Kapila <amit.kapila16@gmail.com>
wrote:

On Tue, Dec 10, 2024 at 11:24 AM Amit Kapila <amit.kapila16@gmail.com>

wrote:

On Tue, Dec 10, 2024 at 8:54 AM vignesh C <vignesh21@gmail.com>

wrote:

On Tue, 10 Dec 2024 at 04:56, Michael Paquier <michael@paquier.xyz>

wrote:

On Mon, Dec 09, 2024 at 03:36:15PM +0530, Amit Kapila wrote:

It couldn't solve the problem completely even in back-branches. The
SQL API case I mentioned and tested by Hou-San in the email [1]

won't

be solved.

[1] -

/messages/by-id/OS0PR01MB57166A4DA0ABBB94F
2FBB28694362%40OS0PR01MB5716.jpnprd01.prod.outlook.com

Yeah, exactly (wanted to reply exactly that yesterday but lacked time,
thanks!).

Yes, that makes sense. How about something like the attached patch.

- oldctx = MemoryContextSwitchTo(CacheMemoryContext);
- if (data->publications)
- {
- list_free_deep(data->publications);
- data->publications = NIL;
- }
+ static MemoryContext pubctx = NULL;
+
+ if (pubctx == NULL)
+ pubctx = AllocSetContextCreate(CacheMemoryContext,
+    "logical replication publication list context",
+    ALLOCSET_SMALL_SIZES);
+ else
+ MemoryContextReset(pubctx);
+
+ oldctx = MemoryContextSwitchTo(pubctx);

Considering the SQL API case, why is it okay to allocate this context
under CacheMemoryContext?

On further thinking, we can't allocate it under
LogicalDecodingContext->context because once that is freed at the end
of SQL API pg_logical_slot_get_changes(), pubctx will be pointing to a
dangling memory. One idea is that we use
MemoryContextRegisterResetCallback() to invoke a reset callback
function where we can reset pubctx but not sure if we want to go there
in back branches. OTOH, the currently proposed fix won't leak memory
on repeated calls to pg_logical_slot_get_changes(), so that might be
okay as well.

Thoughts?

Alternative idea is to declare pubctx as a file static variable. And
we create the memory context under LogicalDecodingContext->context in
the startup callback and free it in the shutdown callback.

I think when an ERROR occurs during the execution of the pg_logical_slot_xx()
API, the shutdown callback function is not invoked. This would result in the
static variable not being reset, which, I think, is why Amit mentioned the use
of MemoryContextRegisterResetCallback().

My idea is that since that new context is cleaned up together with its
parent context (LogicalDecodingContext->context), we unconditionally
set that new context to the static variable at the startup callback.
That being said, Amit's idea would be cleaner.

Your preference is not completely clear. Are you okay with the idea of
Vignesh's currently proposed patch for back-branches, or do you prefer
to use a memory context reset callback, or do you have a different
idea that should be adopted for back-branches?

--
With Regards,
Amit Kapila.

On Wed, Dec 11, 2024 at 9:43 PM Amit Kapila <amit.kapila16@gmail.com> wrote:

On Wed, Dec 11, 2024 at 11:09 AM Masahiko Sawada <sawada.mshk@gmail.com> wrote:

On Tue, Dec 10, 2024 at 6:13 PM Zhijie Hou (Fujitsu)
<houzj.fnst@fujitsu.com> wrote:

On Wednesday, December 11, 2024 2:14 AM Masahiko Sawada <sawada.mshk@gmail.com> wrote:

On Tue, Dec 10, 2024 at 1:16 AM Amit Kapila <amit.kapila16@gmail.com>
wrote:

On Tue, Dec 10, 2024 at 11:24 AM Amit Kapila <amit.kapila16@gmail.com>

wrote:

On Tue, Dec 10, 2024 at 8:54 AM vignesh C <vignesh21@gmail.com>

wrote:

On Tue, 10 Dec 2024 at 04:56, Michael Paquier <michael@paquier.xyz>

wrote:

On Mon, Dec 09, 2024 at 03:36:15PM +0530, Amit Kapila wrote:

It couldn't solve the problem completely even in back-branches. The
SQL API case I mentioned and tested by Hou-San in the email [1]

won't

be solved.

[1] -

/messages/by-id/OS0PR01MB57166A4DA0ABBB94F
2FBB28694362%40OS0PR01MB5716.jpnprd01.prod.outlook.com

Yeah, exactly (wanted to reply exactly that yesterday but lacked time,
thanks!).

Yes, that makes sense. How about something like the attached patch.

- oldctx = MemoryContextSwitchTo(CacheMemoryContext);
- if (data->publications)
- {
- list_free_deep(data->publications);
- data->publications = NIL;
- }
+ static MemoryContext pubctx = NULL;
+
+ if (pubctx == NULL)
+ pubctx = AllocSetContextCreate(CacheMemoryContext,
+    "logical replication publication list context",
+    ALLOCSET_SMALL_SIZES);
+ else
+ MemoryContextReset(pubctx);
+
+ oldctx = MemoryContextSwitchTo(pubctx);

Considering the SQL API case, why is it okay to allocate this context
under CacheMemoryContext?

On further thinking, we can't allocate it under
LogicalDecodingContext->context because once that is freed at the end
of SQL API pg_logical_slot_get_changes(), pubctx will be pointing to a
dangling memory. One idea is that we use
MemoryContextRegisterResetCallback() to invoke a reset callback
function where we can reset pubctx but not sure if we want to go there
in back branches. OTOH, the currently proposed fix won't leak memory
on repeated calls to pg_logical_slot_get_changes(), so that might be
okay as well.

Thoughts?

Alternative idea is to declare pubctx as a file static variable. And
we create the memory context under LogicalDecodingContext->context in
the startup callback and free it in the shutdown callback.

I think when an ERROR occurs during the execution of the pg_logical_slot_xx()
API, the shutdown callback function is not invoked. This would result in the
static variable not being reset, which, I think, is why Amit mentioned the use
of MemoryContextRegisterResetCallback().

My idea is that since that new context is cleaned up together with its
parent context (LogicalDecodingContext->context), we unconditionally
set that new context to the static variable at the startup callback.
That being said, Amit's idea would be cleaner.

Your preference is not completely clear. Are you okay with the idea of
Vignesh's currently proposed patch for back-branches, or do you prefer
to use a memory context reset callback, or do you have a different
idea that should be adopted for back-branches?

IIUC the current Vignesh's patch[1]/messages/by-id/CALDaNm2C=mYNB9ZUS5t9irB2P0Tjm_r+nRVc717JdO+NtCVunw@mail.gmail.com doesn't solve the memory leak in
case of using logical decoding APIs, as you mentioned. I've tried the
idea of using memory context reset callback to reset pubctx. We need
to register the callback to LogicalContextDecodingContext->context,
meaning that we need to pass it to get_rel_sync_entry() (see
fix_memory_leak_v1.patch). I don't prefer this approach as it could
make backpatching complex in the future. Alternatively, we can declare
pubctx as a file static variable, create the memory context at the
startup callback, reset the pubctx at the shutdown callback, and use
the memory context reset callback to ensure the pubctx is reset (see
fix_memory_leak_v2.patch).Or I think we might not necessarily need to
use the memory context reset callback (see fix_memory_leak_v3.patch).
I prefer the latter two approaches.

Regards,

[1]: /messages/by-id/CALDaNm2C=mYNB9ZUS5t9irB2P0Tjm_r+nRVc717JdO+NtCVunw@mail.gmail.com

--
Masahiko Sawada
Amazon Web Services: https://aws.amazon.com

On Thu, Dec 12, 2024 at 11:52:20AM -0800, Masahiko Sawada wrote:

IIUC the current Vignesh's patch[1] doesn't solve the memory leak in
case of using logical decoding APIs, as you mentioned. I've tried the
idea of using memory context reset callback to reset pubctx. We need
to register the callback to LogicalContextDecodingContext->context,
meaning that we need to pass it to get_rel_sync_entry() (see
fix_memory_leak_v1.patch). I don't prefer this approach as it could
make backpatching complex in the future. Alternatively, we can declare
pubctx as a file static variable, create the memory context at the
startup callback, reset the pubctx at the shutdown callback, and use
the memory context reset callback to ensure the pubctx is reset (see
fix_memory_leak_v2.patch).Or I think we might not necessarily need to
use the memory context reset callback (see fix_memory_leak_v3.patch).
I prefer the latter two approaches.

+    pubctx = AllocSetContextCreate(ctx->context,
+                                   "logical replication publication list context",
+                                   ALLOCSET_SMALL_SIZES);
+

Knowing that there can be only one pgoutput context running at a given
time and that pubctx would be reset automatically when exiting
pgoutput with its parent context, I find the simplicity of v3
tempting. Now, keeping in the stack a static pointer that could point
to the void depending on where we are makes me really uneasy because
that could be the source of more bugs (think a-la-CVE if the pointer
points to something that gets reallocated later on still is referenced
in this process because of something), so v2 where the pointer is
reset when leaving the pgoutput context has a much better idea of how
to do the job.
--
Michael

On Fri, Dec 13, 2024 at 1:22 AM Masahiko Sawada <sawada.mshk@gmail.com> wrote:

On Wed, Dec 11, 2024 at 9:43 PM Amit Kapila <amit.kapila16@gmail.com> wrote:

On Wed, Dec 11, 2024 at 11:09 AM Masahiko Sawada <sawada.mshk@gmail.com> wrote:

On Tue, Dec 10, 2024 at 6:13 PM Zhijie Hou (Fujitsu)
<houzj.fnst@fujitsu.com> wrote:

On Wednesday, December 11, 2024 2:14 AM Masahiko Sawada <sawada.mshk@gmail.com> wrote:

On Tue, Dec 10, 2024 at 1:16 AM Amit Kapila <amit.kapila16@gmail.com>
wrote:

On Tue, Dec 10, 2024 at 11:24 AM Amit Kapila <amit.kapila16@gmail.com>

wrote:

On Tue, Dec 10, 2024 at 8:54 AM vignesh C <vignesh21@gmail.com>

wrote:

On Tue, 10 Dec 2024 at 04:56, Michael Paquier <michael@paquier.xyz>

wrote:

On Mon, Dec 09, 2024 at 03:36:15PM +0530, Amit Kapila wrote:

It couldn't solve the problem completely even in back-branches. The
SQL API case I mentioned and tested by Hou-San in the email [1]

won't

be solved.

[1] -

/messages/by-id/OS0PR01MB57166A4DA0ABBB94F
2FBB28694362%40OS0PR01MB5716.jpnprd01.prod.outlook.com

Yeah, exactly (wanted to reply exactly that yesterday but lacked time,
thanks!).

Yes, that makes sense. How about something like the attached patch.

- oldctx = MemoryContextSwitchTo(CacheMemoryContext);
- if (data->publications)
- {
- list_free_deep(data->publications);
- data->publications = NIL;
- }
+ static MemoryContext pubctx = NULL;
+
+ if (pubctx == NULL)
+ pubctx = AllocSetContextCreate(CacheMemoryContext,
+    "logical replication publication list context",
+    ALLOCSET_SMALL_SIZES);
+ else
+ MemoryContextReset(pubctx);
+
+ oldctx = MemoryContextSwitchTo(pubctx);

Considering the SQL API case, why is it okay to allocate this context
under CacheMemoryContext?

On further thinking, we can't allocate it under
LogicalDecodingContext->context because once that is freed at the end
of SQL API pg_logical_slot_get_changes(), pubctx will be pointing to a
dangling memory. One idea is that we use
MemoryContextRegisterResetCallback() to invoke a reset callback
function where we can reset pubctx but not sure if we want to go there
in back branches. OTOH, the currently proposed fix won't leak memory
on repeated calls to pg_logical_slot_get_changes(), so that might be
okay as well.

Thoughts?

Alternative idea is to declare pubctx as a file static variable. And
we create the memory context under LogicalDecodingContext->context in
the startup callback and free it in the shutdown callback.

I think when an ERROR occurs during the execution of the pg_logical_slot_xx()
API, the shutdown callback function is not invoked. This would result in the
static variable not being reset, which, I think, is why Amit mentioned the use
of MemoryContextRegisterResetCallback().

My idea is that since that new context is cleaned up together with its
parent context (LogicalDecodingContext->context), we unconditionally
set that new context to the static variable at the startup callback.
That being said, Amit's idea would be cleaner.

Your preference is not completely clear. Are you okay with the idea of
Vignesh's currently proposed patch for back-branches, or do you prefer
to use a memory context reset callback, or do you have a different
idea that should be adopted for back-branches?

IIUC the current Vignesh's patch[1] doesn't solve the memory leak in
case of using logical decoding APIs, as you mentioned.

Right, but note that it wouldn't leak memory on repeated calls to the
API. Only if the backend ever makes a single call for get_changes will
it leak memory once, which is not ideal. Still, we can live with it if
the other approaches are complex for back branches.

I've tried the
idea of using memory context reset callback to reset pubctx. We need
to register the callback to LogicalContextDecodingContext->context,
meaning that we need to pass it to get_rel_sync_entry() (see
fix_memory_leak_v1.patch). I don't prefer this approach as it could
make backpatching complex in the future. Alternatively, we can declare
pubctx as a file static variable, create the memory context at the
startup callback, reset the pubctx at the shutdown callback, and use
the memory context reset callback to ensure the pubctx is reset (see
fix_memory_leak_v2.patch).Or I think we might not necessarily need to
use the memory context reset callback (see fix_memory_leak_v3.patch).
I prefer the latter two approaches.

Will fix_memory_leak_v3.patch avoid the leak in case of an ERROR in
SQL API? If so, how?

--
With Regards,
Amit Kapila.

On Tue, Dec 17, 2024 at 2:48 AM Amit Kapila <amit.kapila16@gmail.com> wrote:

On Fri, Dec 13, 2024 at 1:22 AM Masahiko Sawada <sawada.mshk@gmail.com> wrote:

On Wed, Dec 11, 2024 at 9:43 PM Amit Kapila <amit.kapila16@gmail.com> wrote:

On Wed, Dec 11, 2024 at 11:09 AM Masahiko Sawada <sawada.mshk@gmail.com> wrote:

On Tue, Dec 10, 2024 at 6:13 PM Zhijie Hou (Fujitsu)
<houzj.fnst@fujitsu.com> wrote:

On Wednesday, December 11, 2024 2:14 AM Masahiko Sawada <sawada.mshk@gmail.com> wrote:

On Tue, Dec 10, 2024 at 1:16 AM Amit Kapila <amit.kapila16@gmail.com>
wrote:

On Tue, Dec 10, 2024 at 11:24 AM Amit Kapila <amit.kapila16@gmail.com>

wrote:

On Tue, Dec 10, 2024 at 8:54 AM vignesh C <vignesh21@gmail.com>

wrote:

On Tue, 10 Dec 2024 at 04:56, Michael Paquier <michael@paquier.xyz>

wrote:

On Mon, Dec 09, 2024 at 03:36:15PM +0530, Amit Kapila wrote:

It couldn't solve the problem completely even in back-branches. The
SQL API case I mentioned and tested by Hou-San in the email [1]

won't

be solved.

[1] -

/messages/by-id/OS0PR01MB57166A4DA0ABBB94F
2FBB28694362%40OS0PR01MB5716.jpnprd01.prod.outlook.com

Yeah, exactly (wanted to reply exactly that yesterday but lacked time,
thanks!).

Yes, that makes sense. How about something like the attached patch.

- oldctx = MemoryContextSwitchTo(CacheMemoryContext);
- if (data->publications)
- {
- list_free_deep(data->publications);
- data->publications = NIL;
- }
+ static MemoryContext pubctx = NULL;
+
+ if (pubctx == NULL)
+ pubctx = AllocSetContextCreate(CacheMemoryContext,
+    "logical replication publication list context",
+    ALLOCSET_SMALL_SIZES);
+ else
+ MemoryContextReset(pubctx);
+
+ oldctx = MemoryContextSwitchTo(pubctx);

Considering the SQL API case, why is it okay to allocate this context
under CacheMemoryContext?

On further thinking, we can't allocate it under
LogicalDecodingContext->context because once that is freed at the end
of SQL API pg_logical_slot_get_changes(), pubctx will be pointing to a
dangling memory. One idea is that we use
MemoryContextRegisterResetCallback() to invoke a reset callback
function where we can reset pubctx but not sure if we want to go there
in back branches. OTOH, the currently proposed fix won't leak memory
on repeated calls to pg_logical_slot_get_changes(), so that might be
okay as well.

Thoughts?

Alternative idea is to declare pubctx as a file static variable. And
we create the memory context under LogicalDecodingContext->context in
the startup callback and free it in the shutdown callback.

I think when an ERROR occurs during the execution of the pg_logical_slot_xx()
API, the shutdown callback function is not invoked. This would result in the
static variable not being reset, which, I think, is why Amit mentioned the use
of MemoryContextRegisterResetCallback().

My idea is that since that new context is cleaned up together with its
parent context (LogicalDecodingContext->context), we unconditionally
set that new context to the static variable at the startup callback.
That being said, Amit's idea would be cleaner.

Your preference is not completely clear. Are you okay with the idea of
Vignesh's currently proposed patch for back-branches, or do you prefer
to use a memory context reset callback, or do you have a different
idea that should be adopted for back-branches?

IIUC the current Vignesh's patch[1] doesn't solve the memory leak in
case of using logical decoding APIs, as you mentioned.

Right, but note that it wouldn't leak memory on repeated calls to the
API. Only if the backend ever makes a single call for get_changes will
it leak memory once, which is not ideal. Still, we can live with it if
the other approaches are complex for back branches.

True. I missed that point.

I've tried the
idea of using memory context reset callback to reset pubctx. We need
to register the callback to LogicalContextDecodingContext->context,
meaning that we need to pass it to get_rel_sync_entry() (see
fix_memory_leak_v1.patch). I don't prefer this approach as it could
make backpatching complex in the future. Alternatively, we can declare
pubctx as a file static variable, create the memory context at the
startup callback, reset the pubctx at the shutdown callback, and use
the memory context reset callback to ensure the pubctx is reset (see
fix_memory_leak_v2.patch).Or I think we might not necessarily need to
use the memory context reset callback (see fix_memory_leak_v3.patch).
I prefer the latter two approaches.

Will fix_memory_leak_v3.patch avoid the leak in case of an ERROR in
SQL API? If so, how?

The pubctx is created as a child of LogicalDecodingContext->context.
On an error, the pubctx is cleaned up altogether when cleaning up
LogicalDecodingContext->context.

Regards,

--
Masahiko Sawada
Amazon Web Services: https://aws.amazon.com

On Wed, Dec 18, 2024 at 12:32 PM Masahiko Sawada <sawada.mshk@gmail.com> wrote:

On Tue, Dec 17, 2024 at 2:48 AM Amit Kapila <amit.kapila16@gmail.com> wrote:

Will fix_memory_leak_v3.patch avoid the leak in case of an ERROR in
SQL API? If so, how?

The pubctx is created as a child of LogicalDecodingContext->context.
On an error, the pubctx is cleaned up altogether when cleaning up
LogicalDecodingContext->context.

The difference between fix_memory_leak_v2 and fix_memory_leak_v3 is
that the earlier one resets the pubctx to NULL along with freeing the
context memory. Resetting a file-level global variable is a good idea,
similar to what we do for RelationSyncCache, so I prefer v2 over v3,
but I am fine if you would like to proceed with v3.

--
With Regards,
Amit Kapila.

On Thu, Dec 19, 2024 at 07:50:57AM +0530, Amit Kapila wrote:

The difference between fix_memory_leak_v2 and fix_memory_leak_v3 is
that the earlier one resets the pubctx to NULL along with freeing the
context memory. Resetting a file-level global variable is a good idea,
similar to what we do for RelationSyncCache, so I prefer v2 over v3,
but I am fine if you would like to proceed with v3.

FWIW, I am not OK with v3. I've raised this exact point a couple of
days ago upthread:
/messages/by-id/Z1t5pXsNEYwS4P5k@paquier.xyz

v2 does not have these weaknesses by design.
--
Michael

On Wed, Dec 18, 2024 at 6:26 PM Michael Paquier <michael@paquier.xyz> wrote:

On Thu, Dec 19, 2024 at 07:50:57AM +0530, Amit Kapila wrote:

The difference between fix_memory_leak_v2 and fix_memory_leak_v3 is
that the earlier one resets the pubctx to NULL along with freeing the
context memory. Resetting a file-level global variable is a good idea,
similar to what we do for RelationSyncCache, so I prefer v2 over v3,
but I am fine if you would like to proceed with v3.

FWIW, I am not OK with v3. I've raised this exact point a couple of
days ago upthread:
/messages/by-id/Z1t5pXsNEYwS4P5k@paquier.xyz

v2 does not have these weaknesses by design.

I agree that v2 is better than v3 in terms of that.

Regards,

--
Masahiko Sawada
Amazon Web Services: https://aws.amazon.com

On Thu, Dec 19, 2024 at 09:27:04AM -0800, Masahiko Sawada wrote:

On Wed, Dec 18, 2024 at 6:26 PM Michael Paquier <michael@paquier.xyz> wrote:

v2 does not have these weaknesses by design.

I agree that v2 is better than v3 in terms of that.

Okay. In terms of the backbranches, would you prefer that I handle
this patch myself as I have done the HEAD part? This would need a
second, closer, review but I could do that at the beginning of next
week.

Or perhaps you'd prefer doing it yourself? That's up to you, happy to
help as always :D
--
Michael

On Thu, Dec 19, 2024 at 6:31 PM Michael Paquier <michael@paquier.xyz> wrote:

On Thu, Dec 19, 2024 at 09:27:04AM -0800, Masahiko Sawada wrote:

On Wed, Dec 18, 2024 at 6:26 PM Michael Paquier <michael@paquier.xyz> wrote:

v2 does not have these weaknesses by design.

I agree that v2 is better than v3 in terms of that.

Okay. In terms of the backbranches, would you prefer that I handle
this patch myself as I have done the HEAD part? This would need a
second, closer, review but I could do that at the beginning of next
week.

Thanks. Please proceed with this fix as you've already fixed the HEAD part.

Regards,

--
Masahiko Sawada
Amazon Web Services: https://aws.amazon.com

On Fri, Dec 20, 2024 at 11:23:30AM -0800, Masahiko Sawada wrote:

Thanks. Please proceed with this fix as you've already fixed the HEAD part.

Thanks. It took me a bit of time to check that across all five stable
branches, including sysbench and the SQL case, and I think that's OK,
so done (added a couple of comments on the way).
--
Michael