Horton

Thread

Commits

  1. Fix memory leak in pgoutput with relation attribute map

  2. Fix memory leak in pgoutput with publication list cache

  3. Allow specifying row filters for logical replication of tables.

  1. Memory leak in WAL sender with pgoutput (v10~)

    Michael Paquier <michael@paquier.xyz> — 2024-11-29T02:05:51Z

    Hi all,

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:
https://www.postgresql.org/message-id/Zz7SRcBXxhOYngOr@paquier.xyz

Reproducing the problem is quite simple, and can be done with the
following steps (please adapt, like previous thread):
1) Initialize a primary and set up some empty tables:
NUM_TABLES=500
PRIMARY_PORT=5432
STANDBY_PORT=5433
sysbench oltp_read_only --db-driver=pgsql \
		 --pgsql-port=$PRIMARY_PORT \
		 --pgsql-db=postgres \
		 --pgsql-user=postgres \
		 --pgsql-host=/tmp \
		 --tables=$NUM_TABLES --table_size=0 \
		 --report-interval=1 \
		 --threads=1 prepare
2) Create a standby, promote it:
STANDBY_DATA=/define/your/standby/path/
pg_basebackup --checkpoint=fast -D $STANDBY_DATA -p $PRIMARY_PORT -h /tmp/ -R
echo "port = $STANDBY_PORT" >> $STANDBY_DATA/postgresql.conf
pg_ctl -D $STANDBY_DATA start
pg_ctl -D $STANDBY_DATA promote
3) Publication and subscription
psql -p $PRIMARY_PORT -c "CREATE PUBLICATION pub1 FOR ALL TABLES;"
psql -p $STANDBY_PORT -c "CREATE SUBSCRIPTION sub1 CONNECTION
'port=$PRIMARY_PORT user=postgres dbname=postgres PUBLICATION pub1 WITH
(enabled, create_slot, slot_name='pub1_to_sub1', copy_data=false);"
4) Run sysbench:
sysbench oltp_write_only --db-driver=pgsql \
		 --pgsql-port=$PRIMARY_PORT \
		 --pgsql-db=postgres \
		 --pgsql-user=postgres \
		 --pgsql-host=/tmp \
		 --tables=$NUM_TABLES --table_size=100 \
		 --report-interval=1 \
		 --threads=$NUM_THREADS run \
		 --time=5000

With that, I've mentioned internally that there was an extra leak and
left the problem lying aside for a few days before being able to come
back to it.  In-between, Jeff Davis has done a review of the code to
note that we leak memory in the CacheMemoryContext, due to the
list_free_deep() done in get_rel_sync_entry() that misses the fact
that the publication name is pstrdup()'d in GetPublication(), but
list_free_deep() does not know that so the memory of the strdupped
publication names stays around.  That's wrong.

Back to today, I've done more benchmarking using the above steps and
logged periodic memory samples of the WAL sender with
pg_log_backend_memory_contexts() (100s, 50 points), and analyzed the 
evolution of the whole thing. "Grand Total" used memory keeps
increasing due to one memory context: CacheMemoryContext.  So yes,
Jeff has spotted what looks like the only leak we have. 

Attached is a graph that shows the evolution of memory between HEAD
and a patched version for the used memory of CacheMemoryContext
(ylabel is in bytes, could not get that right as my gnuplot skills are
bad).

One thing to note in this case is that I've made the leak more
noticeable with this tweak to force the publication to be reset each
time with go through get_rel_sync_entry(), or memory takes much longer
to pile up.  That does not change the problem, speeds up the detection
by a large factor:
--- a/src/backend/replication/pgoutput/pgoutput.c
+++ b/src/backend/replication/pgoutput/pgoutput.c
@@ -2061,12 +2061,13 @@ get_rel_sync_entry(PGOutputData *data, Relation relation)
         List       *rel_publications = NIL;
 
         /* Reload publications if needed before use. */
