> On Mar 6, 2026, at 14:21, Xuneng Zhou <xunengzhou@gmail.com> wrote:
>
> Hi Yuanzhuo,
>
> Thanks for looking into it.
>
> On Fri, Mar 6, 2026 at 1:48 PM yangyz <1197620467@qq.com> wrote:
> I reviewed the patch you submitted and identified two issues.
>
> 1.In the Add pg_stat_recovery system view patch file, the documentation
> modification indicates that the lack of permissions only results in the inability
> to view a few specific columns. But the implementation of the code is:
>
> if (! has_privs_of_role(GetUserId(), ROLE_PG_READ_ALL_STATS))
> PG_RETURN_NULL(); If there is no permission, return an empty line. This is inconsistent with the written
document.
>
> Yeah, this is a mismatch in the patch v3. However, it's been removed by Michael in the commit. So it should be fine
inHEAD.
> 2.In the function pg_stat_get_recovery(), the two arrays "Datum *values" and "bool *nulls" consist of a fixed set of
sevenelements, so there is no need for dynamic allocation.
>
> For a single-row function with ~8–10 columns, saving one palloc is a micro-optimization, not a major performance
issue.I am not that sure of the benefit it brings, still preparing a small patch to turn the palloc into a fixed stack
array.
> --
> Best,
> Xuneng
Hi Xuneng,
I was reviewing the patch. Obviously, Michael was lightning fast and has already pushed 0001. I have one small
additionalcomment on pushed 0001.
```
if (get_call_result_type(fcinfo, NULL, &tupdesc) != TYPEFUNC_COMPOSITE)
elog(ERROR, "return type must be a row type");
```
This uses elog(ERROR), while the other functions in the same file use ereport(ERROR). I think ereport is generally
preferrednowadays over elog. This is a minor point, so only fix it if you have a chance.
I will send my comments on the remaining two patches separately.
Best regards,
--
Chao Li (Evan)
HighGo Software Co., Ltd.
https://www.highgo.com/
