Re: ATTACH/DETACH PARTITION CONCURRENTLY

On 2018/11/15 14:38, Michael Paquier 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.

As things stand today, rd_partdesc of a partitioned table must always be
non-NULL. In fact, there are many places in the backend code that Assert it:

tablecmds.c: ATPrepDropNotNull()

if (rel->rd_rel->relkind == RELKIND_PARTITIONED_TABLE)
{
PartitionDesc partdesc = RelationGetPartitionDesc(rel);

Assert(partdesc != NULL);

prepunion.c: expand_partitioned_rtentry()

PartitionDesc partdesc = RelationGetPartitionDesc(parentrel);

check_stack_depth();

/* A partitioned table should always have a partition descriptor. */
Assert(partdesc);

plancat.c: set_relation_partition_info()

partdesc = RelationGetPartitionDesc(relation);
partkey = RelationGetPartitionKey(relation);
rel->part_scheme = find_partition_scheme(root, relation);
Assert(partdesc != NULL && rel->part_scheme != NULL);

Maybe there are others in a different form.

If there are no partitions, nparts is 0, and other fields are NULL, though
rd_partdesc itself is never NULL.

If we want to redesign that and allow it to be NULL until some code in the
backend wants to use it, then maybe we can consider doing what you say.
But, many non-trivial operations on partitioned tables require the
PartitionDesc, so there is perhaps not much point to designing it such
that rd_partdesc is set only when needed, because it will be referenced
sooner than later. Maybe, we can consider doing that sort of thing for
boundinfo, because it's expensive to build, and not all operations want
the canonicalized bounds.

Thanks,
Amit