On Wed, Dec 11, 2024 at 9:43 PM Amit Kapila <amit.kapil...@gmail.com> wrote: > > On Wed, Dec 11, 2024 at 11:09 AM Masahiko Sawada <sawada.m...@gmail.com> > wrote: > > > > On Tue, Dec 10, 2024 at 6:13 PM Zhijie Hou (Fujitsu) > > <houzj.f...@fujitsu.com> wrote: > > > > > > On Wednesday, December 11, 2024 2:14 AM Masahiko Sawada > > > <sawada.m...@gmail.com> wrote: > > > > > > > > On Tue, Dec 10, 2024 at 1:16 AM Amit Kapila <amit.kapil...@gmail.com> > > > > wrote: > > > > > > > > > > On Tue, Dec 10, 2024 at 11:24 AM Amit Kapila <amit.kapil...@gmail.com> > > > > wrote: > > > > > > > > > > > > On Tue, Dec 10, 2024 at 8:54 AM vignesh C <vignes...@gmail.com> > > > > wrote: > > > > > > > > > > > > > > On Tue, 10 Dec 2024 at 04:56, Michael Paquier > > > > > > > <mich...@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
fix_memory_leak_v2.patch
Description: Binary data
fix_memory_leak_v1.patch
Description: Binary data
fix_memory_leak_v3.patch
Description: Binary data
