Hi Shveta. Thanks for your review!
On Tue, Jul 28, 2026 at 3:41 PM shveta malik <[email protected]> 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. ====== Kind Regards, Peter Smith. Fujitsu Australia
v2-0001-Add-C-function-get_partition_root.patch
Description: Binary data
