Date: Fri, 08 Apr 2022 11:10:14 +0900 (JST)
Message-Id: <20220408.111014.1139039683831064200.horikyota.ntt@gmail.com>
To: andres@anarazel.de
Cc: melanieplageman@gmail.com, pryzby@telsasoft.com,
 thomas.munro@gmail.com, david.g.johnston@gmail.com,
 pgsql-hackers@lists.postgresql.org
Subject: Re: shared-memory based stats collector - v70
From: Kyotaro Horiguchi <horikyota.ntt@gmail.com>
In-Reply-To: <20220407233751.i573vgmuwicphd2t@alap3.anarazel.de>
References: <20220406030008.2qxipjxo776dwnqs@alap3.anarazel.de>
	<20220407072845.mwz7ybaode32kk26@alap3.anarazel.de>
	<20220407233751.i573vgmuwicphd2t@alap3.anarazel.de>
User-Agent: Mew version 6.8 on Emacs 26.1
Mime-Version: 1.0
Content-Type: Text/Plain; charset=us-ascii
Content-Transfer-Encoding: 7bit
Archived-At: <https://www.postgresql.org/message-id/20220408.111014.1139039683831064200.horikyota.ntt%40gmail.com>
Precedence: bulk

At Thu, 7 Apr 2022 16:37:51 -0700, Andres Freund <andres@anarazel.de> wrote in 
> Hi,
> 
> On 2022-04-07 00:28:45 -0700, Andres Freund wrote:
> > I've gotten through the main commits (and then a fix for the apparently
> > inevitable bug that's immediately highlighted by the buildfarm), and the first
> > test. I'll call it a night now, and work on the other tests & docs tomorrow.
> 
> I've gotten through the tests now. There's one known, not yet addressed, issue
> with the stats isolation test, see [1].
> 
> 
> Working on the docs. Found a few things worth raising:
> 
> 1)
> Existing text:
>    When the server shuts down cleanly, a permanent copy of the statistics
>    data is stored in the <filename>pg_stat</filename> subdirectory, so that
>    statistics can be retained across server restarts.  When recovery is
>    performed at server start (e.g., after immediate shutdown, server crash,
>    and point-in-time recovery), all statistics counters are reset.
> 
> The existing docs patch hadn't updated yet. My current edit is
> 
>    When the server shuts down cleanly, a permanent copy of the statistics
>    data is stored in the <filename>pg_stat</filename> subdirectory, so that
>    statistics can be retained across server restarts.  When crash recovery is
>    performed at server start (e.g., after immediate shutdown, server crash,
>    and point-in-time recovery, but not when starting a standby that was shut
>    down normally), all statistics counters are reset.
> 
> but I'm not sure the parenthetical is easy enough to understand?

I can read it. But I'm not sure that the difference is obvious for
average users between "starting a standby from a basebackup" and
"starting a standby after a normal shutdown"..

Other than that, it might be easier to read if the additional part
were moved out to the end of the paragraph, prefixing with "Note:
". For example,

...
statistics can be retained across server restarts.  When crash recovery is
performed at server start (e.g., after immediate shutdown, server crash,
and point-in-time recovery), all statistics counters are reset. Note that
crash recovery is not performed when starting a standby that was shut
down normally then all counters are retained.

> 2)
> The edit is not a problem, but it's hard to understand what the existing
> paragraph actually means?
> 
> diff --git a/doc/src/sgml/high-availability.sgml b/doc/src/sgml/high-availability.sgml
> index 3247e056663..8bfb584b752 100644
> --- a/doc/src/sgml/high-availability.sgml
> +++ b/doc/src/sgml/high-availability.sgml
> @@ -2222,17 +2222,17 @@ HINT:  You can then restart the server after making the necessary configuration
> ...
>     <para>
> -    The statistics collector is active during recovery. All scans, reads, blocks,
> +    The cumulative statistics system is active during recovery. All scans, reads, blocks,
>      index usage, etc., will be recorded normally on the standby. Replayed
>      actions will not duplicate their effects on primary, so replaying an
>      insert will not increment the Inserts column of pg_stat_user_tables.
>      The stats file is deleted at the start of recovery, so stats from primary
>      and standby will differ; this is considered a feature, not a bug.
>     </para>
> 
>     <para>

Agreed partially. It's too detailed.  It might not need to mention WAL
replay.

> I'll just commit the necessary bit, but we really ought to rephrase this.
> 
> 
> 
> 
> Greetings,
> 
> Andres Freund
> 
> [1] https://www.postgresql.org/message-id/20220407165709.jgdkrzqlkcwue6ko%40alap3.anarazel.de

-- 
Kyotaro Horiguchi
NTT Open Source Software Center