Horton

Thread

Commits

  1. Fix order of columns in pg_stat_recovery

  2. Add system view pg_stat_recovery

  3. Refactor code retrieving string for RecoveryPauseState

  4. Move definition of XLogRecoveryCtlData to xlogrecovery.h

  1. Add pg_stat_recovery system view

    Xuneng Zhou <xunengzhou@gmail.com> — 2026-01-27T07:23:50Z

    Hi Hackers,

This patch series introduces pg_stat_recovery, a new view that exposes
the startup process’s internal recovery state at the SQL level. It
follows the pattern of pg_stat_wal_receiver, but focuses on WAL replay
and recovery rather than WAL reception.

The view provides visibility into replay progress, recovery timing,
and operational status—information that was previously scattered
across separate function calls or not exposed at all.

As Michael suggested [1][2], this view is intentionally independent of
walreceiver state, since the startup process can consume WAL from
multiple sources (archive, pg_wal, or streaming), not just streaming
replication.

The patch series:
0001: Refactor: move XLogRecoveryCtlData struct to xlogrecovery.h
Move the XLogRecoveryCtlData struct definition from xlogrecovery.c to
xlogrecovery.h,

0002: Add pg_stat_recovery system view
Introduces the core view with columns

0003: Refactor: move XLogSource enum to xlogrecovery.h
Preparatory refactoring to make XLogSource visible externally.

0004: Add wal_source column to pg_stat_recovery
Adds wal_source column showing where WAL was last read from:
'archive', 'pg_wal', or 'stream'.

Example usage on a standby:

SELECT promote_triggered, pause_state, wal_source,
pg_size_pretty(pg_wal_lsn_diff(replay_end_lsn, last_replayed_end_lsn))
FROM pg_stat_recovery;

promote_triggered | pause_state | wal_source | pg_size_pretty
-------------------+-------------+------------+----------------
f | not paused | stream | 0 bytes

Feedbacks welcome.

[1] https://www.postgresql.org/message-id/aW13GJn_RfTJIFCa@paquier.xyz
[2] https://www.postgresql.org/message-id/aW68b79-9U3WPZiz@paquier.xyz

-- 
Best,
Xuneng

  2. Re: Add pg_stat_recovery system view

    Xuneng Zhou <xunengzhou@gmail.com> — 2026-03-04T01:02:29Z

    Hi,

On Tue, Jan 27, 2026 at 3:23 PM Xuneng Zhou <xunengzhou@gmail.com> wrote:
>
> Hi Hackers,
>
> This patch series introduces pg_stat_recovery, a new view that exposes
> the startup process’s internal recovery state at the SQL level. It
> follows the pattern of pg_stat_wal_receiver, but focuses on WAL replay
> and recovery rather than WAL reception.
>
> The view provides visibility into replay progress, recovery timing,
> and operational status—information that was previously scattered
> across separate function calls or not exposed at all.
>
> As Michael suggested [1][2], this view is intentionally independent of
> walreceiver state, since the startup process can consume WAL from
> multiple sources (archive, pg_wal, or streaming), not just streaming
> replication.
>
> The patch series:
> 0001: Refactor: move XLogRecoveryCtlData struct to xlogrecovery.h
> Move the XLogRecoveryCtlData struct definition from xlogrecovery.c to
> xlogrecovery.h,
>
> 0002: Add pg_stat_recovery system view
> Introduces the core view with columns
>
> 0003: Refactor: move XLogSource enum to xlogrecovery.h
> Preparatory refactoring to make XLogSource visible externally.
>
> 0004: Add wal_source column to pg_stat_recovery
> Adds wal_source column showing where WAL was last read from:
> 'archive', 'pg_wal', or 'stream'.
>
> Example usage on a standby:
>
> SELECT promote_triggered, pause_state, wal_source,
> pg_size_pretty(pg_wal_lsn_diff(replay_end_lsn, last_replayed_end_lsn))
> FROM pg_stat_recovery;
>
> promote_triggered | pause_state | wal_source | pg_size_pretty
> -------------------+-------------+------------+----------------
> f | not paused | stream | 0 bytes
>
> Feedbacks welcome.
>
> [1] https://www.postgresql.org/message-id/aW13GJn_RfTJIFCa@paquier.xyz
> [2] https://www.postgresql.org/message-id/aW68b79-9U3WPZiz@paquier.xyz
>
> --
> Best,
> Xuneng

Just rebase.


--
Best,
Xuneng

  3. Re: Add pg_stat_recovery system view

    Michael Paquier <michael@paquier.xyz> — 2026-03-05T03:58:16Z

    On Wed, Mar 04, 2026 at 09:02:29AM +0800, Xuneng Zhou wrote:
> Just rebase.

I have applied 0001, that simply moves some code around.

Regarding 0002, I would recommend to not bump the catalog version in
catversion.h when sending a patch to the lists.  This task is left to
committers when the code gets merged into the tree.  And this is
annoying for submitters because it can create a lot of conflicts.

Using a set-returning function is I think wrong here, betraying the
representation of the recovery status as stored in the system.  We
know that there is only one state of recovery, fixed in shared memory.
Like the cousins of this new function, let's make thinks non-SRF,
returning one row all the time with PG_RETURN_NULL() if the conditions
for information display are not satisfied.  When we are not in
recovery or when the role querying the function is not granted
ROLE_PG_READ_ALL_STATS, that will simplify the patch as there is no
need for the else branch with the nulls, as written in your patch.

The field values are acquired the right way, spinlock acquisitions
have to be short.

Like pg_stat_wal_receiver, let's add to the view definition a qual to
return a row only if the fields are not NULL.

pg_get_wal_replay_pause_state() displays the pause_state, and it is
not hidden behind the stats read role.  I am not really convinced that
this is worth bothering to treat as an exception in this patch.  It
makes it a bit more complicated, for little gain.  I would just hide
all the fields behind the role granted, to keep the code simpler, even
if that's slightly inconsistent with pg_get_wal_replay_pause_state().

After writing this last point, as far as I can see there is a small
optimization possible in the patch.  When a role is not granted
ROLE_PG_READ_ALL_STATS, we know that we will not return any
information so we could skip the spinlock acquisition and avoid
spinlock acquisitions when one queries the function but has no access
to its data.

+       True if a promotion has been triggered for this standby server.

Standby servers are implied if data is returned, this sentence can be
simpler and cut the "standby server" part.

+       Start LSN of the last WAL record replayed during recovery. 
[...]
+       End LSN of the last WAL record replayed during recovery.
[...]
+       Timeline of the last replayed WAL record. 
For other system views with LSN information, we don't use "LSN", but
"write-ahead log location".  I'd suggest the same term here.  These
three fields refer to the last record *successfully* replayed.  It
seems important to mention this fact, I guess?

+       <structfield>replay_end_lsn</structfield> <type>pg_lsn</type>
+      </para>
+      <para>
+       Current replay position. When replaying a record, this is the end
+       position of the record being replayed; otherwise it equals
+       <structfield>last_replayed_end_lsn</structfield>.

Slightly inexact.  This is the end LSN + 1.

+       <structfield>replay_end_lsn</structfield> <type>pg_lsn</type>
[..]
+       <structfield>replay_end_tli</structfield> <type>integer</type> 

Okay, I can fall behind the addition of these two, it could be helpful
in debugging something like a locking issue when replaying a specific
record, I guess.  At least I'd want to know what is happening for a
record currently being replayed.  It seems to me that this could be
more precise, mentioning that these refer to a record *currently*
being replayed.

Is recoveryLastXTime actually that relevant to have?  We use it for
logging and for recovery target times, which is something less
relevant than the other fields, especially if we think about standbys
where these have no targets to reach.

