Re: expanding inheritance in partition bound order

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_f8(at)lab(dot)ntt(dot)co(dot)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