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