-        if (!publications_valid)
+        elog(WARNING, "forcing reload publications");
         {
             oldctx = MemoryContextSwitchTo(CacheMemoryContext);
             if (data->publications)
             {
                 list_free_deep(data->publications);

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.

Thoughts or comments?
--
Michael

  2. Re: Memory leak in WAL sender with pgoutput (v10~)

    Michael Paquier <michael@paquier.xyz> — 2024-11-30T03:58:44Z

    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:
> https://www.postgresql.org/message-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

  3. Re: Memory leak in WAL sender with pgoutput (v10~)

    Alvaro Herrera <alvherre@alvh.no-ip.org> — 2024-11-30T08:28:31Z

    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/

  4. Re: Memory leak in WAL sender with pgoutput (v10~)

    Michael Paquier <michael@paquier.xyz> — 2024-12-02T01:49:34Z

    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

  5. Re: Memory leak in WAL sender with pgoutput (v10~)

    Amit Kapila <amit.kapila16@gmail.com> — 2024-12-02T06:17:09Z

    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.

  6. Re: Memory leak in WAL sender with pgoutput (v10~)

    Michael Paquier <michael@paquier.xyz> — 2024-12-02T07:19:40Z

    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

  7. Re: Memory leak in WAL sender with pgoutput (v10~)

    Amit Kapila <amit.kapila16@gmail.com> — 2024-12-02T09:59:56Z

    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.

  8. Re: Memory leak in WAL sender with pgoutput (v10~)

    Alvaro Herrera <alvherre@alvh.no-ip.org> — 2024-12-02T20:29:41Z

    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

  9. Re: Memory leak in WAL sender with pgoutput (v10~)

    Alvaro Herrera <alvherre@alvh.no-ip.org> — 2024-12-02T20:42:52Z

    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)

  10. Re: Memory leak in WAL sender with pgoutput (v10~)

    Amit Kapila <amit.kapila16@gmail.com> — 2024-12-03T04:20:42Z

    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.

  11. Re: Memory leak in WAL sender with pgoutput (v10~)

    Michael Paquier <michael@paquier.xyz> — 2024-12-03T06:27:03Z

    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

  12. Re: Memory leak in WAL sender with pgoutput (v10~)

    Michael Paquier <michael@paquier.xyz> — 2024-12-03T06:46:50Z

    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

  13. Re: Memory leak in WAL sender with pgoutput (v10~)

    Peter Smith <smithpb2250@gmail.com> — 2024-12-03T06:56:48Z

    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

  14. Re: Memory leak in WAL sender with pgoutput (v10~)

    Alvaro Herrera <alvherre@alvh.no-ip.org> — 2024-12-03T07:49:41Z

    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)

  15. Re: Memory leak in WAL sender with pgoutput (v10~)

    Michael Paquier <michael@paquier.xyz> — 2024-12-03T07:56:32Z

    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

  16. Re: Memory leak in WAL sender with pgoutput (v10~)

    Amit Kapila <amit.kapila16@gmail.com> — 2024-12-03T08:27:35Z

    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.

  17. Re: Memory leak in WAL sender with pgoutput (v10~)

    Amit Kapila <amit.kapila16@gmail.com> — 2024-12-03T08:31:00Z

    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.

  18. RE: Memory leak in WAL sender with pgoutput (v10~)

    Zhijie Hou (Fujitsu) <houzj.fnst@fujitsu.com> — 2024-12-03T08:36:42Z

    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

  19. RE: Memory leak in WAL sender with pgoutput (v10~)

    Zhijie Hou (Fujitsu) <houzj.fnst@fujitsu.com> — 2024-12-03T08:54:03Z

    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

  20. Re: Memory leak in WAL sender with pgoutput (v10~)

    Michael Paquier <michael@paquier.xyz> — 2024-12-03T09:11:43Z

    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

  21. Re: Memory leak in WAL sender with pgoutput (v10~)

    Alvaro Herrera <alvherre@alvh.no-ip.org> — 2024-12-03T10:33:55Z

    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/

  22. Re: Memory leak in WAL sender with pgoutput (v10~)

    Amit Kapila <amit.kapila16@gmail.com> — 2024-12-03T12:14:10Z

    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.

  23. Re: Memory leak in WAL sender with pgoutput (v10~)

    Alvaro Herrera <alvherre@alvh.no-ip.org> — 2024-12-03T13:45:22Z

    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/

  24. Re: Memory leak in WAL sender with pgoutput (v10~)

    Michael Paquier <michael@paquier.xyz> — 2024-12-03T22:41:19Z

    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

  25. Re: Memory leak in WAL sender with pgoutput (v10~)

    Euler Taveira <euler@eulerto.com> — 2024-12-04T00:46:06Z

    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] 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&literal=0

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

  26. Re: Memory leak in WAL sender with pgoutput (v10~)

    Michael Paquier <michael@paquier.xyz> — 2024-12-04T00:55:16Z

    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

  27. RE: Memory leak in WAL sender with pgoutput (v10~)

    Zhijie Hou (Fujitsu) <houzj.fnst@fujitsu.com> — 2024-12-04T02:09:16Z

    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]. 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] https://www.postgresql.org/message-id/OS0PR01MB57166A4DA0ABBB94F2FBB28694362%40OS0PR01MB5716.jpnprd01.prod.outlook.com