currentChunkStartTime, on the contrary, is much more relevant to me,
due to the fact that we use it in WaitForWALToBecomeAvailable() with
active WAL receivers running.
--
Michael

  4. Re: Add pg_stat_recovery system view

    Xuneng Zhou <xunengzhou@gmail.com> — 2026-03-05T07:37:10Z

    Hi Michael,

Thanks for the detailed feedback!

On Thu, Mar 5, 2026 at 11:58 AM Michael Paquier <michael@paquier.xyz> wrote:
>
> On Wed, Mar 04, 2026 at 09:02:29AM +0800, Xuneng Zhou wrote:
> > Just rebase.
>
> I have applied 0001, that simply moves some code around.
>

Thanks for applying.

> Regarding 0002, I would recommend to not bump the catalog version in
> catversion.h when sending a patch to the lists.  This task is left to
> committers when the code gets merged into the tree.  And this is
> annoying for submitters because it can create a lot of conflicts.

I'll leave it untouched.

> Using a set-returning function is I think wrong here, betraying the
> representation of the recovery status as stored in the system.  We
> know that there is only one state of recovery, fixed in shared memory.
> Like the cousins of this new function, let's make thinks non-SRF,
> returning one row all the time with PG_RETURN_NULL() if the conditions
> for information display are not satisfied.  When we are not in
> recovery or when the role querying the function is not granted
> ROLE_PG_READ_ALL_STATS, that will simplify the patch as there is no
> need for the else branch with the nulls, as written in your patch.
> The field values are acquired the right way, spinlock acquisitions
> have to be short.

My earlier thought for keeping pg_stat_get_recovery as an SRF is to
make pg_stat_recovery simpler by avoiding an extra filter such as
WHERE s.promote_triggered IS NOT NULL to preserve 0/1-row semantics.
pg_stat_get_recovery_prefetch also uses the SRF pattern.

> Like pg_stat_wal_receiver, let's add to the view definition a qual to
> return a row only if the fields are not NULL.
>
> pg_get_wal_replay_pause_state() displays the pause_state, and it is
> not hidden behind the stats read role.  I am not really convinced that
> this is worth bothering to treat as an exception in this patch.  It
> makes it a bit more complicated, for little gain.  I would just hide
> all the fields behind the role granted, to keep the code simpler, even
> if that's slightly inconsistent with pg_get_wal_replay_pause_state().

I agree that exposing a subset of columns unconditionally is not worth
the added complexity.

> After writing this last point, as far as I can see there is a small
> optimization possible in the patch.  When a role is not granted
> ROLE_PG_READ_ALL_STATS, we know that we will not return any
> information so we could skip the spinlock acquisition and avoid
> spinlock acquisitions when one queries the function but has no access
> to its data.

Makes sense to me. I'll apply it as suggested.

> +       True if a promotion has been triggered for this standby server.
>
> Standby servers are implied if data is returned, this sentence can be
> simpler and cut the "standby server" part.

+ 1

> +       Start LSN of the last WAL record replayed during recovery.
> [...]
> +       End LSN of the last WAL record replayed during recovery.
> [...]
> +       Timeline of the last replayed WAL record.
> For other system views with LSN information, we don't use "LSN", but
> "write-ahead log location".  I'd suggest the same term here.  These
> three fields refer to the last record *successfully* replayed.  It
> seems important to mention this fact, I guess?

I'll replace them.

> +       <structfield>replay_end_lsn</structfield> <type>pg_lsn</type>
> +      </para>
> +      <para>
> +       Current replay position. When replaying a record, this is the end
> +       position of the record being replayed; otherwise it equals
> +       <structfield>last_replayed_end_lsn</structfield>.
>
> Slightly inexact.  This is the end LSN + 1.

Yeh, this needs to be corrected.

> +       <structfield>replay_end_lsn</structfield> <type>pg_lsn</type>
> [..]
> +       <structfield>replay_end_tli</structfield> <type>integer</type>
>
> Okay, I can fall behind the addition of these two, it could be helpful
> in debugging something like a locking issue when replaying a specific
> record, I guess.  At least I'd want to know what is happening for a
> record currently being replayed.  It seems to me that this could be
> more precise, mentioning that these refer to a record *currently*
> being replayed.

I will adjust the docs to say these describe the record currently
being replayed, with replay_end_lsn being the end position + 1.

> Is recoveryLastXTime actually that relevant to have?  We use it for
> logging and for recovery target times, which is something less
> relevant than the other fields, especially if we think about standbys
> where these have no targets to reach.

I agree it is less central for standby monitoring (and partly overlaps
with pg_last_xact_replay_timestamp()), so I’ll remove it from this
view in the next revision.

> currentChunkStartTime, on the contrary, is much more relevant to me,
> due to the fact that we use it in WaitForWALToBecomeAvailable() with
> active WAL receivers running.

Yeah, it could be useful for apply-delay/catch-up diagnostics.

-- 
Best,
Xuneng

  5. Re: Add pg_stat_recovery system view

    Xuneng Zhou <xunengzhou@gmail.com> — 2026-03-05T13:01:35Z

    Hi,

On Thu, Mar 5, 2026 at 3:37 PM Xuneng Zhou <xunengzhou@gmail.com> wrote:
>
> Hi Michael,
>
> Thanks for the detailed feedback!
>
> On Thu, Mar 5, 2026 at 11:58 AM Michael Paquier <michael@paquier.xyz> wrote:
> >
> > On Wed, Mar 04, 2026 at 09:02:29AM +0800, Xuneng Zhou wrote:
> > > Just rebase.
> >
> > I have applied 0001, that simply moves some code around.
> >
>
> Thanks for applying.
>
> > Regarding 0002, I would recommend to not bump the catalog version in
> > catversion.h when sending a patch to the lists.  This task is left to
> > committers when the code gets merged into the tree.  And this is
> > annoying for submitters because it can create a lot of conflicts.
>
> I'll leave it untouched.
>
> > Using a set-returning function is I think wrong here, betraying the
> > representation of the recovery status as stored in the system.  We
> > know that there is only one state of recovery, fixed in shared memory.
> > Like the cousins of this new function, let's make thinks non-SRF,
> > returning one row all the time with PG_RETURN_NULL() if the conditions
> > for information display are not satisfied.  When we are not in
> > recovery or when the role querying the function is not granted
> > ROLE_PG_READ_ALL_STATS, that will simplify the patch as there is no
> > need for the else branch with the nulls, as written in your patch.
> > The field values are acquired the right way, spinlock acquisitions
> > have to be short.
>
> My earlier thought for keeping pg_stat_get_recovery as an SRF is to
> make pg_stat_recovery simpler by avoiding an extra filter such as
> WHERE s.promote_triggered IS NOT NULL to preserve 0/1-row semantics.
> pg_stat_get_recovery_prefetch also uses the SRF pattern.
>
> > Like pg_stat_wal_receiver, let's add to the view definition a qual to
> > return a row only if the fields are not NULL.
> >
> > pg_get_wal_replay_pause_state() displays the pause_state, and it is
> > not hidden behind the stats read role.  I am not really convinced that
> > this is worth bothering to treat as an exception in this patch.  It
> > makes it a bit more complicated, for little gain.  I would just hide
> > all the fields behind the role granted, to keep the code simpler, even
> > if that's slightly inconsistent with pg_get_wal_replay_pause_state().
>
> I agree that exposing a subset of columns unconditionally is not worth
> the added complexity.
>
> > After writing this last point, as far as I can see there is a small
> > optimization possible in the patch.  When a role is not granted
> > ROLE_PG_READ_ALL_STATS, we know that we will not return any
> > information so we could skip the spinlock acquisition and avoid
> > spinlock acquisitions when one queries the function but has no access
> > to its data.
>
> Makes sense to me. I'll apply it as suggested.
>
> > +       True if a promotion has been triggered for this standby server.
> >
> > Standby servers are implied if data is returned, this sentence can be
> > simpler and cut the "standby server" part.
>
> + 1
>
> > +       Start LSN of the last WAL record replayed during recovery.
> > [...]
> > +       End LSN of the last WAL record replayed during recovery.
> > [...]
> > +       Timeline of the last replayed WAL record.
> > For other system views with LSN information, we don't use "LSN", but
> > "write-ahead log location".  I'd suggest the same term here.  These
> > three fields refer to the last record *successfully* replayed.  It
> > seems important to mention this fact, I guess?
>
> I'll replace them.
>
> > +       <structfield>replay_end_lsn</structfield> <type>pg_lsn</type>
> > +      </para>
> > +      <para>
> > +       Current replay position. When replaying a record, this is the end
> > +       position of the record being replayed; otherwise it equals
> > +       <structfield>last_replayed_end_lsn</structfield>.
> >
> > Slightly inexact.  This is the end LSN + 1.
>
> Yeh, this needs to be corrected.
>
> > +       <structfield>replay_end_lsn</structfield> <type>pg_lsn</type>
> > [..]
> > +       <structfield>replay_end_tli</structfield> <type>integer</type>
> >
> > Okay, I can fall behind the addition of these two, it could be helpful
> > in debugging something like a locking issue when replaying a specific
> > record, I guess.  At least I'd want to know what is happening for a
> > record currently being replayed.  It seems to me that this could be
> > more precise, mentioning that these refer to a record *currently*
> > being replayed.
>
> I will adjust the docs to say these describe the record currently
> being replayed, with replay_end_lsn being the end position + 1.
>
> > Is recoveryLastXTime actually that relevant to have?  We use it for
> > logging and for recovery target times, which is something less
> > relevant than the other fields, especially if we think about standbys
> > where these have no targets to reach.
>
> I agree it is less central for standby monitoring (and partly overlaps
> with pg_last_xact_replay_timestamp()), so I’ll remove it from this
> view in the next revision.
>
> > currentChunkStartTime, on the contrary, is much more relevant to me,
> > due to the fact that we use it in WaitForWALToBecomeAvailable() with
> > active WAL receivers running.
>
> Yeah, it could be useful for apply-delay/catch-up diagnostics.
>

