On Tue, Dec 17, 2024 at 2:48 AM Amit Kapila <amit.kapil...@gmail.com> wrote: > > On Fri, Dec 13, 2024 at 1:22 AM Masahiko Sawada <sawada.m...@gmail.com> wrote: > > > > 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. > > > > 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
