Re: [HACKERS] expanding inheritance in partition bound order

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@lab.ntt.co.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.

> Also, RelationGetPartitionDispatchInfo() has to call get_rel_relkind()
> for each child. In expand_inherited_rtentry(), we anyway have to open
> all the child tables, so we get the partition descriptors for each of
> the children for free. So how about, in expand_inherited_rtentry(), we
> traverse the partition tree using these descriptors similar to how it
> is traversed in RelationGetPartitionDispatchInfo() ? May be to avoid
> code duplication for traversing, we can have a common API.

As mentioned, one goal I'm seeking is to avoid having to open the child
tables in expand_inherited_rtentry().

Thanks,
Amit