Re: Add pg_partition_root to get top-most parent of a partition tree - Mailing list pgsql-hackers

Hi,

On 2018/12/12 10:48, Michael Paquier wrote:
> On Fri, Dec 07, 2018 at 11:46:05AM +0900, Michael Paquier wrote:
>> On Thu, Dec 06, 2018 at 10:48:59PM -0300, Alvaro Herrera wrote:
>>> I think adding a pg_partition_root() call to as many pg_partition_tree
>>> tests as you modified is overkill ... OTOH I'd have one test that
>>> invokes pg_partition_tree(pg_partition_root(some-partition)) to verify
>>> that starting from any point in the tree you get the whole tree.
>>
>> Good idea, thanks for the input.
> 
> The recent commit cc53123 has fixed a couple of issues with
> pg_partition_tree, so attached is a rebased patch which similarly makes
> pg_partition_root return NULL for unsupported relkinds and undefined
> relations.  I have also simplified the tests based on Alvaro's
> suggestion to use pg_partition_tree(pg_partition_root(partfoo)).

Thanks for working on this.  I have looked at this patch and here are some
comments.

+        Return the top-most parent of a partition tree for the given
+        partitioned table or partitioned index.

Given that pg_partition_root will return a valid result for any relation
that can be part of a partition tree, it seems strange that the above
sentence says "for the given partitioned table or partitioned index".  It
should perhaps say:

Return the top-most parent of the partition tree to which the given
relation belongs

+/*
+ * Perform several checks on a relation on which is extracted some
+ * information related to its partition tree.  Returns false if the
+ * relation cannot be processed, in which case it is up to the caller
+ * to decide what to do, by either raising an error or doing something
+ * else.
+ */
+static bool
+check_rel_for_partition_info(Oid relid)
+{
+    char        relkind;
+
+    /* Check if relation exists */
+    if (!SearchSysCacheExists1(RELOID, ObjectIdGetDatum(relid)))
+        return false;

This should be checked in the caller imho.

+
+    relkind = get_rel_relkind(relid);
+
+    /* Only allow relation types that can appear in partition trees. */
+    if (relkind != RELKIND_RELATION &&
+        relkind != RELKIND_FOREIGN_TABLE &&
+        relkind != RELKIND_INDEX &&
+        relkind != RELKIND_PARTITIONED_TABLE &&
+        relkind != RELKIND_PARTITIONED_INDEX)
+        return false;
+
+    return true;
+}

I can't imagine this function growing more code to perform additional
checks beside just checking the relkind, so the name of this function may
be a bit too ambitious.  How about calling it
check_rel_can_be_partition()?  The comment above the function could be a
much simpler sentence too.  I know I may be just bikeshedding here though.

+    /*
+     * If the relation is not a partition, return itself as a result.
+     */
+    if (!get_rel_relispartition(relid))
+        PG_RETURN_OID(relid);

Maybe the comment here could say "The relation itself may be the root parent".

+    /*
+     * If the relation is actually a partition, 'rootrelid' has been set to
+     * the OID of the root table in the partition tree.  It should be a valid
+     * valid per the previous check for partition leaf above.
+     */
+    Assert(OidIsValid(rootrelid));

"valid" is duplicated in the last sentence in the comment.  Anyway, what's
being Asserted can be described simply as:

/*
 * 'rootrelid' must contain a valid OID, given that the input relation is
 * a valid partition tree member as checked above.
 */

Thanks,
Amit