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

On Fri, Dec 14, 2018 at 02:20:27PM +0900, Amit Langote wrote:
> 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

Check.

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

On this one I disagree, both pg_partition_root and pg_partition_tree
share the same semantics on the matter.  If the set of functions gets
expanded again later on, I got the feeling that we could forget about it
again, and at least placing the check here has the merit to make out
future selves not forget about that pattern..

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

The review is also here for that.  The routine name you are suggesting
looks good to me.

> +    /*
> +     * 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".

Check.  I tweaked the comment in this sense.

> "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.
>  */

Changed in this sense.  Please find attached an updated patch.
--
Michael