Re: A new C function `get_partition_root`.

On Wed, Jul 29, 2026 at 9:53 AM Peter Smith <smithpb2250(at)gmail(dot)com> wrote:
>
> Hi Shveta.
>
> Thanks for your review!
>
> On Tue, Jul 28, 2026 at 3:41 PM shveta malik <shveta(dot)malik(at)gmail(dot)com> wrote:
> >
> ...
> >
> > I agree that the patch you mentioned will benefit from such a
> > function. So before we commit that patch ( FOR TABLES IN SCHEMA
> > EXCEPT), this function addition can be considered. A few comments:
> >
> > 1)
> > + * Note: This should only be called when it is known that the relation is a
> > + * partition (see function get_partition_ancestors).
> >
> > The comment does not match the actual flow through
> > pg_partition_root(). This function can also be reached when the input
> > relation is not a partition but a partitioned table (the root itself).
> > We should update the comment to reflect the behavior enforced by
> > check_rel_can_be_partition() in pg_partition_root().
>
> OK. I've changed the function comment to say the specified relid may
> be a partitioned table.
>
> >
> > 2)
> > Looking only at get_partition_root(), it doesn't seem to be
> > self-contained in validating that the caller has supplied a valid
> > input.
> >
> > Since this internal function is expected to be used more widely (in
> > the thread you referenced), I would expect it to handle all input
> > cases consistently:
> >
> > --Return NULL if the input relation is neither a partition nor a
> > partitioned table. Or may be Assert will be better?
> > --Return the same relation if the input is already the partitioned
> > table (the root).
> > --Return the root partitioned table if the input is a partition.
> >
> > The second and third cases currently work as expected. But the first
> > and second cases are not distinguishable. Both a regular table and a
> > partitioned table results in the input relation's OID being returned
> > by get_partition_root (verified by pg_partition_root() by bypassing
> > check_rel_can_be_partition()). I think get_partition_root needs some
> > improvement for validaitng and distinguishing above cases.
> >
>
> You are right, there was some quirky behaviour for regular tables.
>
> The original code fragment of pg_get_partition_root (below)
> ------
> if (ancestors == NIL)
> PG_RETURN_OID(relid);
> ------
> only made sense because the relid had already been verified up-front
> to be a partition or partitioned table.
>
> I wanted my new function to be more like `get_partition_ancestors`,
> which has expectations on the caller, rather than adding a lot of
> up-front parameter validation like `pg_get_partition_root` has. OTOH,
> I agree with your point that a regular table should not be returned
> from this function, so I have added the necessary Assert to fix that.
>
> ~~~
>
> PSA patch v2.
>
> This still passes make check-world, and I verified different inputs
> manually using the same technique that you described.
>

Thanks for addressing comments Peter.

1)
CREATE TABLE p (id int);
CREATE TABLE c () INHERITS (p);

select pg_partition_root('c') retruns NULL, while if we experiment
with get_partition_root('c') alone, it returns 'p'.

The diffefrence in behaviour boils down to get_partition_ancestors()
returning valid ancestor list for 'c' (as you stated that this expects
caller to ensure input is a parition). So what do you suggest here?
More Assert in ' if (ancestors)' or leave it like this?

2)
- List *ancestors = get_partition_ancestors(relid);
+ Oid root_relid = get_partition_root(relid);
const char *attname = get_attname(relid, attnum, false);

- relid = llast_oid(ancestors);
+ relid = root_relid;

Should we get rid of root_relid and simply do:
relid = get_partition_root(relid);

thanks
Shveta