On Mon, Aug 3, 2026 at 3:55 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.
yes, thats right, no validation for normal builds.
> Surely that's the wrong thing?
Here we have tried to mimic the behaviour of
'get_partition_ancestors'. get_partition_ancestors() does not have
even basic Asserts in place and completely relies on user to verify
the input. So we were not sure whether we should use elog in
get_partition_root().
Also do you think get_partition_root() should give elog even when
'even_if_detached'=false and 'detach is pending' similar to what we do
in get_partition_parent (see [1]).
[1]:
get_partition_parent:
if (detach_pending && !even_if_detached)
elog(ERROR, "relation %u has no parent because it's
being detached",
relid);
> 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.
>
Okay.
~
Since we are on this topic, Peter and myself had discussed it
internally, whether get_partition_ancestors() should be enhanced to
have similar input validations and also should accept
'even_if_detached' input? This will help avoiding potential issues
caused by get_partition_ancestors() on HEAD. One of such issues is
discussed at [1]
[1]:
https://www.postgresql.org/message-id/CAJpy0uAdF3K0QDANkT2HgiMzPzOpHKiMXQsM8bZfaaaCKYfxtQ%40mail.gmail.com
thanks
Shveta
