Re: A new C function `get_partition_root`.

On Thu, Jul 30, 2026 at 3:46 PM Chao Li <li(dot)evan(dot)chao(at)gmail(dot)com> wrote:
>
>
>
> > On Jul 30, 2026, at 08:04, Peter Smith <smithpb2250(at)gmail(dot)com> wrote:
> >
> > On Wed, Jul 29, 2026 at 7:18 PM shveta malik <shveta(dot)malik(at)gmail(dot)com> wrote:
> >>
> >> On Wed, Jul 29, 2026 at 2:28 PM Peter Smith <smithpb2250(at)gmail(dot)com> wrote:
> > ...
> >>> BTW, although I think using sanity Assert was correct, just in case
> >>> there is some unanticipated way to reach the C function with a bad
> >>> relid, I've changed to use an errlog(ERROR).
> >>> Thoughts?
> >>
> >> I don't immediately see any such possibility. I feel Assert is better.
> >>
> >
> > OK. PSA v4, which is the same as v3, but uses Assert instead of elog.
> >
> > ======
> > Kind Regards,
> > Peter Smith.
> > Fujitsu Australia
> > <v4-0001-Add-C-function-get_partition_root.patch>
>
>
> I just reviewed v4 and got a doubt:
> ```
> + /* Sanity check: The root must be a partitioned table */
> + Assert(RELKIND_HAS_PARTITIONS(get_rel_relkind(root_relid)));
> ```
>
> Looking into get_partition_ancestors(), it returns NIL in two cases:
>
> 1) No more parent
> 2) detach_pending is true
>
> Case 1 is an expected case, I doubt case 2 may fire the Assert.
>
> Say, partitioned table p has a leaf partition p1, now p1 is being detached. get_partition_ancestors(p1) may return NIL because of detach_pending, so root_relid is set to p1, but p1 is not a partitioned table, then this Assert is fired.
>

Hi Chao-San.

Thanks for reporting that issue.

I've dealt with that now by exposing the `detach_pending` so now the
`get_partition_root` can know whether a detach was the cause of
ancestors == NIL.

I also added another flag `even_if_detached` to `get_partition_root`
so callers can decide what to do if a detach is in progress. That's
analogous to other code that has a similar parameter.

It seems ok using Shveta's 3 sessions example for testing.

~~~

I was wondering if I should have taken this further and similarly
changed the `get_partition_ancestors` signature to optionally expose a
detach_pending flag.
That could be used to protect some of the existing code that AFAICT
has potential to crash:
ancestors = get_partition_ancestors(relid);
last_ancestor_relid = llast_oid(ancestors);

But, that would be a bit more invasive so I wanted some 2nd opinions
before going too far.

Thoughts?

======
Kind Regards,
Peter Smith
Fujitsu Australia