public inbox for [email protected]
help / color / mirror / Atom feedFrom: Zhijie Hou (Fujitsu) <[email protected]>
To: vignesh C <[email protected]>
Cc: PostgreSQL Hackers <[email protected]>
Subject: RE: Memory leak in pg_logical_slot_{get,peek}_changes
Date: Wed, 11 Dec 2024 09:45:50 +0000
Message-ID: <OS0PR01MB5716C28AC48C3313D9CBA38F943E2@OS0PR01MB5716.jpnprd01.prod.outlook.com> (raw)
In-Reply-To: <CALDaNm3qt0JPUO9aajVhnRdJo9-vOt2Wm3K3mFSkrRqeeFT+Eg@mail.gmail.com>
References: <CALDaNm1hewNAsZ_e6FF52a=9drmkRJxtEPrzCB6-9mkJyeBBqA@mail.gmail.com>
<OS0PR01MB5716AD5128F16AA1A02B6D29943E2@OS0PR01MB5716.jpnprd01.prod.outlook.com>
<CALDaNm3qt0JPUO9aajVhnRdJo9-vOt2Wm3K3mFSkrRqeeFT+Eg@mail.gmail.com>
On Wednesday, December 11, 2024 5:11 PM vignesh C <[email protected]> wrote:
>
> On Wed, 11 Dec 2024 at 11:39, Zhijie Hou (Fujitsu) <[email protected]>
> wrote:
> >
> > On Wednesday, December 11, 2024 12:28 PM vignesh C
> <[email protected]> wrote:
> > > The attached patch resolves a memory leak by ensuring that the
> > > attribute map is properly freed during plugin shutdown. This process
> > > is triggered by the SQL API when the decoding context is being
> > > released. Additionally, I am conducting a review to identify and
> > > address any similar memory leaks that may exist elsewhere in the code.
> >
> > Thanks for reporting the issue and share the fix.
> >
> > I am not sure if freeing them in shutdown callback is safe, because
> > shutdown callback Is not invoked in case of ERRORs. I think we'd
> > better allocate them under cachectx in the beginning to avoid freeing them
> explicitly.
>
> I initially considered addressing this issue in a way similar to your suggestion
> while fixing it, but later decided to make the change in pgoutput_shutdown,
> following the approach used for RelationSyncCache.
> This was because RelationSyncCache relies on CacheMemoryContext, and
> attrmap is a member of RelationSyncCache entry.
I think we have tended to allocate the member of RelationSyncEntry under
logical decoding context since 52e4f0c. I think that makes more sense because these
members ideally should live as long as the decoding context. In addition, it was
suggested[1] that allocating all the thing under CacheMemoryContext is hard to
debug. And people in another thread[2] also seems agree to remove the dependency
of CacheMemoryContext in the long term.
> Now that we're considering
> attrmap in the context of cachectx, do you think we should apply cachectx to
> RelationSyncCache as well to solve the similar issue that can occur with
> RelationSyncCache.
I personally think it could be considered as a separate project because
RelationSyncCache is accessed even after shutting down the output plugin due to
the registered cache invalidation callbacks. We probably need
MemoryContextRegisterResetCallback() to reset the static pointer
(RelationSyncCache).
[1] https://www.postgresql.org/message-id/20220129003110.6ndrrpanem5sb4ee%40alap3.anarazel.de
[2] https://www.postgresql.org/message-id/Z1d-uR20pt6wtQIS%40paquier.xyz
Best Regards,
Hou zj
view thread (9+ 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]
Subject: RE: Memory leak in pg_logical_slot_{get,peek}_changes
In-Reply-To: <OS0PR01MB5716C28AC48C3313D9CBA38F943E2@OS0PR01MB5716.jpnprd01.prod.outlook.com>
* 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