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
Currently there isn't. Robert suggested a RelationLockHeldByMe(Oid) ,
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
> + 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".
> +#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
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
> 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
> 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.
Sent via pgsql-hackers mailing list (firstname.lastname@example.org)
To make changes to your subscription: