Re: Add pg_stat_recovery system view

> On Mar 6, 2026, at 14:21, Xuneng Zhou <xunengzhou(at)gmail(dot)com> wrote:
>
> Hi Yuanzhuo,
>
> Thanks for looking into it.
>
> On Fri, Mar 6, 2026 at 1:48 PM yangyz <1197620467(at)qq(dot)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 in HEAD.
> 2.In the function pg_stat_get_recovery(), the two arrays "Datum *values" and "bool *nulls" consist of a fixed set of seven elements, 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 additional comment 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 preferred nowadays 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/