On Mon, Aug 3, 2026 at 8:25 PM Álvaro Herrera <[email protected]> wrote: > > On 2026-Aug-03, shveta malik wrote: > > > On Mon, Aug 3, 2026 at 2:34 PM Álvaro Herrera <[email protected]> wrote: > > > > I'd say this looks okay, but why do you need get_partition_root_guts() > > > exposed in partition.h? In fact, it's not clear to me why you need a > > > second routine at all. Why isn't enough to have just > > > get_partition_root()? > > > > I think to avoid performing the validation twice in > > pg_partition_root(): first via check_rel_can_be_partition(), and then > > again in get_partition_root (see [1]), get_partition_root_guts() is > > introduced and exposed in partition.h. > > > > [1]: > > + /* Validate relid is member of a partition tree */ > > + Assert(get_rel_relispartition(relid) || > > + RELKIND_HAS_PARTITIONS(get_rel_relkind(relid))); > > This seems wrong actually (having this as an assert rather than > if/elog), because it means no validation at all occur on normal builds. > Surely that's the wrong thing? > > Redundant asserts are no cause for concern IMO. I would certainly not > create two routines just to avoid an assert, which is nothing at all in > production builds. >
Hi Alvaro. Thankyou for your review cvomments. I'd like to implement this whichever way you think is best, but I wasn't sure what had been decided for some of the questions. e.g. Q1. Keep Asserts for validation, or change to if/elog? - As Shveta mentioned, the get_partition_ancestors also has no validation for normal builds Q2. Keep or remove the wrapper layer for get_partition_root_guts? - As Shveta mentioned, the purpose of the wrapper was to avoid doubling the validation when called from SQL pg_partition_root() - If keeping Aserts, there would be zero overheads in a normal build, so removing the wrapper is fine - If get_partition_root uses if/elog validation, I'm not so sure ====== Kind Regards, Peter Smith. Fujitsu Australia