Here is the updated patch set. Please take a look.


--
Best,
Xuneng

  6. Re: Add pg_stat_recovery system view

    Michael Paquier <michael@paquier.xyz> — 2026-03-06T04:00:25Z

    On Thu, Mar 05, 2026 at 09:01:35PM +0800, Xuneng Zhou wrote:
> Here is the updated patch set. Please take a look.

Thanks.  It was mostly fine, still:
- The extra mention of ROLE_PG_READ_ALL_STATS was not necessary.  We
already have one at the top of the section.
- After sleeping on it, I have decided to keep recoveryLastXTime,
partially because we have a system function to retrieve it already.
- There was a refactoring piece possible when we switch a
RecoveryPauseState to a non-translatable string, which I have applied
separately.
- Added one test with pg_stat_recovery checked on a standby.
--
Michael

  7. Re: Add pg_stat_recovery system view

    Shinya Kato <shinya11.kato@gmail.com> — 2026-03-06T05:19:01Z

    Reading the committed patch, it appears that the values for
current_chunk_start_time and recovery_last_xact_time have been
swapped. Could you please look into this?

I have attached the patch (with redundant comments removed).

-- 
Best regards,
Shinya Kato
NTT OSS Center

  8. Re: Add pg_stat_recovery system view

    Michael Paquier <michael@paquier.xyz> — 2026-03-06T05:28:20Z

    On Fri, Mar 06, 2026 at 02:19:01PM +0900, Shinya Kato wrote:
> Reading the committed patch, it appears that the values for
> current_chunk_start_time and recovery_last_xact_time have been
> swapped. Could you please look into this?
> 
> I have attached the patch (with redundant comments removed).

Oops, indeed.  Will fix shortly.
--
Michael

  9. Re: Add pg_stat_recovery system view

    yangyz <1197620467@qq.com> — 2026-03-06T05:48:42Z

    I reviewed the patch you submitted and identified two issues.


1.In the Add pg_stat_recovery system view patch file, the documentation&nbsp;
modification indicates that the lack of permissions only results in the inability&nbsp;
to view a few specific columns. But the implementation of the code is:


&nbsp;if (! has_privs_of_role(GetUserId(), ROLE_PG_READ_ALL_STATS))
    PG_RETURN_NULL();&nbsp;


If there is no permission, return an empty line. This is inconsistent with the written document.
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.



Regards,

Yuanzhuo






         原始邮件
         
       
发件人：Xuneng Zhou <xunengzhou@gmail.com&gt;
发件时间：2026年3月5日 21:01
收件人：Michael Paquier <michael@paquier.xyz&gt;
抄送：pgsql-hackers <pgsql-hackers@lists.postgresql.org&gt;
主题：Re: Add pg_stat_recovery system view



       Hi,

