> On Aug 3, 2026, at 11:59, Peter Smith <[email protected]> wrote:
>
> 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.
>
Thanks for updating the patch.
Given the comment:
```
+ * Note: This should only be called when it is known that the relation is a
+ * partition or partitioned table.
```
Does it make sense to add an Assert for that, like:
```
Assert(get_rel_relispartition(relid) ||
RELKIND_HAS_PARTITIONS(get_rel_relkind(relid)));
```
Then, maybe we don’t need the final sanity check assert.
Otherwise v5 looks good to me. The new parameter even_if_detached matches the
existing get_partition_parent().
Best regards,
--
Chao Li (Evan)
HighGo Software Co., Ltd.
https://www.highgo.com/
