On 17 August 2017 at 06:39, Amit Langote <langote_amit...@lab.ntt.co.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...@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.
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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