On&nbsp;Thu,&nbsp;Mar&nbsp;5,&nbsp;2026&nbsp;at&nbsp;3:37 PM&nbsp;Xuneng&nbsp;Zhou&nbsp;<xunengzhou@gmail.com&gt;&nbsp;wrote:
&gt;
&gt;&nbsp;Hi&nbsp;Michael,
&gt;
&gt;&nbsp;Thanks&nbsp;for&nbsp;the&nbsp;detailed&nbsp;feedback!
&gt;
&gt;&nbsp;On&nbsp;Thu,&nbsp;Mar&nbsp;5,&nbsp;2026&nbsp;at&nbsp;11:58 AM&nbsp;Michael&nbsp;Paquier&nbsp;<michael@paquier.xyz&gt;&nbsp;wrote:
&gt;&nbsp;&gt;
&gt;&nbsp;&gt;&nbsp;On&nbsp;Wed,&nbsp;Mar&nbsp;04,&nbsp;2026&nbsp;at&nbsp;09:02:29AM&nbsp;+0800,&nbsp;Xuneng&nbsp;Zhou&nbsp;wrote:
&gt;&nbsp;&gt;&nbsp;&gt;&nbsp;Just&nbsp;rebase.
&gt;&nbsp;&gt;
&gt;&nbsp;&gt;&nbsp;I&nbsp;have&nbsp;applied&nbsp;0001,&nbsp;that&nbsp;simply&nbsp;moves&nbsp;some&nbsp;code&nbsp;around.
&gt;&nbsp;&gt;
&gt;
&gt;&nbsp;Thanks&nbsp;for&nbsp;applying.
&gt;
&gt;&nbsp;&gt;&nbsp;Regarding&nbsp;0002,&nbsp;I&nbsp;would&nbsp;recommend&nbsp;to&nbsp;not&nbsp;bump&nbsp;the&nbsp;catalog&nbsp;version&nbsp;in
&gt;&nbsp;&gt;&nbsp;catversion.h&nbsp;when&nbsp;sending&nbsp;a&nbsp;patch&nbsp;to&nbsp;the&nbsp;lists.&nbsp;&nbsp;This&nbsp;task&nbsp;is&nbsp;left&nbsp;to
&gt;&nbsp;&gt;&nbsp;committers&nbsp;when&nbsp;the&nbsp;code&nbsp;gets&nbsp;merged&nbsp;into&nbsp;the&nbsp;tree.&nbsp;&nbsp;And&nbsp;this&nbsp;is
&gt;&nbsp;&gt;&nbsp;annoying&nbsp;for&nbsp;submitters&nbsp;because&nbsp;it&nbsp;can&nbsp;create&nbsp;a&nbsp;lot&nbsp;of&nbsp;conflicts.
&gt;
&gt;&nbsp;I'll&nbsp;leave&nbsp;it&nbsp;untouched.
&gt;
&gt;&nbsp;&gt;&nbsp;Using&nbsp;a&nbsp;set-returning&nbsp;function&nbsp;is&nbsp;I&nbsp;think&nbsp;wrong&nbsp;here,&nbsp;betraying&nbsp;the
&gt;&nbsp;&gt;&nbsp;representation&nbsp;of&nbsp;the&nbsp;recovery&nbsp;status&nbsp;as&nbsp;stored&nbsp;in&nbsp;the&nbsp;system.&nbsp;&nbsp;We
&gt;&nbsp;&gt;&nbsp;know&nbsp;that&nbsp;there&nbsp;is&nbsp;only&nbsp;one&nbsp;state&nbsp;of&nbsp;recovery,&nbsp;fixed&nbsp;in&nbsp;shared&nbsp;memory.
&gt;&nbsp;&gt;&nbsp;Like&nbsp;the&nbsp;cousins&nbsp;of&nbsp;this&nbsp;new&nbsp;function,&nbsp;let's&nbsp;make&nbsp;thinks&nbsp;non-SRF,
&gt;&nbsp;&gt;&nbsp;returning&nbsp;one&nbsp;row&nbsp;all&nbsp;the&nbsp;time&nbsp;with&nbsp;PG_RETURN_NULL()&nbsp;if&nbsp;the&nbsp;conditions
&gt;&nbsp;&gt;&nbsp;for&nbsp;information&nbsp;display&nbsp;are&nbsp;not&nbsp;satisfied.&nbsp;&nbsp;When&nbsp;we&nbsp;are&nbsp;not&nbsp;in
&gt;&nbsp;&gt;&nbsp;recovery&nbsp;or&nbsp;when&nbsp;the&nbsp;role&nbsp;querying&nbsp;the&nbsp;function&nbsp;is&nbsp;not&nbsp;granted
&gt;&nbsp;&gt;&nbsp;ROLE_PG_READ_ALL_STATS,&nbsp;that&nbsp;will&nbsp;simplify&nbsp;the&nbsp;patch&nbsp;as&nbsp;there&nbsp;is&nbsp;no
&gt;&nbsp;&gt;&nbsp;need&nbsp;for&nbsp;the&nbsp;else&nbsp;branch&nbsp;with&nbsp;the&nbsp;nulls,&nbsp;as&nbsp;written&nbsp;in&nbsp;your&nbsp;patch.
&gt;&nbsp;&gt;&nbsp;The&nbsp;field&nbsp;values&nbsp;are&nbsp;acquired&nbsp;the&nbsp;right&nbsp;way,&nbsp;spinlock&nbsp;acquisitions
&gt;&nbsp;&gt;&nbsp;have&nbsp;to&nbsp;be&nbsp;short.
&gt;
&gt;&nbsp;My&nbsp;earlier&nbsp;thought&nbsp;for&nbsp;keeping&nbsp;pg_stat_get_recovery&nbsp;as&nbsp;an&nbsp;SRF&nbsp;is&nbsp;to
&gt;&nbsp;make&nbsp;pg_stat_recovery&nbsp;simpler&nbsp;by&nbsp;avoiding&nbsp;an&nbsp;extra&nbsp;filter&nbsp;such&nbsp;as
&gt;&nbsp;WHERE&nbsp;s.promote_triggered&nbsp;IS&nbsp;NOT&nbsp;NULL&nbsp;to&nbsp;preserve&nbsp;0/1-row&nbsp;semantics.
&gt;&nbsp;pg_stat_get_recovery_prefetch&nbsp;also&nbsp;uses&nbsp;the&nbsp;SRF&nbsp;pattern.
&gt;
&gt;&nbsp;&gt;&nbsp;Like&nbsp;pg_stat_wal_receiver,&nbsp;let's&nbsp;add&nbsp;to&nbsp;the&nbsp;view&nbsp;definition&nbsp;a&nbsp;qual&nbsp;to
&gt;&nbsp;&gt;&nbsp;return&nbsp;a&nbsp;row&nbsp;only&nbsp;if&nbsp;the&nbsp;fields&nbsp;are&nbsp;not&nbsp;NULL.
&gt;&nbsp;&gt;
&gt;&nbsp;&gt;&nbsp;pg_get_wal_replay_pause_state()&nbsp;displays&nbsp;the&nbsp;pause_state,&nbsp;and&nbsp;it&nbsp;is
&gt;&nbsp;&gt;&nbsp;not&nbsp;hidden&nbsp;behind&nbsp;the&nbsp;stats&nbsp;read&nbsp;role.&nbsp;&nbsp;I&nbsp;am&nbsp;not&nbsp;really&nbsp;convinced&nbsp;that
&gt;&nbsp;&gt;&nbsp;this&nbsp;is&nbsp;worth&nbsp;bothering&nbsp;to&nbsp;treat&nbsp;as&nbsp;an&nbsp;exception&nbsp;in&nbsp;this&nbsp;patch.&nbsp;&nbsp;It
&gt;&nbsp;&gt;&nbsp;makes&nbsp;it&nbsp;a&nbsp;bit&nbsp;more&nbsp;complicated,&nbsp;for&nbsp;little&nbsp;gain.&nbsp;&nbsp;I&nbsp;would&nbsp;just&nbsp;hide
&gt;&nbsp;&gt;&nbsp;all&nbsp;the&nbsp;fields&nbsp;behind&nbsp;the&nbsp;role&nbsp;granted,&nbsp;to&nbsp;keep&nbsp;the&nbsp;code&nbsp;simpler,&nbsp;even
&gt;&nbsp;&gt;&nbsp;if&nbsp;that's&nbsp;slightly&nbsp;inconsistent&nbsp;with&nbsp;pg_get_wal_replay_pause_state().
&gt;
&gt;&nbsp;I&nbsp;agree&nbsp;that&nbsp;exposing&nbsp;a&nbsp;subset&nbsp;of&nbsp;columns&nbsp;unconditionally&nbsp;is&nbsp;not&nbsp;worth
&gt;&nbsp;the&nbsp;added&nbsp;complexity.
&gt;
&gt;&nbsp;&gt;&nbsp;After&nbsp;writing&nbsp;this&nbsp;last&nbsp;point,&nbsp;as&nbsp;far&nbsp;as&nbsp;I&nbsp;can&nbsp;see&nbsp;there&nbsp;is&nbsp;a&nbsp;small
&gt;&nbsp;&gt;&nbsp;optimization&nbsp;possible&nbsp;in&nbsp;the&nbsp;patch.&nbsp;&nbsp;When&nbsp;a&nbsp;role&nbsp;is&nbsp;not&nbsp;granted
&gt;&nbsp;&gt;&nbsp;ROLE_PG_READ_ALL_STATS,&nbsp;we&nbsp;know&nbsp;that&nbsp;we&nbsp;will&nbsp;not&nbsp;return&nbsp;any
&gt;&nbsp;&gt;&nbsp;information&nbsp;so&nbsp;we&nbsp;could&nbsp;skip&nbsp;the&nbsp;spinlock&nbsp;acquisition&nbsp;and&nbsp;avoid
&gt;&nbsp;&gt;&nbsp;spinlock&nbsp;acquisitions&nbsp;when&nbsp;one&nbsp;queries&nbsp;the&nbsp;function&nbsp;but&nbsp;has&nbsp;no&nbsp;access
&gt;&nbsp;&gt;&nbsp;to&nbsp;its&nbsp;data.
&gt;
&gt;&nbsp;Makes&nbsp;sense&nbsp;to&nbsp;me.&nbsp;I'll&nbsp;apply&nbsp;it&nbsp;as&nbsp;suggested.
&gt;
&gt;&nbsp;&gt;&nbsp;+&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;True&nbsp;if&nbsp;a&nbsp;promotion&nbsp;has&nbsp;been&nbsp;triggered&nbsp;for&nbsp;this&nbsp;standby&nbsp;server.
&gt;&nbsp;&gt;
&gt;&nbsp;&gt;&nbsp;Standby&nbsp;servers&nbsp;are&nbsp;implied&nbsp;if&nbsp;data&nbsp;is&nbsp;returned,&nbsp;this&nbsp;sentence&nbsp;can&nbsp;be
&gt;&nbsp;&gt;&nbsp;simpler&nbsp;and&nbsp;cut&nbsp;the&nbsp;"standby&nbsp;server"&nbsp;part.
&gt;
&gt;&nbsp;+&nbsp;1
&gt;
&gt;&nbsp;&gt;&nbsp;+&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;Start&nbsp;LSN&nbsp;of&nbsp;the&nbsp;last&nbsp;WAL&nbsp;record&nbsp;replayed&nbsp;during&nbsp;recovery.
&gt;&nbsp;&gt;&nbsp;[...]
&gt;&nbsp;&gt;&nbsp;+&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;End&nbsp;LSN&nbsp;of&nbsp;the&nbsp;last&nbsp;WAL&nbsp;record&nbsp;replayed&nbsp;during&nbsp;recovery.
&gt;&nbsp;&gt;&nbsp;[...]
&gt;&nbsp;&gt;&nbsp;+&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;Timeline&nbsp;of&nbsp;the&nbsp;last&nbsp;replayed&nbsp;WAL&nbsp;record.
&gt;&nbsp;&gt;&nbsp;For&nbsp;other&nbsp;system&nbsp;views&nbsp;with&nbsp;LSN&nbsp;information,&nbsp;we&nbsp;don't&nbsp;use&nbsp;"LSN",&nbsp;but
&gt;&nbsp;&gt;&nbsp;"write-ahead&nbsp;log&nbsp;location".&nbsp;&nbsp;I'd&nbsp;suggest&nbsp;the&nbsp;same&nbsp;term&nbsp;here.&nbsp;&nbsp;These
&gt;&nbsp;&gt;&nbsp;three&nbsp;fields&nbsp;refer&nbsp;to&nbsp;the&nbsp;last&nbsp;record&nbsp;*successfully*&nbsp;replayed.&nbsp;&nbsp;It
&gt;&nbsp;&gt;&nbsp;seems&nbsp;important&nbsp;to&nbsp;mention&nbsp;this&nbsp;fact,&nbsp;I&nbsp;guess?
&gt;
&gt;&nbsp;I'll&nbsp;replace&nbsp;them.
&gt;
&gt;&nbsp;&gt;&nbsp;+&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;<structfield&gt;replay_end_lsn</structfield&gt;&nbsp;<type&gt;pg_lsn</type&gt;
&gt;&nbsp;&gt;&nbsp;+&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;</para&gt;
&gt;&nbsp;&gt;&nbsp;+&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;<para&gt;
&gt;&nbsp;&gt;&nbsp;+&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;Current&nbsp;replay&nbsp;position.&nbsp;When&nbsp;replaying&nbsp;a&nbsp;record,&nbsp;this&nbsp;is&nbsp;the&nbsp;end
&gt;&nbsp;&gt;&nbsp;+&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;position&nbsp;of&nbsp;the&nbsp;record&nbsp;being&nbsp;replayed;&nbsp;otherwise&nbsp;it&nbsp;equals
&gt;&nbsp;&gt;&nbsp;+&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;<structfield&gt;last_replayed_end_lsn</structfield&gt;.
&gt;&nbsp;&gt;
&gt;&nbsp;&gt;&nbsp;Slightly&nbsp;inexact.&nbsp;&nbsp;This&nbsp;is&nbsp;the&nbsp;end&nbsp;LSN&nbsp;+&nbsp;1.
&gt;
&gt;&nbsp;Yeh,&nbsp;this&nbsp;needs&nbsp;to&nbsp;be&nbsp;corrected.
&gt;
&gt;&nbsp;&gt;&nbsp;+&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;<structfield&gt;replay_end_lsn</structfield&gt;&nbsp;<type&gt;pg_lsn</type&gt;
&gt;&nbsp;&gt;&nbsp;[..]
&gt;&nbsp;&gt;&nbsp;+&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;<structfield&gt;replay_end_tli</structfield&gt;&nbsp;<type&gt;integer</type&gt;
&gt;&nbsp;&gt;
&gt;&nbsp;&gt;&nbsp;Okay,&nbsp;I&nbsp;can&nbsp;fall&nbsp;behind&nbsp;the&nbsp;addition&nbsp;of&nbsp;these&nbsp;two,&nbsp;it&nbsp;could&nbsp;be&nbsp;helpful
&gt;&nbsp;&gt;&nbsp;in&nbsp;debugging&nbsp;something&nbsp;like&nbsp;a&nbsp;locking&nbsp;issue&nbsp;when&nbsp;replaying&nbsp;a&nbsp;specific
&gt;&nbsp;&gt;&nbsp;record,&nbsp;I&nbsp;guess.&nbsp;&nbsp;At&nbsp;least&nbsp;I'd&nbsp;want&nbsp;to&nbsp;know&nbsp;what&nbsp;is&nbsp;happening&nbsp;for&nbsp;a
&gt;&nbsp;&gt;&nbsp;record&nbsp;currently&nbsp;being&nbsp;replayed.&nbsp;&nbsp;It&nbsp;seems&nbsp;to&nbsp;me&nbsp;that&nbsp;this&nbsp;could&nbsp;be
&gt;&nbsp;&gt;&nbsp;more&nbsp;precise,&nbsp;mentioning&nbsp;that&nbsp;these&nbsp;refer&nbsp;to&nbsp;a&nbsp;record&nbsp;*currently*
&gt;&nbsp;&gt;&nbsp;being&nbsp;replayed.
&gt;
&gt;&nbsp;I&nbsp;will&nbsp;adjust&nbsp;the&nbsp;docs&nbsp;to&nbsp;say&nbsp;these&nbsp;describe&nbsp;the&nbsp;record&nbsp;currently
&gt;&nbsp;being&nbsp;replayed,&nbsp;with&nbsp;replay_end_lsn&nbsp;being&nbsp;the&nbsp;end&nbsp;position&nbsp;+&nbsp;1.
&gt;
&gt;&nbsp;&gt;&nbsp;Is&nbsp;recoveryLastXTime&nbsp;actually&nbsp;that&nbsp;relevant&nbsp;to&nbsp;have?&nbsp;&nbsp;We&nbsp;use&nbsp;it&nbsp;for
&gt;&nbsp;&gt;&nbsp;logging&nbsp;and&nbsp;for&nbsp;recovery&nbsp;target&nbsp;times,&nbsp;which&nbsp;is&nbsp;something&nbsp;less
&gt;&nbsp;&gt;&nbsp;relevant&nbsp;than&nbsp;the&nbsp;other&nbsp;fields,&nbsp;especially&nbsp;if&nbsp;we&nbsp;think&nbsp;about&nbsp;standbys
&gt;&nbsp;&gt;&nbsp;where&nbsp;these&nbsp;have&nbsp;no&nbsp;targets&nbsp;to&nbsp;reach.
&gt;
&gt;&nbsp;I&nbsp;agree&nbsp;it&nbsp;is&nbsp;less&nbsp;central&nbsp;for&nbsp;standby&nbsp;monitoring&nbsp;(and&nbsp;partly&nbsp;overlaps
&gt;&nbsp;with&nbsp;pg_last_xact_replay_timestamp()),&nbsp;so&nbsp;I’ll&nbsp;remove&nbsp;it&nbsp;from&nbsp;this
&gt;&nbsp;view&nbsp;in&nbsp;the&nbsp;next&nbsp;revision.
&gt;
&gt;&nbsp;&gt;&nbsp;currentChunkStartTime,&nbsp;on&nbsp;the&nbsp;contrary,&nbsp;is&nbsp;much&nbsp;more&nbsp;relevant&nbsp;to&nbsp;me,
&gt;&nbsp;&gt;&nbsp;due&nbsp;to&nbsp;the&nbsp;fact&nbsp;that&nbsp;we&nbsp;use&nbsp;it&nbsp;in&nbsp;WaitForWALToBecomeAvailable()&nbsp;with
&gt;&nbsp;&gt;&nbsp;active&nbsp;WAL&nbsp;receivers&nbsp;running.
&gt;
&gt;&nbsp;Yeah,&nbsp;it&nbsp;could&nbsp;be&nbsp;useful&nbsp;for&nbsp;apply-delay/catch-up&nbsp;diagnostics.
&gt;

