Re: Add pg_stat_recovery system view

yangyz Thu, 05 Mar 2026 21:49:13 -0800

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 <[email protected]&gt;
发件时间：2026年3月5日 21:01
收件人：Michael Paquier <[email protected]&gt;
抄送：pgsql-hackers <[email protected]&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;<[email protected]&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;<[email protected]&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

Re: Add pg_stat_recovery system view

Reply via email to