Re: expanding inheritance in partition bound order

On Wed, Aug 16, 2017 at 11:06 AM, Amit Langote
<Langote_Amit_f8@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?

        /*
-        * 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, so removed it in the attached patch.

+   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. So removed the hunk in the
attached set.

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.

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

        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.

I am still reviewing changes in this patch.

-- 
Best Wishes,
Ashutosh Bapat
EnterpriseDB Corporation
The Postgres Database Company