|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|
|Views:||Raw Message | Whole Thread | Download mbox | Resend email|
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
+ * 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.
+ 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))
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.
"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.
|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|