Here&nbsp;is&nbsp;the&nbsp;updated&nbsp;patch&nbsp;set.&nbsp;Please&nbsp;take&nbsp;a&nbsp;look.


--
Best,
Xuneng

  10. Re: Add pg_stat_recovery system view

    Xuneng Zhou <xunengzhou@gmail.com> — 2026-03-06T06:06:01Z

    Hi,

On Fri, Mar 6, 2026 at 12:00 PM Michael Paquier <michael@paquier.xyz> wrote:
>
> On Thu, Mar 05, 2026 at 09:01:35PM +0800, Xuneng Zhou wrote:
> > Here is the updated patch set. Please take a look.
>
> Thanks.  It was mostly fine, still:
> - The extra mention of ROLE_PG_READ_ALL_STATS was not necessary.  We
> already have one at the top of the section.
> - After sleeping on it, I have decided to keep recoveryLastXTime,
> partially because we have a system function to retrieve it already.
> - There was a refactoring piece possible when we switch a
> RecoveryPauseState to a non-translatable string, which I have applied
> separately

It looks cleaner.

> - Added one test with pg_stat_recovery checked on a standby.
> --
> Michael

Thanks for improving and pushing!

-- 
Best,
Xuneng

  11. Re: Add pg_stat_recovery system view

    Xuneng Zhou <xunengzhou@gmail.com> — 2026-03-06T06:21:49Z

    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 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

  12. Re: Add pg_stat_recovery system view

    Michael Paquier <michael@paquier.xyz> — 2026-03-06T06:30:17Z

    On Fri, Mar 06, 2026 at 02:21:49PM +0800, Xuneng Zhou wrote:
