Re: expanding inheritance in partition bound order

On 17 August 2017 at 06:39, Amit Langote <Langote_Amit_f8(at)lab(dot)ntt(dot)co(dot)jp> wrote:
> Hi Amit,
>
> Thanks for the comments.
>
> On 2017/08/16 20:30, Amit Khandekar wrote:
>> On 16 August 2017 at 11:06, Amit Langote <Langote_Amit_f8(at)lab(dot)ntt(dot)co(dot)jp> wrote:
>>
>>> Attached updated patches.
>>
>> Thanks Amit for the patches.
>>
>> I too agree with the overall approach taken for keeping the locking
>> order consistent: it's best to do the locking with the existing
>> find_all_inheritors() since it is much cheaper than to lock them in
>> partition-bound order, the later being expensive since it requires
>> opening partitioned tables.
>
> Yeah. Per the Robert's design idea, we will always do the *locking* in
> the order determined by find_all_inheritors/find_inheritance_children.
>
>>> I haven't yet done anything about changing the timing of opening and
>>> locking leaf partitions, because it will require some more thinking about
>>> the required planner changes. But the above set of patches will get us
>>> far enough to get leaf partition sub-plans appear in the partition bound
>>> order (same order as what partition tuple-routing uses in the executor).
>>
>> So, I believe none of the changes done in pg_inherits.c are essential
>> for expanding the inheritence tree in bound order, right ?
>
> Right.
>
> The changes to pg_inherits.c are only about recognizing partitioned tables
> in an inheritance hierarchy and putting them ahead in the returned list.
> Now that I think of it, the title of the patch (teach pg_inherits.c about
> "partitioning") sounds a bit confusing. In particular, the patch does not
> teach it things like partition bound order, just that some tables in the
> hierarchy could be partitioned tables.
>
>> I am not
>> sure whether we are planning to commit these two things together or
>> incrementally :
>> 1. Expand in bound order
>> 2. Allow for locking only the partitioned tables first.
>>
>> For #1, the changes in pg_inherits.c are not required (viz, keeping
>> partitioned tables at the head of the list, adding inhchildparted
>> column, etc).
>
> Yes. Changes to pg_inherits.c can be committed completely independently
> of either 1 or 2, although then there would be nobody using that capability.
>
> About 2: I think the capability to lock only the partitioned tables in
> expand_inherited_rtentry() will only be used once we have the patch to do
> the necessary planner restructuring that will allow us to defer child
> table locking to some place that is not expand_inherited_rtentry(). I am
> working on that patch now and should be able to show something soon.
>
>> If we are going to do #2 together with #1, then in the patch set there
>> is no one using the capability introduced by #2. That is, there are no
>> callers of find_all_inheritors() that make use of the new
>> num_partitioned_children parameter. Also, there is no boolean
>> parameter for find_all_inheritors() to be used to lock only the
>> partitioned tables.
>>
>> I feel we should think about
>> 0002-Teach-pg_inherits.c-a-bit-about-partitioning.patch later, and
>> first get the review done for the other patches.
>
> I think that makes sense.
>
>> I see that RelationGetPartitionDispatchInfo() now returns quite a
>> small subset of what it used to return, which is good. But I feel for
>> expand_inherited_rtentry(), RelationGetPartitionDispatchInfo() is
>> still a bit heavy. We only require the oids, so the
>> PartitionedTableInfo data structure (including the pd->indexes array)
>> gets discarded.
>
> Maybe we could make the output argument optional, but I don't see much
> point in being too conservative here. Building the indexes array does not
> cost us that much and if a not-too-distant-in-future patch could use that
> information somehow, it could do so for free.

Ok, so these changes are mostly kept keeping in mind some future
use-cases. Otherwise, I was thinking we could just keep a light-weight
function to generate the oids, and keep the current
RelationGetPartitionDispatchInfo() intact.

Anyways, some more comments :

In ExecSetupPartitionTupleRouting(), not sure why ptrinfos array is an
array of pointers. Why can't it be an array of
PartitionTupleRoutingInfo structure rather than pointer to that
structure ?

diff --git a/src/backend/catalog/partition.c b/src/backend/catalog/partition.c
+ * Close all the leaf partitions and their indices.
*
Above comment needs to be shifted a bit down to the subsequent "for"
loop where it's actually applicable.

* node->mt_partition_dispatch_info[0] corresponds to the root partitioned
* table, for which we didn't create tupslot.
Above : node->mt_partition_dispatch_info[0] => node->mt_ptrinfos[0]

/*
* XXX- do we need a pinning mechanism for partition descriptors
* so that there references can be managed independently of
* the parent relcache entry? Like PinPartitionDesc(partdesc)?
*/
pd->partdesc = partdesc;

Any idea if the above can be handled ? I am not too sure.

>
> Thanks,
> Amit
>

--
Thanks,
-Amit Khandekar
EnterpriseDB Corporation
The Postgres Database Company