On 2018/11/15 22:57, Robert Haas wrote: > On Thu, Nov 15, 2018 at 12:38 AM Michael Paquier <mich...@paquier.xyz> wrote: >> On Thu, Nov 15, 2018 at 01:38:55PM +0900, Amit Langote wrote: >>> I've fixed 0001 again to re-order the code so that allocations happen the >>> correct context and now tests pass with the rebased patches. >> >> I have been looking at 0001, and it seems to me that you make even more >> messy the current situation. Coming to my point: do we have actually >> any need to set rel->rd_pdcxt and rel->rd_partdesc at all if a relation >> has no partitions? It seems to me that we had better set rd_pdcxt and >> rd_partdesc to NULL in this case. > > I think that's unrelated to this patch, as Amit also says, but I have > to say that the last few hunks of the rebased version of this patch do > not make a lot of sense to me. This patch is supposed to be reducing > list construction, and the original version did that, but the rebased > version adds a partition_bounds_copy() operation, whereas my version > did not add any expensive operations - it only removed some cost. I > don't see why anything I changed should necessitate such a change, nor > does it seem like a good idea.
The partition_bounds_copy() is not because of your changes, it's there in HEAD. The reason we do that is because partition_bounds_create allocates the memory for the PartitionBoundInfo it returns along with other temporary allocations in CurrentMemoryContext. But we'd need to copy it into rd_pdcxt before calling it a property of rd_partdesc, so the partition_bounds_copy. Maybe partition_bounds_create() should've had a MemoryContext argument to pass it the context we want it to create the PartitionBoundInfo in. That way, we can simply pass rd_pdcxt to it and avoid making a copy. As is, we're now allocating two copies of PartitionBoundInfo, one in the CurrentMemoryContext and another in rd_pdcxt, whereas the previous code would only allocate the latter. Maybe we should fix it as being a regression. Thanks, Amit
