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

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.

I haven't actually tried to write a query that obtains a tree of
constraints using this, mind ...

--
�lvaro Herrera https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

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.

I haven't actually tried to write a query that obtains a tree of
constraints using this, mind ...

Sure. It would be good to agree on an interface. I have not tried
either, but you should be able to get away with a join on relid returned
by pg_partition_tree() with pg_constraint.conrelid with
pg_get_constraintdef() instead of a WITH RECURSIVE, no?
--
Michael

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,
--
Michael

Hi,

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

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

Thanks,
Amit

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

On Sat, Dec 15, 2018 at 10:16:08AM +0900, Michael Paquier wrote:

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

Rebased as per the attached, and moved to next CF.
--
Michael

Hi Michael,

Sorry about the long delay in replying. Looking at the latest patch (v4)
but replying to an earlier email of yours.

On 2018/12/15 10:16, Michael Paquier wrote:

On Fri, Dec 14, 2018 at 02:20:27PM +0900, Amit Langote wrote:

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

OK, no problem.

Some minor comments on v4:

+/*
+ * Perform several checks on a relation on which is extracted some
+ * information related to its partition tree.

This is a bit unclear to me. How about:

Checks if a given relation can be part of a partition tree

+/*
+ * pg_partition_root
+ *
+ * For the given relation part of a partition tree, return its top-most
+ * root parent.
+ */

How about writing:

Returns the top-most parent of the partition tree to which a given
relation belongs, or NULL if it's not (or cannot be) part of any partition
tree

Given that a couple (?) of other patches depend on this, maybe it'd be a
good idea to proceed with this. Sorry that I kept this hanging too long
by not sending these comments sooner.

Thanks,
Amit

On Wed, Feb 06, 2019 at 05:26:48PM +0900, Amit Langote wrote:

Some minor comments on v4:

Thanks for the review.

+/*
+ * Perform several checks on a relation on which is extracted some
+ * information related to its partition tree.

This is a bit unclear to me. How about:

Checks if a given relation can be part of a partition tree

Done as suggested.

Returns the top-most parent of the partition tree to which a given
relation belongs, or NULL if it's not (or cannot be) part of any partition
tree

Fine for me as well.

Given that a couple (?) of other patches depend on this, maybe it'd be a
good idea to proceed with this.

If you are happy with the version attached, I am fine to commit it. I
think that we have the right semantics and the right test coverage for
this patch.

Sorry that I kept this hanging too long by not sending these
comments sooner.

No problem, don't worry. There are many patches hanging around.
--
Michael

Hi,

On 2019/02/06 19:14, Michael Paquier wrote:

Given that a couple (?) of other patches depend on this, maybe it'd be a
good idea to proceed with this.

If you are happy with the version attached, I am fine to commit it. I
think that we have the right semantics and the right test coverage for
this patch.

Yeah, I agree. Should also check with Alvaro maybe?

Thanks,
Amit

On Thu, Feb 07, 2019 at 01:34:15PM +0900, Amit Langote wrote:

Yeah, I agree. Should also check with Alvaro maybe?

Good idea. Let's wait for his input.
--
Michael

On 2019-Feb-07, Michael Paquier wrote:

On Thu, Feb 07, 2019 at 01:34:15PM +0900, Amit Langote wrote:

Yeah, I agree. Should also check with Alvaro maybe?

Good idea. Let's wait for his input.

I looked at it briefly a few weeks ago and it seemed sound, though I
haven't yet tried to write the constraint display query for psql using
it, yet -- but that should be straightforward anyway. Let's get it
committed so we have one less thing to worry about.

--
�lvaro Herrera https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

On Thu, Feb 07, 2019 at 12:11:49PM -0300, Alvaro Herrera wrote:

I looked at it briefly a few weeks ago and it seemed sound, though I
haven't yet tried to write the constraint display query for psql using
it, yet -- but that should be straightforward anyway. Let's get it
committed so we have one less thing to worry about.

item_to_worry_about--;

Thanks for the successive reviews.
--
Michael