Re: Add pg_partition_root to get top-most parent of a partition tree

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