On Sun, Aug 16, 2026 at 1:47 PM Peter Smith <[email protected]> wrote:
>
> On Mon, Aug 3, 2026 at 8:15 PM shveta malik <[email protected]> wrote:
> >
> > On Mon, Aug 3, 2026 at 2:34 PM Álvaro Herrera <[email protected]> wrote:
> > >
> > > Hi,
> > >
> > > 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)));
> >
>
> PSA patch v7, which has the following changes:
>
> 1. The wrapped function get_partition_root_guts() is removed. It
> previously existed in v6 only to avoid a double validation when called
> from the SQL pg_partition_root() function. Now, we chose not to care
> about double validation because the inner validation is Assert code,
> so it only happens in DEBUG build anyhow.
>
> 3. The status validation function check_rel_can_be_partition() is now
> common code so that it can be called from all places.
>
> 3. Similar validation (checking if relid can be part of a partition
> tree) is now also done for the get_partition_ancestors() C function.
> (Note: Existing code calls this function with RELKIND_INDEX/
> RELKIND_PARTITIONED_INDEX, so the assertion is slightly different)
>
Thanks Peter.
1)
+ Assert(check_rel_can_be_partition(relid));
I am unsure whether this is the right approach for two reasons:
a) Generally, we avoid doing expensive operations such as catalog
lookups or taking locks in Assert() expressions. I tried to find
similar logic but could not find.
b) check_rel_can_be_partition() does not distinguish between the
relation not being a partition (or partitioned table) and the relation
no longer existing. get_rel_relkind() and get_rel_relispartition()
have the same limitation. One alternative would be to do this:
tp = SearchSysCache1(RELOID, ObjectIdGetDatum(relid));
if (HeapTupleIsValid(tp))
{
Form_pg_class reltup = (Form_pg_class) GETSTRUCT(tp);
/* Assert on relkind and relispartition here. */
}
else
/* ERROR/return/set a flag: to distinguish the two cases. */
Doing it this way gives us better control over how we handle different
scenarios.
Also, we can look at how StorePartitionBound() handles this, see the
logic under #ifdef USE_ASSERT_CHECKING. It is not fundamentally
different from Assert(check_rel_can_be_partition(relid)), but keeping
the assertion-related work in a separate block may be better for
maintainability.
2)
Another question is whether get_partition_root() should raise an error
when the partition is being detached and even_if_detached is false,
similar to get_partition_parent(), or whether it should simply return
InvalidOid, as it does currently.
~~
For both points, getting a third perspective would be helpful.
thanks
Shveta