> 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.

Yes, I don't see a point in changing anything here.
--
Michael

  13. Re: Add pg_stat_recovery system view

    Chao Li <li.evan.chao@gmail.com> — 2026-03-06T06:32:01Z

    
> 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 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/

  14. Re: Add pg_stat_recovery system view

    Chao Li <li.evan.chao@gmail.com> — 2026-03-06T07:31:36Z

    
> On Mar 5, 2026, at 21:01, Xuneng Zhou <xunengzhou@gmail.com> wrote:
> 
> Hi,
> 
> On Thu, Mar 5, 2026 at 3:37 PM Xuneng Zhou <xunengzhou@gmail.com> wrote:
>> 
>> Hi Michael,
>> 
>> Thanks for the detailed feedback!
>> 
>> On Thu, Mar 5, 2026 at 11:58 AM Michael Paquier <michael@paquier.xyz> wrote:
>>> 
>>> On Wed, Mar 04, 2026 at 09:02:29AM +0800, Xuneng Zhou wrote:
>>>> Just rebase.
>>> 
>>> I have applied 0001, that simply moves some code around.
>>> 
>> 
>> Thanks for applying.
>> 
>>> Regarding 0002, I would recommend to not bump the catalog version in
>>> catversion.h when sending a patch to the lists.  This task is left to
>>> committers when the code gets merged into the tree.  And this is
>>> annoying for submitters because it can create a lot of conflicts.
>> 
>> I'll leave it untouched.
>> 
>>> Using a set-returning function is I think wrong here, betraying the
>>> representation of the recovery status as stored in the system.  We
>>> know that there is only one state of recovery, fixed in shared memory.
>>> Like the cousins of this new function, let's make thinks non-SRF,
>>> returning one row all the time with PG_RETURN_NULL() if the conditions
>>> for information display are not satisfied.  When we are not in
>>> recovery or when the role querying the function is not granted
>>> ROLE_PG_READ_ALL_STATS, that will simplify the patch as there is no
>>> need for the else branch with the nulls, as written in your patch.
>>> The field values are acquired the right way, spinlock acquisitions
>>> have to be short.
>> 
>> My earlier thought for keeping pg_stat_get_recovery as an SRF is to
>> make pg_stat_recovery simpler by avoiding an extra filter such as
>> WHERE s.promote_triggered IS NOT NULL to preserve 0/1-row semantics.
>> pg_stat_get_recovery_prefetch also uses the SRF pattern.
>> 
>>> Like pg_stat_wal_receiver, let's add to the view definition a qual to
>>> return a row only if the fields are not NULL.
>>> 
>>> pg_get_wal_replay_pause_state() displays the pause_state, and it is
>>> not hidden behind the stats read role.  I am not really convinced that
>>> this is worth bothering to treat as an exception in this patch.  It
>>> makes it a bit more complicated, for little gain.  I would just hide
>>> all the fields behind the role granted, to keep the code simpler, even
>>> if that's slightly inconsistent with pg_get_wal_replay_pause_state().
>> 
>> I agree that exposing a subset of columns unconditionally is not worth
>> the added complexity.
>> 
>>> After writing this last point, as far as I can see there is a small
>>> optimization possible in the patch.  When a role is not granted
>>> ROLE_PG_READ_ALL_STATS, we know that we will not return any
>>> information so we could skip the spinlock acquisition and avoid
>>> spinlock acquisitions when one queries the function but has no access
>>> to its data.
>> 
>> Makes sense to me. I'll apply it as suggested.
>> 
>>> +       True if a promotion has been triggered for this standby server.
>>> 
>>> Standby servers are implied if data is returned, this sentence can be
>>> simpler and cut the "standby server" part.
>> 
>> + 1
>> 
>>> +       Start LSN of the last WAL record replayed during recovery.
>>> [...]
>>> +       End LSN of the last WAL record replayed during recovery.
>>> [...]
>>> +       Timeline of the last replayed WAL record.
>>> For other system views with LSN information, we don't use "LSN", but
>>> "write-ahead log location".  I'd suggest the same term here.  These
>>> three fields refer to the last record *successfully* replayed.  It
>>> seems important to mention this fact, I guess?
>> 
>> I'll replace them.
>> 
>>> +       <structfield>replay_end_lsn</structfield> <type>pg_lsn</type>
>>> +      </para>
>>> +      <para>
>>> +       Current replay position. When replaying a record, this is the end
>>> +       position of the record being replayed; otherwise it equals
>>> +       <structfield>last_replayed_end_lsn</structfield>.
>>> 
>>> Slightly inexact.  This is the end LSN + 1.
>> 
>> Yeh, this needs to be corrected.
>> 
>>> +       <structfield>replay_end_lsn</structfield> <type>pg_lsn</type>
>>> [..]
>>> +       <structfield>replay_end_tli</structfield> <type>integer</type>
>>> 
>>> Okay, I can fall behind the addition of these two, it could be helpful
>>> in debugging something like a locking issue when replaying a specific
>>> record, I guess.  At least I'd want to know what is happening for a
>>> record currently being replayed.  It seems to me that this could be
>>> more precise, mentioning that these refer to a record *currently*
>>> being replayed.
>> 
>> I will adjust the docs to say these describe the record currently
>> being replayed, with replay_end_lsn being the end position + 1.
>> 
>>> Is recoveryLastXTime actually that relevant to have?  We use it for
>>> logging and for recovery target times, which is something less
>>> relevant than the other fields, especially if we think about standbys
>>> where these have no targets to reach.
>> 
>> I agree it is less central for standby monitoring (and partly overlaps
>> with pg_last_xact_replay_timestamp()), so I’ll remove it from this
>> view in the next revision.
>> 
>>> currentChunkStartTime, on the contrary, is much more relevant to me,
>>> due to the fact that we use it in WaitForWALToBecomeAvailable() with
>>> active WAL receivers running.
>> 
>> Yeah, it could be useful for apply-delay/catch-up diagnostics.
>> 
> 
> Here is the updated patch set. Please take a look.
> 
> 
> --
> Best,
> Xuneng
> <v3-0001-Add-pg_stat_recovery-system-view.patch><v3-0002-Refactor-move-XLogSource-enum-to-xlogrecovery.h.patch><v3-0003-Add-wal_source-column-to-pg_stat_recovery.patch>