Best Regards,
Hou zj

  28. Re: Memory leak in WAL sender with pgoutput (v10~)

    Amit Kapila <amit.kapila16@gmail.com> — 2024-12-04T05:35:43Z

    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.

  29. Re: Memory leak in WAL sender with pgoutput (v10~)

    Michael Paquier <michael@paquier.xyz> — 2024-12-04T06:21:55Z

    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

  30. RE: Memory leak in WAL sender with pgoutput (v10~)

    Zhijie Hou (Fujitsu) <houzj.fnst@fujitsu.com> — 2024-12-04T06:42:55Z

    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

  31. Re: Memory leak in WAL sender with pgoutput (v10~)

    Michael Paquier <michael@paquier.xyz> — 2024-12-04T11:38:58Z

    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

  32. RE: Memory leak in WAL sender with pgoutput (v10~)

    Zhijie Hou (Fujitsu) <houzj.fnst@fujitsu.com> — 2024-12-05T04:31:56Z

    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] for the back branches.

[1] https://www.postgresql.org/message-id/202411300828.hwe55pzx5a4x%40alvherre.pgsql

Best Regards,
Hou zj

  33. Re: Memory leak in WAL sender with pgoutput (v10~)

    Michael Paquier <michael@paquier.xyz> — 2024-12-05T04:52:19Z

    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] https://www.postgresql.org/message-id/202411300828.hwe55pzx5a4x%40alvherre.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

  34. Re: Memory leak in WAL sender with pgoutput (v10~)

    Masahiko Sawada <sawada.mshk@gmail.com> — 2024-12-05T09:25:51Z

    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

  35. Re: Memory leak in WAL sender with pgoutput (v10~)

    Euler Taveira <euler@eulerto.com> — 2024-12-05T15:39:26Z

    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/

  36. RE: Memory leak in WAL sender with pgoutput (v10~)

    Zhijie Hou (Fujitsu) <houzj.fnst@fujitsu.com> — 2024-12-06T08:23:00Z

    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

  37. RE: Memory leak in WAL sender with pgoutput (v10~)

    Zhijie Hou (Fujitsu) <houzj.fnst@fujitsu.com> — 2024-12-06T08:23:13Z

    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

  38. Re: Memory leak in WAL sender with pgoutput (v10~)

    Michael Paquier <michael@paquier.xyz> — 2024-12-09T07:43:30Z

    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

  39. Re: Memory leak in WAL sender with pgoutput (v10~)

    Amit Kapila <amit.kapila16@gmail.com> — 2024-12-09T10:06:15Z

    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.

[1] - https://www.postgresql.org/message-id/OS0PR01MB57166A4DA0ABBB94F2FBB28694362%40OS0PR01MB5716.jpnprd01.prod.outlook.com

-- 
With Regards,
Amit Kapila.

  40. Re: Memory leak in WAL sender with pgoutput (v10~)

    Masahiko Sawada <sawada.mshk@gmail.com> — 2024-12-09T20:46:35Z

    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

  41. Re: Memory leak in WAL sender with pgoutput (v10~)

    Michael Paquier <michael@paquier.xyz> — 2024-12-09T23:26:20Z

    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] - https://www.postgresql.org/message-id/OS0PR01MB57166A4DA0ABBB94F2FBB28694362%40OS0PR01MB5716.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.

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

[2]: https://www.postgresql.org/message-id/202411300828.hwe55pzx5a4x@alvherre.pgsql
--
Michael

  42. Re: Memory leak in WAL sender with pgoutput (v10~)

    Michael Paquier <michael@paquier.xyz> — 2024-12-09T23:35:21Z

    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

  43. Re: Memory leak in WAL sender with pgoutput (v10~)

    Amit Kapila <amit.kapila16@gmail.com> — 2024-12-10T02:52:40Z

    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.

  44. Re: Memory leak in WAL sender with pgoutput (v10~)

    vignesh C <vignesh21@gmail.com> — 2024-12-10T03:24:19Z

    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] - https://www.postgresql.org/message-id/OS0PR01MB57166A4DA0ABBB94F2FBB28694362%40OS0PR01MB5716.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

  45. Re: Memory leak in WAL sender with pgoutput (v10~)

    Michael Paquier <michael@paquier.xyz> — 2024-12-10T04:11:43Z

    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

  46. Re: Memory leak in WAL sender with pgoutput (v10~)

    Amit Kapila <amit.kapila16@gmail.com> — 2024-12-10T05:54:38Z

    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] - https://www.postgresql.org/message-id/OS0PR01MB57166A4DA0ABBB94F2FBB28694362%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?

