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