Re: BUG #15592: Memory overuse with subquery containing unnest() andset operations (11.x regression)

Hi,

On 2019-02-09 10:09:57 -0500, Tom Lane wrote:
> Andres Freund <andres@anarazel.de> writes:
> > On 2019-02-09 09:34:41 -0500, Tom Lane wrote:
> >> Andres Freund <andres@anarazel.de> writes:
> >>> ... Given that, I think it's ok
> >>> to not explicitly shutdown the expr context.
> 
> >> Uh, what?
> 
> > Well, you explicitly removed the surrounding reasoning. What's the issue
> > you see here? The generated expression cannot contain anything that uses
> > shutdown callbacks
> 
> Even if that happens to be true today, it certainly looks like a booby
> trap primed and ready to bite somebody in the future.  An ExprContext
> that fails to provide one of the basic services of an ExprContext ---
> indeed, the *only* basic service of an ExprContext, since whatever else
> it does is just a memory context feature --- doesn't sound like a great
> plan to me.
> 
> What is it you're actually hoping to do by removing this guarantee?
> (I apologize for not having been paying attention to this thread,
> but I do have finite bandwidth.)

I think we probably can do better in master, but I don't see a good
solution that's not expensive in v11. The tuple hash table can be
created / destroyed at a prodigious rate before 317ffdfea / 356687bd825,
and I don't see a good way to get rid of needing an ExprContext created
therein. We could register a callback on the memory context to drop the
ExprContext, but unfortunately dropping ExprContexts retail isn't
particularly cheap as it has to go through a singly linked list
(something we ought to fix one day by using a doubly linked list, but
certainly not a minor release).  After the aforementioned changes that's
much less an issue, but without an API break, I don't see how we can
make sure that external code isn't broken by forcing it to only reset
tuple hash tables rather than recreating them via a memory context
reset.

FWIW, for me the most basic service of ExprContext is to provide
scan/inner/out slot.


> >> This doesn't really seem like the kind of patch to push on a release
> >> weekend.  At this point you can't even be confident of getting a readout
> >> from every active buildfarm member.
> 
> > Yea, I'd hoped to push this earlier, but unfortune family issues +
> > related travel + work travel prevented me from getting to this
> > earlier. The memory leak is significant, the patch hasn't materially
> > changed in the two weeks since it has been posted, and there's nothing
> > operating system / architecture related, which I think makes this
> > acceptable.
> 
> I'm thinking more of the valgrind and clobber_cache_always members,
> which are pretty slow by definition.

I'd run valgrind locally (for the regression/isolation tests, not for
the tap tests, but that seems more then plenty for this change). I
didn't run with clobber caches always, but that seems pretty unlikely to
be affected by this change.

Greetings,

Andres Freund