Hi Ashutosh, Thanks for the review and the updated patch.
On 2017/08/16 21:48, Ashutosh Bapat wrote: > On Wed, Aug 16, 2017 at 11:06 AM, Amit Langote > <langote_amit...@lab.ntt.co.jp> wrote: > >> >>> This patch series is blocking a bunch of other things, so it would be >>> nice if you could press forward with this quickly. >> >> Attached updated patches. >> > > Review for 0001. The attached patch has some minor changes to the > comments and code. > > + * All the relations in the partition tree (including 'rel') must have been > + * locked (using at least the AccessShareLock) by the caller. > > It would be good if we can Assert this in the function. But I couldn't find a > way to check whether the backend holds a lock of required strength. Is there > any? Currently there isn't. Robert suggested a RelationLockHeldByMe(Oid) [1], which is still a TODO on my plate. > /* > - * We locked all the partitions above including the leaf partitions. > - * Note that each of the relations in *partitions are eventually > - * closed by the caller. > + * All the partitions were locked above. Note that the relcache > + * entries will be closed by ExecEndModifyTable(). > */ > I don't see much value in this hunk, I thought the new text was a bit clearer, but maybe that's just me. Will remove. > + list_free(all_parts); > ExecSetupPartitionTupleRouting() will be called only once per DML statement. > Leaking the memory for the duration of DML may be worth the time spent > in the traversing > the list and freeing each cell independently. Fair enough, will remove the list_free(). > 0002 review > + > + <row> > + <entry><structfield>inhchildparted</structfield></entry> > + <entry><type>bool</type></entry> > + <entry></entry> > + <entry> > + This is <literal>true</> if the child table is a partitioned table, > + <literal>false</> otherwise > + </entry> > + </row> > In the catalogs we are using full "partitioned" e.g. pg_partitioned_table. May > be we should name the column as "inhchildpartitioned". Sure. > +#define OID_CMP(o1, o2) \ > + ((o1) < (o2) ? -1 : ((o1) > (o2) ? 1 : 0)); > Instead of duplicating the logic in this macro and oid_cmp(), we may want to > call this macro in oid_cmp()? Or simply call oid_cmp() from inhchildinfo_cmp() > passing pointers to the OIDs; a pointer indirection would be costly, but still > maintainable. Actually, I avoided using oid_cmp exactly for that reason. > + if (num_partitioned_children) > + *num_partitioned_children = my_num_partitioned_children; > + > Instead of this conditional, why not to make every caller pass a pointer to > integer. The callers will just ignore the value if they don't want it. If we > do > this change, we can get rid of my_num_partitioned_children variable and > directly update the passed in pointer variable. There are a bunch of callers of find_all_inheritors() and find_inheritance_children. Changes to make them all declare a pointless variable seemed off to me. The conditional in question doesn't seem to be that expensive. (To be fair, the one introduced in find_all_inheritors() kind of is as implemented by the patch, because it's executed for every iteration of the foreach(l, rels_list) loop, which I will fix.) > > inhrelid = ((Form_pg_inherits) GETSTRUCT(inheritsTuple))->inhrelid; > - if (numoids >= maxoids) > + is_partitioned = ((Form_pg_inherits) > + GETSTRUCT(inheritsTuple))->inhchildparted; > Now that we are fetching two members from Form_pg_inherits structure, may be > we > should use a local variable > Form_pg_inherits inherits_tuple = GETSTRUCT(inheritsTuple); > and use that to fetch its members. Sure, will do. > I am still reviewing changes in this patch. Okay, will wait for more comments before sending the updated patches. Thanks, Amit [1] https://www.postgresql.org/message-id/CA%2BTgmobwbh12OJerqAGyPEjb_%2B2y7T0nqRKTcjed6L4NTET6Fg%40mail.gmail.com -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
