Thread: Add pg_partition_root to get top-most parent of a partition tree
Hi all, Álvaro has given faced a use case where it would be useful to have a function which is able to return the top-most parent of a partition tree: https://postgr.es/m/20181204184159.eue3wlchqrkh4vsc@alvherre.pgsql This has been mentioned as well on the thread where was discussed pg_partition_tree, but it got shaved from the committed patch as many things happened when discussing the thing. Attached is a patch to do the work, which includes documentation and tests. An argument could be made to include the top-most parent as part of pg_partition_tree, but it feels more natural to me to have a separate function. This makes sure to handle invalid relations by returning NULL, and it generates an error for incorrect relkind. I have included as well a fix for the recent crash on pg_partition_tree I have reported, but let's discuss the crash on its thread here: https://www.postgresql.org/message-id/20181207010406.GO2407@paquier.xyz The bug fix would most likely get committed first, and I'll rebase this patch as need be. I am adding this patch to the CF of January. I think that Amit should also be marked as a co-author of this patch, as that's inspired from what has been submitted previously, still I have no reused the code. Thanks, -- Michael
Attachment
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
Attachment
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
Attachment
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
Attachment
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
Attachment
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
Attachment
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
Attachment
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