Re: Memory leak from ExecutorState context?

On Mon, 20 Mar 2023 09:32:17 +0100
Tomas Vondra <tomas(dot)vondra(at)enterprisedb(dot)com> wrote:

> >> * Patch 1 could be rebased/applied/backpatched
> >
> > Would it help if I rebase Patch 1 ("move BufFile stuff into separate
> > context")?
>
> Yeah, I think this is something we'd want to do. It doesn't change the
> behavior, but it makes it easier to track the memory consumption etc.

Will do this week.

> I think focusing on the backpatchability is not particularly good
> approach. It's probably be better to fist check if there's any hope of
> restarting the work on BNLJ, which seems like a "proper" solution for
> the future.

Backpatching worth some consideration. Balancing is almost there and could help
in 16. As time is flying, it might well miss release 16, but maybe it could be
backpacthed in 16.1 or a later minor release? But what if it is not eligible for
backpatching? We would stall another year without having at least an imperfect
stop-gap solution (sorry for picking your words ;)).

BNJL and/or other considerations are for 17 or even after. In the meantime,
Melanie, who authored BNLJ, +1 the balancing patch as it can coexists with other
discussed solutions. No one down vote since then. Melanie, what is your
opinion today on this patch? Did you change your mind as you worked for many
months on BNLJ since then?

> On 3/19/23 20:31, Justin Pryzby wrote:
> > On Fri, Mar 17, 2023 at 05:41:11PM +0100, Tomas Vondra wrote:
> >>>> * Patch 2 is worth considering to backpatch
> >>
> >> I'm not quite sure what exactly are the numbered patches, as some of the
> >> threads had a number of different patch ideas, and I'm not sure which
> >> one was/is the most promising one.
> >
> > patch 2 is referring to the list of patches that was compiled
> > https://www.postgresql.org/message-id/20230310195114.6d0c5406%40karst
>
> Ah, I see - it's just the "rebalancing" patch which minimizes the total
> amount of memory used (i.e. grow work_mem a bit, so that we don't
> allocate too many files).
>
> Yeah, I think that's the best we can do without reworking how we spill
> data (slicing or whatever).

Indeed, that's why I was speaking about backpatching for this one:

* it surely helps 16 (and maybe previous release) in such skewed situation
* it's constrained
* it's not too invasive, it doesn't shake to whole algorithm all over the place
* Melanie was +1 for it, no one down vote.

What need to be discussed/worked:

* any regressions for existing queries running fine or without OOM?
* add the bufFile memory consumption in the planner considerations?

> I think the spilling is "nicer" in that it actually enforces work_mem
> more strictly than (a),

Sure it enforces work_mem more strictly, but this is a discussion for 17 or
later in my humble opinion.

> but we should probably spend a bit more time on
> the exact spilling strategy. I only really tried two trivial ideas, but
> maybe we can be smarter about allocating / sizing the files? Maybe
> instead of slices of constant size we could/should make them larger,
> similarly to what log-structured storage does.
>
> For example we might double the number of batches a file represents, so
> the first file would be for batch 1, the next one for batches 2+3, then
> 4+5+6+7, then 8-15, then 16-31, ...
>
> We should have some model calculating (i) amount of disk space we would
> need for the spilled files, and (ii) the amount of I/O we do when
> writing the data between temp files.

And:

* what about Robert's discussion on uneven batch distribution? Why is it
ignored? Maybe there was some IRL or off-list discussions? Or did I missed
some mails?
* what about dealing with all normal batch first, then revamp in
freshly emptied batches the skewed ones, spliting them if needed, then rince &
repeat? At some point, we would probably still need something like slicing
and/or BNLJ though...

> let's revive the rebalance and/or spilling patches. Imperfect but better than
> nothing.

+1 for rebalance. I'm not even sure it could make it to 16 as we are running
out time, but it worth to try as it's the closest one that could be
reviewed and ready'd-for-commiter.

I might lack of ambition, but spilling patches seems too much to make it for
16. It seems to belongs with other larger patches/ideas (patches 4 a 5 in my sum
up). But this is just my humble feeling.

> And then in the end we can talk about if/what can be backpatched.
>
> FWIW I don't think there's a lot of rush, considering this is clearly a
> matter for PG17. So the summer CF at the earliest, people are going to
> be busy until then.

100% agree, there's no rush for patches 3, 4 ... and 5.

Thanks!