Re: speeding up planning with partitions

On 2018/09/04 22:24, David Rowley wrote:
> On 30 August 2018 at 00:06, Amit Langote <Langote_Amit_f8(at)lab(dot)ntt(dot)co(dot)jp> wrote:
>> With various overheads gone thanks to 0001 and 0002, locking of all
>> partitions via find_all_inheritos can be seen as the single largest
>> bottleneck, which 0003 tries to address. I've kept it a separate patch,
>> because I'll need to think a bit more to say that it's actually to safe to
>> defer locking to late planning, due mainly to the concern about the change
>> in the order of locking from the current method. I'm attaching it here,
>> because I also want to show the performance improvement we can expect with it.
>
> For now, find_all_inheritors() locks the tables in ascending Oid
> order. This makes sense with inheritance parent tables as it's much
> cheaper to sort on this rather than on something like the table's
> namespace and name. I see no reason why what we sort on really
> matters, as long as we always sort on the same thing and the key we
> sort on is always unique so that the locking order is well defined.
>
> For partitioned tables, there's really not much sense in sticking to
> the same lock by Oid order. The order of the PartitionDesc is already
> well defined so I don't see any reason why we can't just perform the
> locking in PartitionDesc order.

The reason we do the locking with find_all_inheritors for regular queries
(planner (expand_inherited_rtentry) does it for select/update/delete and
executor (ExecSetupPartitionTupleRouting) for insert) is that that's the
order used by various DDL commands when locking partitions, such as when
adding a column. So, we'd have one piece of code locking partitions by
Oid order and others by canonical partition bound order or PartitionDesc
order. I'm no longer sure if that's problematic though, about which more
below.

> This would mean you could perform the
> locking of the partitions once pruning is complete somewhere around
> add_rel_partitions_to_query(). Also, doing this would remove the need
> for scanning pg_inherits during find_all_inheritors() and would likely
> further speed up the planning of queries to partitioned tables with
> many partitions.

That's what happens with 0003; note the following hunk in it:

@@ -1555,14 +1555,15 @@ expand_inherited_rtentry(PlannerInfo *root,
RangeTblEntry *rte, Index rti)
lockmode = AccessShareLock;

/* Scan for all members of inheritance set, acquire needed locks */
- inhOIDs = find_all_inheritors(parentOID, lockmode, NULL);
+ if (rte->relkind != RELKIND_PARTITIONED_TABLE)
+ inhOIDs = find_all_inheritors(parentOID, lockmode, NULL);

> I wrote a function named
> get_partition_descendants_worker() to do this in patch 0002 in [1] (it
> may have been a bad choice to have made this a separate function
> rather than just part of find_all_inheritors() as it meant I had to
> change a bit too much code in tablecmds.c). There might be something
> you can salvage from that patch to help here.
>
> [1] https://www.postgresql.org/message-id/CAKJS1f9QjUwQrio20Pi%3DyCHmnouf4z3SfN8sqXaAcwREG6k0zQ%40mail.gmail.com

Actually, I had written a similar patch to replace the usages of
find_all_inheritors and find_inheritance_children by different
partitioning-specific functions which would collect the the partition OIDs
from the already open parent table's PartitionDesc, more or less like the
patch you mention does. But I think we don't need any new function(s) to
do that, that is, we don't need to change all the sites that call
find_all_inheritors or find_inheritance_children in favor of new functions
that return partition OIDs in PartitionDesc order, if only *because* we
want to change planner to lock partitions in the PartitionDesc order. I'm
failing to see why the difference in locking order matters. I understood
the concern as that locking partitions in different order could lead to a
deadlock if concurrent backends request mutually conflicting locks, but
only one of the conflicting backends, the one that got lock on the parent,
would be allowed to lock children. Thoughts on that?

Thanks,
Amit