0001 has been pushed.

0002 is pure refactoring and only moves the structure XLogSource from .h to .c, thus no comment.

A few comments on 0003.

1
```
+	/*
+	 * Source from which the startup process most recently read WAL data.
+	 * Updated when the startup process successfully reads WAL from a source.
+	 * Note: this reflects the read source, not the original receipt source;
+	 * streamed WAL read from local files will show XLOG_FROM_PG_WAL.
+	 */
+	XLogSource	lastReadSource;
```

This comment says, "streamed WAL read from local files will show XLOG_FROM_PG_WAL”. But I don’t see how the conversion happens. In WaitForWALToBecomeAvailable(), there is a path XLogFileRead() is called with XLOG_FROM_STREAM, but in XLogFileRead(), source is directly assigned to XLogRecoveryCtl->lastReadSource without conversion.

On the other side, if “stream” is impossible, then the doc should not mention it:
```
+         <para>
+          <literal>stream</literal>: WAL actively being streamed from the
+          upstream server.
+         </para>
```


2 Related to 1. WRT the new column name wal_source, it sounds like “where WAL is coming from”. Say the comment of lastReadSource is true, WAL from stream is shown as pg_wal, “wal source” sounds more like stream, and “wal read source” is pg_wal. From this perspective, I just feel the new column name should be something like “wal_read_source”.

Best regards,
--
Chao Li (Evan)
HighGo Software Co., Ltd.
https://www.highgo.com/

  15. Re: Add pg_stat_recovery system view

    Tom Lane <tgl@sss.pgh.pa.us> — 2026-03-06T15:45:05Z

    Chao Li <li.evan.chao@gmail.com> writes:
> 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.

No: you are incorrect and this snippet is perfectly normal (in fact,
probably copied-and-pasted from one of many other occurrences).
The actual coding rule is basically "use ereport() for user-facing
errors and elog() for not-supposed-to-happen errors".  What we're
after is to not expend translator effort on not-supposed-to-happen
error messages.  While you can build a ereport call that's not
translated, elog() is a lower-notation way to get the same result.
See [1], particularly the elog() discussion near the end of the
page.

I've not read the patch so I don't know if it made sane ereport-vs-
elog choices elsewhere, but this one is fine.

			regards, tom lane

[1] https://www.postgresql.org/docs/devel/error-message-reporting.html

  16. Re: Add pg_stat_recovery system view

    Xuneng Zhou <xunengzhou@gmail.com> — 2026-03-06T15:49:41Z

    Hi,

Thanks for looking into this.

On Fri, Mar 6, 2026 at 3:32 PM Chao Li <li.evan.chao@gmail.com> wrote:
>
>
>
> > On Mar 5, 2026, at 21:01, Xuneng Zhou <xunengzhou@gmail.com> wrote:
> >
> > Hi,
> >
> > On Thu, Mar 5, 2026 at 3:37 PM Xuneng Zhou <xunengzhou@gmail.com> wrote:
> >>
> >> Hi Michael,
> >>
> >> Thanks for the detailed feedback!
> >>
> >> On Thu, Mar 5, 2026 at 11:58 AM Michael Paquier <michael@paquier.xyz> wrote:
> >>>
> >>> On Wed, Mar 04, 2026 at 09:02:29AM +0800, Xuneng Zhou wrote:
> >>>> Just rebase.
> >>>
> >>> I have applied 0001, that simply moves some code around.
> >>>
> >>
> >> Thanks for applying.
> >>
> >>> Regarding 0002, I would recommend to not bump the catalog version in
> >>> catversion.h when sending a patch to the lists.  This task is left to
> >>> committers when the code gets merged into the tree.  And this is
> >>> annoying for submitters because it can create a lot of conflicts.
> >>
> >> I'll leave it untouched.
> >>
> >>> Using a set-returning function is I think wrong here, betraying the
> >>> representation of the recovery status as stored in the system.  We
> >>> know that there is only one state of recovery, fixed in shared memory.
> >>> Like the cousins of this new function, let's make thinks non-SRF,
> >>> returning one row all the time with PG_RETURN_NULL() if the conditions
> >>> for information display are not satisfied.  When we are not in
> >>> recovery or when the role querying the function is not granted
> >>> ROLE_PG_READ_ALL_STATS, that will simplify the patch as there is no
> >>> need for the else branch with the nulls, as written in your patch.
> >>> The field values are acquired the right way, spinlock acquisitions
> >>> have to be short.
> >>
> >> My earlier thought for keeping pg_stat_get_recovery as an SRF is to
> >> make pg_stat_recovery simpler by avoiding an extra filter such as
> >> WHERE s.promote_triggered IS NOT NULL to preserve 0/1-row semantics.
> >> pg_stat_get_recovery_prefetch also uses the SRF pattern.
> >>
> >>> Like pg_stat_wal_receiver, let's add to the view definition a qual to
> >>> return a row only if the fields are not NULL.
> >>>
> >>> pg_get_wal_replay_pause_state() displays the pause_state, and it is
> >>> not hidden behind the stats read role.  I am not really convinced that
> >>> this is worth bothering to treat as an exception in this patch.  It
> >>> makes it a bit more complicated, for little gain.  I would just hide
> >>> all the fields behind the role granted, to keep the code simpler, even
> >>> if that's slightly inconsistent with pg_get_wal_replay_pause_state().
> >>
> >> I agree that exposing a subset of columns unconditionally is not worth
> >> the added complexity.
> >>
> >>> After writing this last point, as far as I can see there is a small
> >>> optimization possible in the patch.  When a role is not granted
> >>> ROLE_PG_READ_ALL_STATS, we know that we will not return any
> >>> information so we could skip the spinlock acquisition and avoid
> >>> spinlock acquisitions when one queries the function but has no access
> >>> to its data.
> >>
> >> Makes sense to me. I'll apply it as suggested.
> >>
> >>> +       True if a promotion has been triggered for this standby server.
> >>>
> >>> Standby servers are implied if data is returned, this sentence can be
> >>> simpler and cut the "standby server" part.
> >>
> >> + 1
> >>
> >>> +       Start LSN of the last WAL record replayed during recovery.
> >>> [...]
> >>> +       End LSN of the last WAL record replayed during recovery.
> >>> [...]
> >>> +       Timeline of the last replayed WAL record.
> >>> For other system views with LSN information, we don't use "LSN", but
> >>> "write-ahead log location".  I'd suggest the same term here.  These
> >>> three fields refer to the last record *successfully* replayed.  It
> >>> seems important to mention this fact, I guess?
> >>
> >> I'll replace them.
> >>
> >>> +       <structfield>replay_end_lsn</structfield> <type>pg_lsn</type>
> >>> +      </para>
> >>> +      <para>
> >>> +       Current replay position. When replaying a record, this is the end
> >>> +       position of the record being replayed; otherwise it equals
> >>> +       <structfield>last_replayed_end_lsn</structfield>.
> >>>
> >>> Slightly inexact.  This is the end LSN + 1.
> >>
> >> Yeh, this needs to be corrected.
> >>
> >>> +       <structfield>replay_end_lsn</structfield> <type>pg_lsn</type>
> >>> [..]
> >>> +       <structfield>replay_end_tli</structfield> <type>integer</type>
> >>>
> >>> Okay, I can fall behind the addition of these two, it could be helpful
> >>> in debugging something like a locking issue when replaying a specific
> >>> record, I guess.  At least I'd want to know what is happening for a
> >>> record currently being replayed.  It seems to me that this could be
> >>> more precise, mentioning that these refer to a record *currently*
> >>> being replayed.
> >>
> >> I will adjust the docs to say these describe the record currently
> >> being replayed, with replay_end_lsn being the end position + 1.
> >>
> >>> Is recoveryLastXTime actually that relevant to have?  We use it for
> >>> logging and for recovery target times, which is something less
> >>> relevant than the other fields, especially if we think about standbys
> >>> where these have no targets to reach.
> >>
> >> I agree it is less central for standby monitoring (and partly overlaps
> >> with pg_last_xact_replay_timestamp()), so I’ll remove it from this
> >> view in the next revision.
> >>
> >>> currentChunkStartTime, on the contrary, is much more relevant to me,
> >>> due to the fact that we use it in WaitForWALToBecomeAvailable() with
> >>> active WAL receivers running.
> >>
> >> Yeah, it could be useful for apply-delay/catch-up diagnostics.
> >>
> >
> > Here is the updated patch set. Please take a look.
> >
> >
> > --
> > Best,
> > Xuneng
> > <v3-0001-Add-pg_stat_recovery-system-view.patch><v3-0002-Refactor-move-XLogSource-enum-to-xlogrecovery.h.patch><v3-0003-Add-wal_source-column-to-pg_stat_recovery.patch>
>
> 0001 has been pushed.
>
> 0002 is pure refactoring and only moves the structure XLogSource from .h to .c, thus no comment.
>
> A few comments on 0003.
>
> 1
> ```
> +       /*
> +        * Source from which the startup process most recently read WAL data.
> +        * Updated when the startup process successfully reads WAL from a source.
> +        * Note: this reflects the read source, not the original receipt source;
> +        * streamed WAL read from local files will show XLOG_FROM_PG_WAL.
> +        */
> +       XLogSource      lastReadSource;
> ```
>
> This comment says, "streamed WAL read from local files will show XLOG_FROM_PG_WAL”. But I don’t see how the conversion happens. In WaitForWALToBecomeAvailable(), there is a path XLogFileRead() is called with XLOG_FROM_STREAM, but in XLogFileRead(), source is directly assigned to XLogRecoveryCtl->lastReadSource without conversion.
>
> On the other side, if “stream” is impossible, then the doc should not mention it:
> ```
> +         <para>
> +          <literal>stream</literal>: WAL actively being streamed from the
> +          upstream server.
> +         </para>
> ```

