Date: Sat, 23 Jan 2021 19:15:40 +0900
From: Michael Paquier <michael@paquier.xyz>
To: Justin Pryzby <pryzby@telsasoft.com>
Cc: pgsql-hackers@postgresql.org, Bruce Momjian <bruce@momjian.us>,
	Thomas Munro <thomas.munro@gmail.com>
Subject: Re: doc review for v14
Message-ID: <YAv3TG9fOrNXxJoK@paquier.xyz>
References: <20201222041153.GK30237@telsasoft.com>
 <X+RNUu+zotOjfL7K@paquier.xyz>
 <20201227202604.GC26311@telsasoft.com>
MIME-Version: 1.0
Content-Type: multipart/signed; micalg=pgp-sha512;
	protocol="application/pgp-signature"; boundary="AKb2K7BD83NsRGMA"
Content-Disposition: inline
In-Reply-To: <20201227202604.GC26311@telsasoft.com>
Precedence: bulk


--AKb2K7BD83NsRGMA
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline

Hi Justin,

On Sun, Dec 27, 2020 at 02:26:05PM -0600, Justin Pryzby wrote:
> Thank you.

I have been looking at 0005, the patch dealing with the docs of the
replication stats, and have some comments.

        <para>
         Number of times transactions were spilled to disk while decoding changes
-        from WAL for this slot. Transactions may get spilled repeatedly, and
-        this counter gets incremented on every such invocation.
+        from WAL for this slot. A given transaction may be spilled multiple times, and
+        this counter is incremented each time.
       </para></entry>
The original can be a bit hard to read, and I don't think that the new
formulation is an improvement.  I actually find confusing that this
mixes in the same sentence that a transaction can be spilled multiple
times and increment this counter each time.  What about splitting that
into two sentences?  Here is an idea:
"This counter is incremented each time a transaction is spilled.  The
same transaction may be spilled multiple times."

-        Number of transactions spilled to disk after the memory used by
-        logical decoding of changes from WAL for this slot exceeds
+        Number of transactions spilled to disk because the memory used by
+        logical decoding of changes from WAL for this slot exceeded
What does "logical decoding of changes from WAL" mean?  Here is an
idea to clarify all that:
"Number of transactions spilled to disk once the memory used by
logical decoding to decode changes from WAL has exceeded
logical_decoding_work_mem."

         Number of in-progress transactions streamed to the decoding output plugin
-        after the memory used by logical decoding of changes from WAL for this
-        slot exceeds <literal>logical_decoding_work_mem</literal>. Streaming only
+        because the memory used by logical decoding of changes from WAL for this
+        slot exceeded <literal>logical_decoding_work_mem</literal>. Streaming only
         works with toplevel transactions (subtransactions can't be streamed
-        independently), so the counter does not get incremented for subtransactions+        independently), so the counter is not incremented for subtransactions.
I have the same issue here with "by logical decoding of changes from
WAL".  I'd say "after the memory used by logical decoding to decode
changes from WAL for this slot has exceeded logical_decoding_work_mem".

         output plugin while decoding changes from WAL for this slot. Transactions
-        may get streamed repeatedly, and this counter gets incremented on every
-        such invocation.
+        may be streamed multiple times, and this counter is incremented each time.
I would split this stuff into two sentences:
"This counter is incremented each time a transaction is streamed.  The
same transaction may be streamed multiple times.

          Resets statistics to zero for a single replication slot, or for all
-         replication slots in the cluster.  The argument can be either the name
-         of the slot to reset the stats or NULL.  If the argument is NULL, all
-         counters shown in the <structname>pg_stat_replication_slots</structname>
-         view for all replication slots are reset.
+         replication slots in the cluster.  The argument can be either NULL or the name
+         of a slot for which stats are to be reset.  If the argument is NULL, all
+         counters in the <structname>pg_stat_replication_slots</structname>
+         view are reset for all replication slots.
Here also, I find rather confusing that this paragraph tells multiple
times that NULL resets the stats for all the replication slots.  NULL
should use a <literal> markup, and it is cleaner to use "statistics"
rather than "stats" IMO.  So I guess we could simplify things as
follows:
"Resets statistics of the replication slot defined by the argument. If
the argument is NULL, resets statistics for all the replication
slots."
--
Michael

--AKb2K7BD83NsRGMA
Content-Type: application/pgp-signature; name="signature.asc"

-----BEGIN PGP SIGNATURE-----

iQIzBAABCgAdFiEEG72nH6vTowiyblFKnvQgOdbyQH0FAmAL90wACgkQnvQgOdby
QH1OrxAAlbacnemrqmEpyVgWmZ70jZlHSOQPTbaC6dAz662LOyKGYtoB8vFrvpEP
lC8kE5hg0HqO34K/i+DNT97MZMCswKbfCehej1T7/eZvGbnnX0NQcvt9KcbtaN9f
g8LVuA2/iYnLvMsRyRPxIu/Ocned2P9ovNWdVD4SIeNWGrRYobC9T6bfkkvgQwmM
2rnhtuvZwV/axUoCIdvInYywe4srqbBklVf0za49EgL6ThtlGqQpUL5cWwrpV8gn
8uVk0X3IQisI471d/04v+YedNd8+McdOoGtKyzc810RWBgv0rOgd3Gb2sDHDmOgm
yh4TvPAORG5LhQEP3Ei4kuxCdM1JeNcv5TLuz5cTTpx2yzGBs5/XZryFMCgJfFv6
+Uo+ldF8EYt0HDv/4mPdZweVC3nZHRsFjSe6SirR9inwfJQMp6RBrFY4eflZlEa0
5TuDAKG+iwCzz77/4cz51J7YGBQpIZYTFXhmtawI52pD5VoYE+0OAE70KlKGLcv2
GcWen8qgwV9o9er7Ezl8PMNtLVUpSLh2DCbpEAp1n+YwenD/nkuwqe54BWGK/FgQ
T+l0QvHYg/mFmHIWQ+cXPaatdiNZ1lV1OUal0jsTvcLpraGDAWqsTWj9K1n1n9Xi
ounhO6DyV8K0tTK5QIoAqXhLHBehY0BLOUgf01rcNd3YIcYW3cc=
=/cAF
-----END PGP SIGNATURE-----

--AKb2K7BD83NsRGMA--