From: | Jehan-Guillaume de Rorthais <jgdr(at)dalibo(dot)com> |
---|---|
To: | Melanie Plageman <melanieplageman(at)gmail(dot)com> |
Cc: | Tomas Vondra <tomas(dot)vondra(at)enterprisedb(dot)com>, pgsql-hackers(at)lists(dot)postgresql(dot)org, Konstantin Knizhnik <knizhnik(at)garret(dot)ru>, Thomas Munro <thomas(dot)munro(at)gmail(dot)com> |
Subject: | Re: Memory leak from ExecutorState context? |
Date: | 2023-05-04 17:30:06 |
Message-ID: | 20230504193006.1b5b9622@karst |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | pgsql-hackers |
Hi,
On Fri, 21 Apr 2023 16:44:48 -0400
Melanie Plageman <melanieplageman(at)gmail(dot)com> wrote:
> On Fri, Apr 7, 2023 at 8:01 PM Jehan-Guillaume de Rorthais
> <jgdr(at)dalibo(dot)com> wrote:
> >
> > On Fri, 31 Mar 2023 14:06:11 +0200
> > Jehan-Guillaume de Rorthais <jgdr(at)dalibo(dot)com> wrote:
> >
> > > > [...]
> > > > >> Hmmm, not sure is WARNING is a good approach, but I don't have a
> > > > >> better idea at the moment.
> > > > >
> > > > > I stepped it down to NOTICE and added some more infos.
> > > > >
> > > > > [...]
> > > > > NOTICE: Growing number of hash batch to 32768 is exhausting allowed
> > > > > memory (137494656 > 2097152)
> > > > [...]
> > > >
> > > > OK, although NOTICE that may actually make it less useful - the default
> > > > level is WARNING, and regular users are unable to change the level. So
> > > > very few people will actually see these messages.
> > [...]
> > > Anyway, maybe this should be added in the light of next patch, balancing
> > > between increasing batches and allowed memory. The WARNING/LOG/NOTICE
> > > message could appears when we actually break memory rules because of some
> > > bad HJ situation.
> >
> > So I did some more minor editions to the memory context patch and start
> > working on the balancing memory patch. Please, find in attachment the v4
> > patch set:
> >
> > * 0001-v4-Describe-hybrid-hash-join-implementation.patch:
> > Adds documentation written by Melanie few years ago
> > * 0002-v4-Allocate-hash-batches-related-BufFile-in-a-dedicated.patch:
> > The batches' BufFile dedicated memory context patch
>
> This is only a review of the code in patch 0002 (the patch to use a more
> granular memory context for multi-batch hash join batch files). I have
> not reviewed the changes to comments in detail either.
Ok.
> I think the biggest change that is needed is to implement this memory
> context usage for parallel hash join.
Indeed.
> To implement a file buffer memory context for multi-patch parallel hash
> join [...]
Thank you for your review and pointers!
After (some days off and then) studying the parallel code, I end up with this:
1. As explained by Melanie, the v5 patch sets accessor->context to fileCxt.
2. BufFile buffers were wrongly allocated in ExecutorState context for
accessor->read|write_file, from sts_puttuple and sts_parallel_scan_next when
they first need to work with the accessor FileSet.
The v5 patch now allocate them in accessor->context, directly in
sts_puttuple and sts_parallel_scan_next. This avoids to wrap each single
call of these functions inside MemoryContextSwitchTo calls. I suppose this
is correct as the comment about accessor->context says
"/* Memory context for **buffers**. */" in struct SharedTuplestoreAccessor.
3. accessor->write_chunk is currently already allocated in accessor->context.
In consequence, this buffer is now allocated in the fileCxt instead
of hashCxt context.
This is a bit far fetched, but I suppose this is ok as it act as a second
level buffer, on top of the BufFile.
4. accessor->read_buffer is currently allocated in accessor->context as well.
This buffer holds tuple read from the fileset. This is still a buffer, but
not related to any file anymore...
Because of 3 and 4, and the gross context granularity of SharedTuplestore
related-code, I'm now wondering if "fileCxt" shouldn't be renamed "bufCxt".
Regards,
Attachment | Content-Type | Size |
---|---|---|
0001-v5-Describe-hybrid-hash-join-implementation.patch | text/x-patch | 2.8 KB |
0002-v5-Allocate-hash-batches-related-BufFile-in-a-dedicated.patch | text/x-patch | 11.1 KB |
From | Date | Subject | |
---|---|---|---|
Next Message | Alvaro Herrera | 2023-05-04 17:59:17 | Re: A bug with ExecCheckPermissions |
Previous Message | Andres Freund | 2023-05-04 16:57:38 | Re: pg_stat_io not tracking smgrwriteback() is confusing |