you're right, the comment is wrong. There is no conversion. In
WaitForWALToBecomeAvailable(), when the walreceiver has flushed data
ahead of the startup process, the code deliberately opens the segment
file with XLOG_FROM_STREAM , and the existing comment explains the
intent:

* Use XLOG_FROM_STREAM so that source info is set correctly
* and XLogReceiptTime isn't changed.

XLogFileRead() then assigns that directly to lastReadSource without
conversion. So XLOG_FROM_STREAM is a valid observable value — it means
the segment was opened during active walreceiver streaming, even
though the actual I/O is against a local file in pg_wal. I think
stream documentation entry should stay.

> 2 Related to 1. WRT the new column name wal_source, it sounds like “where WAL is coming from”. Say the comment of lastReadSource is true, WAL from stream is shown as pg_wal, “wal source” sounds more like stream, and “wal read source” is pg_wal. From this perspective, I just feel the new column name should be something like “wal_read_source”.
>

agreed, wal_read_source seems to be a better name. It more closely
mirrors the underlying lastReadSource field and avoids ambiguity about
whether the column describes the delivery mechanism or the read path.

Please check the updated patches.


--
Best,
Xuneng

  17. Re: Add pg_stat_recovery system view

    Xuneng Zhou <xunengzhou@gmail.com> — 2026-03-06T15:52:38Z

    Hi Tom,

On Fri, Mar 6, 2026 at 11:45 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>
> Chao Li <li.evan.chao@gmail.com> writes:
> > 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.
>
> No: you are incorrect and this snippet is perfectly normal (in fact,
> probably copied-and-pasted from one of many other occurrences).
> The actual coding rule is basically "use ereport() for user-facing
> errors and elog() for not-supposed-to-happen errors".  What we're
> after is to not expend translator effort on not-supposed-to-happen
> error messages.  While you can build a ereport call that's not
> translated, elog() is a lower-notation way to get the same result.
> See [1], particularly the elog() discussion near the end of the
> page.
>
> I've not read the patch so I don't know if it made sane ereport-vs-
> elog choices elsewhere, but this one is fine.
>
>                         regards, tom lane
>
> [1] https://www.postgresql.org/docs/devel/error-message-reporting.html

Thanks for your clarification!

-- 
Best,
Xuneng

  18. Re: Add pg_stat_recovery system view

    Chao Li <li.evan.chao@gmail.com> — 2026-03-09T05:20:45Z

    
> On Mar 6, 2026, at 23:45, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> 
> Chao Li <li.evan.chao@gmail.com> writes:
>> 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.
> 
> No: you are incorrect and this snippet is perfectly normal (in fact,
> probably copied-and-pasted from one of many other occurrences).
> The actual coding rule is basically "use ereport() for user-facing
> errors and elog() for not-supposed-to-happen errors".  What we're
> after is to not expend translator effort on not-supposed-to-happen
> error messages.  While you can build a ereport call that's not
> translated, elog() is a lower-notation way to get the same result.
> See [1], particularly the elog() discussion near the end of the
> page.
> 
> I've not read the patch so I don't know if it made sane ereport-vs-
> elog choices elsewhere, but this one is fine.
> 
> regards, tom lane
> 
> [1] https://www.postgresql.org/docs/devel/error-message-reporting.html


Hi Tom, Thanks for the detailed explanation. Noted.

Would it make sense to mention this distinction in the header comments for ereport and elog in the code? For these logging APIs, I think developers are often more likely to read the code comments than the documentation.

Best regards,
--
Chao Li (Evan)
HighGo Software Co., Ltd.
https://www.highgo.com/

  19. Re: Add pg_stat_recovery system view

    Michael Paquier <michael@paquier.xyz> — 2026-03-09T06:06:20Z

    On Mon, Mar 09, 2026 at 01:20:45PM +0800, Chao Li wrote:
>> [1] https://www.postgresql.org/docs/devel/error-message-reporting.html
> 
> Would it make sense to mention this distinction in the header
> comments for ereport and elog in the code? For these logging APIs, I
> think developers are often more likely to read the code comments
> than the documentation.

As of the link already mentioned:
"Therefore, elog should be used only for internal errors .."
--
Michael

  20. Re: Add pg_stat_recovery system view

    Chao Li <li.evan.chao@gmail.com> — 2026-03-09T06:57:03Z

    
> On Mar 9, 2026, at 14:06, Michael Paquier <michael@paquier.xyz> wrote:
> 
> On Mon, Mar 09, 2026 at 01:20:45PM +0800, Chao Li wrote:
>>> [1] https://www.postgresql.org/docs/devel/error-message-reporting.html
>> 
>> Would it make sense to mention this distinction in the header
>> comments for ereport and elog in the code? For these logging APIs, I
>> think developers are often more likely to read the code comments
>> than the documentation.
> 
> As of the link already mentioned:
> "Therefore, elog should be used only for internal errors .."
> --
> Michael

Yes, the doc has clearly explained everything. I meant to ask if we want to add a bit explanation to the header comment of elog and ereport. Currently elog’s comment only says:
```
/*----------
* Old-style error reporting API: to be used in this way:
*        elog(ERROR, "portal \"%s\" not found", stmt->portalname);
*----------
```
That sounds elog is just old, but somewhat equivalent to ereport.

Best regards,
--
Chao Li (Evan)
HighGo Software Co., Ltd.
https://www.highgo.com/