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

From: Michael Paquier <michael(at)paquier(dot)xyz>
To: Amit Langote <Langote_Amit_f8(at)lab(dot)ntt(dot)co(dot)jp>
Cc: Alvaro Herrera <alvherre(at)2ndquadrant(dot)com>, Postgres hackers <pgsql-hackers(at)postgresql(dot)org>
Subject: Re: Add pg_partition_root to get top-most parent of a partition tree
Date: 2018-12-15 01:16:08
Message-ID: 20181215011608.GE5012@paquier.xyz
Views: Raw Message | Whole Thread | Download mbox
Thread:
Lists: pgsql-hackers

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

Attachment Content-Type Size
partition-root-v3.patch text/x-diff 9.8 KB

In response to

Responses

Browse pgsql-hackers by date

  From Date Subject
Next Message Peter Geoghegan 2018-12-15 01:50:56 Re: Computing the conflict xid for index page-level-vacuum on primary
Previous Message Andres Freund 2018-12-15 00:53:03 Re: automatically assigning catalog toast oids