On Thu, Jul 30, 2026 at 3:46 PM Chao Li <[email protected]> wrote: > > > > > On Jul 30, 2026, at 08:04, Peter Smith <[email protected]> wrote: > > > > On Wed, Jul 29, 2026 at 7:18 PM shveta malik <[email protected]> wrote: > >> > >> On Wed, Jul 29, 2026 at 2:28 PM Peter Smith <[email protected]> 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
v5-0001-Add-C-function-get_partition_root.patch
Description: Binary data
