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
On Thu, Dec 19, 2024 at 6:31 PM Michael Paquier 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 wrote:
> >> v2 does not have these weaknesses by design.
> >
> > I agree that v2 is better than v3 in terms of that.
>
>
On Thu, Dec 19, 2024 at 09:27:04AM -0800, Masahiko Sawada wrote:
> On Wed, Dec 18, 2024 at 6:26 PM Michael Paquier 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
th
On Wed, Dec 18, 2024 at 6:26 PM Michael Paquier 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
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
On Wed, Dec 18, 2024 at 12:32 PM Masahiko Sawada wrote:
>
> On Tue, Dec 17, 2024 at 2:48 AM Amit Kapila 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
On Tue, Dec 17, 2024 at 2:48 AM Amit Kapila wrote:
>
> On Fri, Dec 13, 2024 at 1:22 AM Masahiko Sawada wrote:
> >
> > On Wed, Dec 11, 2024 at 9:43 PM Amit Kapila wrote:
> > >
> > > On Wed, Dec 11, 2024 at 11:09 AM Masahiko Sawada
> > > wrote:
> > > >
> > > > On Tue, Dec 10, 2024 at 6:13 PM Zhi
On Fri, Dec 13, 2024 at 1:22 AM Masahiko Sawada wrote:
>
> On Wed, Dec 11, 2024 at 9:43 PM Amit Kapila wrote:
> >
> > On Wed, Dec 11, 2024 at 11:09 AM Masahiko Sawada
> > wrote:
> > >
> > > On Tue, Dec 10, 2024 at 6:13 PM Zhijie Hou (Fujitsu)
> > > wrote:
> > > >
> > > > On Wednesday, December
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 ca
On Wed, Dec 11, 2024 at 9:43 PM Amit Kapila wrote:
>
> On Wed, Dec 11, 2024 at 11:09 AM Masahiko Sawada
> wrote:
> >
> > On Tue, Dec 10, 2024 at 6:13 PM Zhijie Hou (Fujitsu)
> > wrote:
> > >
> > > On Wednesday, December 11, 2024 2:14 AM Masahiko Sawada
> > > wrote:
> > > >
> > > > On Tue, Dec
On Wed, Dec 11, 2024 at 11:09 AM Masahiko Sawada wrote:
>
> On Tue, Dec 10, 2024 at 6:13 PM Zhijie Hou (Fujitsu)
> wrote:
> >
> > On Wednesday, December 11, 2024 2:14 AM Masahiko Sawada
> > wrote:
> > >
> > > On Tue, Dec 10, 2024 at 1:16 AM Amit Kapila
> > > wrote:
> > > >
> > > > On Tue, Dec
On Tue, Dec 10, 2024 at 6:13 PM Zhijie Hou (Fujitsu)
wrote:
>
> On Wednesday, December 11, 2024 2:14 AM Masahiko Sawada
> wrote:
> >
> > On Tue, Dec 10, 2024 at 1:16 AM Amit Kapila
> > wrote:
> > >
> > > On Tue, Dec 10, 2024 at 11:24 AM Amit Kapila
> > wrote:
> > > >
> > > > On Tue, Dec 10, 20
On Tue, 10 Dec 2024 at 23:36, Masahiko Sawada wrote:
>
> On Mon, Dec 9, 2024 at 6:52 PM Amit Kapila wrote:
> >
> > On Tue, Dec 10, 2024 at 2:17 AM Masahiko Sawada
> > wrote:
> > >
> > > On Mon, Dec 9, 2024 at 2:06 AM Amit Kapila
> > > wrote:
> > > >
> > > > On Thu, Dec 5, 2024 at 2:56 PM Masa
On Wednesday, December 11, 2024 2:14 AM Masahiko Sawada
wrote:
>
> On Tue, Dec 10, 2024 at 1:16 AM Amit Kapila
> wrote:
> >
> > On Tue, Dec 10, 2024 at 11:24 AM Amit Kapila
> wrote:
> > >
> > > On Tue, Dec 10, 2024 at 8:54 AM vignesh C
> wrote:
> > > >
> > > > On Tue, 10 Dec 2024 at 04:56, Mi
On Tue, Dec 10, 2024 at 1:16 AM Amit Kapila wrote:
>
> On Tue, Dec 10, 2024 at 11:24 AM Amit Kapila wrote:
> >
> > On Tue, Dec 10, 2024 at 8:54 AM vignesh C wrote:
> > >
> > > On Tue, 10 Dec 2024 at 04:56, Michael Paquier wrote:
> > > >
> > > > On Mon, Dec 09, 2024 at 03:36:15PM +0530, Amit Kap
On Mon, Dec 9, 2024 at 6:52 PM Amit Kapila wrote:
>
> On Tue, Dec 10, 2024 at 2:17 AM Masahiko Sawada wrote:
> >
> > On Mon, Dec 9, 2024 at 2:06 AM Amit Kapila wrote:
> > >
> > > On Thu, Dec 5, 2024 at 2:56 PM Masahiko Sawada
> > > wrote:
> > > >
> > > > >
> > > > > I realized that this patch
On Tue, Dec 10, 2024 at 11:24 AM Amit Kapila wrote:
>
> On Tue, Dec 10, 2024 at 8:54 AM vignesh C wrote:
> >
> > On Tue, 10 Dec 2024 at 04:56, Michael Paquier wrote:
> > >
> > > On Mon, Dec 09, 2024 at 03:36:15PM +0530, Amit Kapila wrote:
> > > > It couldn't solve the problem completely even in
On Tue, Dec 10, 2024 at 8:54 AM vignesh C wrote:
>
> On Tue, 10 Dec 2024 at 04:56, Michael Paquier 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
On Tue, Dec 10, 2024 at 03:24:19AM +, 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(CacheMem
On Tue, 10 Dec 2024 at 04:56, Michael Paquier 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] -
> >
On Tue, Dec 10, 2024 at 2:17 AM Masahiko Sawada wrote:
>
> On Mon, Dec 9, 2024 at 2:06 AM Amit Kapila wrote:
> >
> > On Thu, Dec 5, 2024 at 2:56 PM Masahiko Sawada
> > wrote:
> > >
> > > >
> > > > I realized that this patch cannot be backpatched because it introduces
> > > > a new
> > > > fiel
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(CacheMemo
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/OS0PR01MB57166A4DA0ABBB94F2FBB2
On Mon, Dec 9, 2024 at 2:06 AM Amit Kapila wrote:
>
> On Thu, Dec 5, 2024 at 2:56 PM Masahiko Sawada 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
On Thu, Dec 5, 2024 at 2:56 PM Masahiko Sawada 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 bac
On Fri, Dec 06, 2024 at 08:23:00AM +, 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
On Thursday, December 5, 2024 11:39 PM Euler Taveira 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 co
On Thursday, December 5, 2024 12:52 PM Michael Paquier
wrote:
Hi,
>
> On Thu, Dec 05, 2024 at 04:31:56AM +, 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
> >
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
> cont
On Thu, Dec 5, 2024 at 1:32 PM Zhijie Hou (Fujitsu)
wrote:
>
> On Wednesday, December 4, 2024 7:39 PM Michael Paquier
> wrote:
> >
> > On Wed, Dec 04, 2024 at 06:42:55AM +, Zhijie Hou (Fujitsu) wrote:
> > > I can try to write a patch if no one else is working on this.
> >
> > If you have som
On Thu, Dec 05, 2024 at 04:31:56AM +, 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]
> htt
On Wednesday, December 4, 2024 7:39 PM Michael Paquier
wrote:
>
> On Wed, Dec 04, 2024 at 06:42:55AM +, 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.
On Wed, Dec 04, 2024 at 06:42:55AM +, 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
signature.asc
Description: PGP signature
On Wednesday, December 4, 2024 2:22 PM Michael Paquier
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)
> wrote:
> >> It appears there is an additional memory leak caused by allocating
> >> publication names within t
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)
> 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 b
On Wed, Dec 4, 2024 at 7:39 AM Zhijie Hou (Fujitsu)
wrote:
>
> On Wednesday, December 4, 2024 8:55 AM Michael Paquier
> 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
On Wednesday, December 4, 2024 8:55 AM Michael Paquier
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
> >
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
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 prop
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 arra
On 2024-Dec-03, Amit Kapila wrote:
> On Tue, Dec 3, 2024 at 4:03 PM Alvaro Herrera 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
On Tue, Dec 3, 2024 at 4:03 PM Alvaro Herrera 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 pr
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 a
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.
On Tuesday, December 3, 2024 4:43 AM Alvaro Herrera
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
On Tue, Dec 3, 2024 at 11:57 AM Michael Paquier 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 (publicati
On Tuesday, December 3, 2024 4:28 PM Amit Kapila
wrote:
>
> On Tue, Dec 3, 2024 at 11:57 AM Michael Paquier
> 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 struc
On Tue, Dec 3, 2024 at 12:17 PM Michael Paquier 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 w
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
signature.asc
Description: PGP signature
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
On Tue, Dec 3, 2024 at 5:47 PM Michael Paquier 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
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 t
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 s
On Tue, Dec 3, 2024 at 2:12 AM Alvaro Herrera 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'
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 w
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 Publicatio
On Mon, Dec 2, 2024 at 12:49 PM Michael Paquier 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,
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
On Mon, Dec 2, 2024 at 7:19 AM Michael Paquier 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
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
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(),
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/zz7srcbxxhoyn...@paquier.x
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/zz7srcbxxhoyn...@paquier.xyz
Reproducing the problem is quite simple, and can be done wit
63 matches
Mail list logo
