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

From: Amit Langote <Langote_Amit_f8(at)lab(dot)ntt(dot)co(dot)jp>
To: Michael Paquier <michael(at)paquier(dot)xyz>, Alvaro Herrera <alvherre(at)2ndquadrant(dot)com>
Cc: 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-14 05:20:27
Views: Raw Message | Whole Thread | Download mbox | Resend email
Lists: pgsql-hackers


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

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


In response to


Browse pgsql-hackers by date

  From Date Subject
Next Message Tom Lane 2018-12-14 05:23:17 Re: tab-completion debug print
Previous Message Tom Lane 2018-12-14 05:13:58 Re: Valgrind failures in Apply Launcher's bgworker_quickdie() exit