Re: Add pg_stat_recovery system view

Hi,

On Thu, Mar 5, 2026 at 3:37 PM Xuneng Zhou <xunengzhou(at)gmail(dot)com> wrote:
>
> Hi Michael,
>
> Thanks for the detailed feedback!
>
> On Thu, Mar 5, 2026 at 11:58 AM Michael Paquier <michael(at)paquier(dot)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