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

> +   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
> 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.



Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:

Reply via email to