public inbox for [email protected]
help / color / mirror / Atom feedFrom: Michael Paquier <[email protected]>
To: Amit Kapila <[email protected]>
Cc: Alvaro Herrera <[email protected]>
Cc: Postgres hackers <[email protected]>
Subject: Re: Memory leak in WAL sender with pgoutput (v10~)
Date: Mon, 2 Dec 2024 16:19:40 +0900
Message-ID: <[email protected]> (raw)
In-Reply-To: <CAA4eK1JMuOni-s5ZX1ahWZJLtOJbzbBsXZ8LiBPV_LN=736xEQ@mail.gmail.com>
References: <[email protected]>
<[email protected]>
<[email protected]>
<CAA4eK1JMuOni-s5ZX1ahWZJLtOJbzbBsXZ8LiBPV_LN=736xEQ@mail.gmail.com>
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
Attachments:
[application/pgp-signature] signature.asc (833B, ../[email protected]/2-signature.asc)
download
view thread (7+ messages) latest in thread
reply
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: [email protected]
Cc: [email protected], [email protected], [email protected], [email protected]
Subject: Re: Memory leak in WAL sender with pgoutput (v10~)
In-Reply-To: <[email protected]>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox