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