Re: BUG #15321: XMLTABLE leaks memory like crazy

2018-08-12 12:25 GMT+02:00 Andrew Gierth <andrew(at)tao11(dot)riddles(dot)org(dot)uk>:

> >>>>> "Pavel" == Pavel Stehule <pavel(dot)stehule(at)gmail(dot)com> writes:
>
> >> No, tuplestore_putvalues is only responsible for the memory it
> >> allocates itself, which belongs to the tuplestore, it has nothing to
> >> do with the memory allocated by its caller - and XmlTableGetValue
> >> does quite a few allocations.
>
> Pavel> Maybe I used wrong words. Sorry, my English lang is not good.
>
> Pavel> In master tfuncLoadRows switch to tstate->perValueCxt. You
> Pavel> change it by switch econtext->ecxt_per_tuple_memory what is
> Pavel> wrong I thing.
>
> Pavel> If we don't change memory context, we will stay inside
> Pavel> perValueCxt. And this context will be cleaned outside.
>
> Yes, but it won't be cleaned up until _all_ the rows from a single call
> have been generated, which means that the allocations from GetValue have
> been (uselessly) accumulating during this time, wasting memory.
>
> We do need a context that is reset for every output row, as perValueCxt
> used to be. I don't see why this is even in question.
>
> >> Before, you were using perValueCxt and resetting it once per GetValue
> >> call. Since my patch takes perValueCxt to use for another purpose
> >> instead, it needs to be replaced, and econtext->ecxt_per_tuple_memory
> >> is suitable for the job (and consistent with functionscan).
>
> Pavel> I think so using cxt_per_tuple_memory is not necessary - and my
> Pavel> patch is working too.
>
> Working, just using more memory than it needs to.
>
> Pavel> Just I removed MemoryContextReset(tstate->perValueCxt) after
> Pavel> tuplestore_putvalues. It is possible, because
> Pavel> tstate->perValueCxt is cleaned immediately in caller
> Pavel> tfuncFetchRows function.
>
> But that's not "immediately" because tfuncLoadRows is looping over the
> FetchRow call, and calling GetValue for each column in that row, and in
> your version it is _not cleaning up memory in that loop_.
>

ok, now I am maybe understand to your motivation.

Usually, loading row should be memory cheap operation, but sure some bytes
it can take.

Then I don't like too much using ecxt_per_tuple_memory for this. Maybe
better to create own short life context for purpose. Or do better comments
about using this memory context for very short life task, please.

Regards

Pavel

>
> --
> Andrew (irc:RhodiumToad)
>