Re: [report] memory leaks in COPY FROM on partitioned table

On 2018/08/02 5:38, Alvaro Herrera wrote:
> On 2018-Jul-24, Amit Langote wrote:
>
>> Your patch takes care of allocation happening inside
>> get_partition_for_tuple, but as you mention there might be others in its
>> caller ExecFindPartition. So, I think we should switch to the per-tuple
>> context in ExecFindPartition.
>
> Right, makes sense. Pushed that way.

Thanks.

> I also moved the
> ExecFetchSlotTuple call to happen after the memcxt change, because it
> seemed to me that it may be possible for tuple_expand to allocate memory
> (if not, it's not obvious).

Oops, you're right.

> I also reworded some comments -- hope not
> to have broken anything too bad there. I also renamed variable
> "parent", which confused the heck out of me.

TBH, "parent" had started to become distracting even for me, who gave it
that name to begin with.

> I had conflicts when applying this in master after developing it in
> pg11, because of some new development there (and my variable rename). I
> really hope we don't open the pg13 tree as early as we opened the pg12
> one ...
>
>> When I tried to do that, I discovered that we have to be careful about
>> releasing some of the memory that's allocated in ExecFindPartition
>> ourselves instead of relying on the reset of per-tuple context to take
>> care of it. That's because some of the structures that ExecFindPartition
>> assigns the allocated memory to are cleaned up at the end of the query, by
>> when it's too late to try to release per-tuple memory. So, the patch I
>> ended up with is slightly bigger than simply adding a
>> MemoryContextSwitchTo() call at the beginning of ExecFindPartition.
>
> Yeah, that stuff looks a bit brittle. I wish I had an idea on how to
> make it less so. Thanks for taking care of that.

Just to recap in the light of this commit, we cannot do a full
ExecDropSingleTupleTableSlot right in ExecFindPartition, because we may
want to use the slot again for the next tuple. Per-tuple memory taken up
by the copy of the tuple in the slot would be released by resetting
per-tuple context in which it is (now) allocated, even if we didn't
release it ourselves. But we also need to do some bookkeeping, such as
setting the slot's tts_shouldFree to false. Hence the explicit
ExecClearTuple(), which both frees the memory and does the necessary
bookkeeping.

Thanks,
Amit