Re: ATTACH/DETACH PARTITION CONCURRENTLY

On 2018/11/15 22:57, Robert Haas wrote:
> On Thu, Nov 15, 2018 at 12:38 AM Michael Paquier <michael(at)paquier(dot)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