-- 
With Regards,
Amit Kapila.

  47. Re: Memory leak in WAL sender with pgoutput (v10~)

    Amit Kapila <amit.kapila16@gmail.com> — 2024-12-10T09:15:48Z

    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] - https://www.postgresql.org/message-id/OS0PR01MB57166A4DA0ABBB94F2FBB28694362%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?

-- 
With Regards,
Amit Kapila.

  48. Re: Memory leak in WAL sender with pgoutput (v10~)

    Masahiko Sawada <sawada.mshk@gmail.com> — 2024-12-10T18:05:52Z

    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

  49. Re: Memory leak in WAL sender with pgoutput (v10~)

    Masahiko Sawada <sawada.mshk@gmail.com> — 2024-12-10T18:14:24Z

    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] - https://www.postgresql.org/message-id/OS0PR01MB57166A4DA0ABBB94F2FBB28694362%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.

Regards,

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

  50. RE: Memory leak in WAL sender with pgoutput (v10~)

    Zhijie Hou (Fujitsu) <houzj.fnst@fujitsu.com> — 2024-12-11T02:13:50Z

    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] -
> https://www.postgresql.org/message-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

  51. Re: Memory leak in WAL sender with pgoutput (v10~)

    vignesh C <vignesh21@gmail.com> — 2024-12-11T05:08:18Z

    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]:
[1] - https://www.postgresql.org/message-id/flat/CALDaNm1hewNAsZ_e6FF52a%3D9drmkRJxtEPrzCB6-9mkJyeBBqA%40mail.gmail.com

Regards,
Vignesh

  52. Re: Memory leak in WAL sender with pgoutput (v10~)

    Masahiko Sawada <sawada.mshk@gmail.com> — 2024-12-11T05:38:26Z

    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] -
> > https://www.postgresql.org/message-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

  53. Re: Memory leak in WAL sender with pgoutput (v10~)

    Amit Kapila <amit.kapila16@gmail.com> — 2024-12-12T05:43:39Z

    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] -
> > > https://www.postgresql.org/message-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.

  54. Re: Memory leak in WAL sender with pgoutput (v10~)

    Masahiko Sawada <sawada.mshk@gmail.com> — 2024-12-12T19:52:20Z

    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] -
> > > > https://www.postgresql.org/message-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. 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] https://www.postgresql.org/message-id/CALDaNm2C%3DmYNB9ZUS5t9irB2P0Tjm_r%2BnRVc717JdO%2BNtCVunw%40mail.gmail.com

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

  55. Re: Memory leak in WAL sender with pgoutput (v10~)

    Michael Paquier <michael@paquier.xyz> — 2024-12-13T00:02:45Z

    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

  56. Re: Memory leak in WAL sender with pgoutput (v10~)

    Amit Kapila <amit.kapila16@gmail.com> — 2024-12-17T10:48:03Z

    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] -
> > > > > https://www.postgresql.org/message-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.

  57. Re: Memory leak in WAL sender with pgoutput (v10~)

    Masahiko Sawada <sawada.mshk@gmail.com> — 2024-12-18T07:01:33Z

    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] -
> > > > > > https://www.postgresql.org/message-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

  58. Re: Memory leak in WAL sender with pgoutput (v10~)

    Amit Kapila <amit.kapila16@gmail.com> — 2024-12-19T02:20:57Z

    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.

  59. Re: Memory leak in WAL sender with pgoutput (v10~)

    Michael Paquier <michael@paquier.xyz> — 2024-12-19T02:26:35Z

    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:
https://www.postgresql.org/message-id/Z1t5pXsNEYwS4P5k@paquier.xyz

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

  60. Re: Memory leak in WAL sender with pgoutput (v10~)

    Masahiko Sawada <sawada.mshk@gmail.com> — 2024-12-19T17:27:04Z

    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:
> https://www.postgresql.org/message-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

  61. Re: Memory leak in WAL sender with pgoutput (v10~)

    Michael Paquier <michael@paquier.xyz> — 2024-12-20T02:30:31Z

    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

  62. Re: Memory leak in WAL sender with pgoutput (v10~)

    Masahiko Sawada <sawada.mshk@gmail.com> — 2024-12-20T19:23:30Z

    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

  63. Re: Memory leak in WAL sender with pgoutput (v10~)

    Michael Paquier <michael@paquier.xyz> — 2024-12-23T03:53:33Z

    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