Date: Tue, 30 Nov 2021 15:36:45 +0900
From: Michael Paquier <michael@paquier.xyz>
To: Justin Pryzby <pryzby@telsasoft.com>
Cc: pgsql-hackers@lists.postgresql.org
Subject: Re: GUC flags
Message-ID: <YaXGfc+tRYFFN+bo@paquier.xyz>
References: <20211129030833.GJ17618@telsasoft.com>
 <YaSJcceKORqKS+cM@paquier.xyz>
MIME-Version: 1.0
Content-Type: multipart/signed; micalg=pgp-sha512;
	protocol="application/pgp-signature"; boundary="iA3HINUSGLD+vE3p"
Content-Disposition: inline
In-Reply-To: <YaSJcceKORqKS+cM@paquier.xyz>
Archived-At: 
 <https://www.postgresql.org/message-id/YaXGfc%2BtRYFFN%2Bbo%40paquier.xyz>
Precedence: bulk


--iA3HINUSGLD+vE3p
Content-Type: multipart/mixed; boundary="FBEfDBVKUI45k/gh"
Content-Disposition: inline


--FBEfDBVKUI45k/gh
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
Content-Transfer-Encoding: quoted-printable

On Mon, Nov 29, 2021 at 05:04:01PM +0900, Michael Paquier wrote:
> 0001, to adjust the units, and 0003, to make the GUC descriptions less
> unit-dependent, are good ideas.

Actually, after sleeping on it and doing some digging in
codesearch.debian.org, changing the units of max_identifier_length,
block_size and wal_block_size could induce some breakages for anything
using a SHOW command, something that becomes particularly worse now
that SHOW is supported in replication connections, and it would force=20
clients to know and parse the units of a value.  Perhaps I am being
too careful here, but that could harm a lot of users.  It is worth
noting that I have found some driver code making use of pg_settings,
which would not be influenced by such a change, but it is unsafe to
assume that everybody does that.

The addition of GUC_EXPLAIN for enable_incremental_sort, the comment
fix for autovacuum_freeze_max_age, the use of COMPAT_OPTIONS_PREVIOUS
for ssl_renegotiation_limit and the addition of GUC_NOT_IN_SAMPLE for
trace_recovery_messages are fine, though.

> I am not completely sure that all the contents of 0002 are
> improvements, but the suggestions done for huge_pages,
> ssl_passphrase_command_supports_reload, checkpoint_warning and
> commit_siblings seem fine to me.

Hmm, I think the patched description of checkpoint_warning is not that
much an improvement compared to the current one.  While the current
description uses the term "checkpoint segments", which is, I agree,
weird.  The new one would lose the term "checkpoint", making the short
description of the parameter lose some of its context.

I have done a full review of the patch set, and applied the obvious
fixes/improvements as of be54551.  Attached is an extra patch based on
the contents of the whole set sent upthread:
- Improvement of the description of checkpoint_segments.
- Reworded the description of all parameters using "N units", rather
than just switching to "this period of units".  I have been using
something more generic.

Thoughts?
--
Michael

--FBEfDBVKUI45k/gh
Content-Type: text/x-diff; charset=us-ascii
Content-Disposition: attachment; filename="guc-descriptions.patch"
Content-Transfer-Encoding: quoted-printable

diff --git a/src/backend/utils/misc/guc.c b/src/backend/utils/misc/guc.c
index 25ac4ca85b..927212b8ae 100644
--- a/src/backend/utils/misc/guc.c
+++ b/src/backend/utils/misc/guc.c
@@ -2131,8 +2131,8 @@ static struct config_int ConfigureNamesInt[] =3D
 {
 	{
 		{"archive_timeout", PGC_SIGHUP, WAL_ARCHIVING,
-			gettext_noop("Forces a switch to the next WAL file if a "
-						 "new file has not been started within N seconds."),
+			gettext_noop("Sets the amount of time to wait before forcing a "
+						 "switch to the next WAL file."),
 			NULL,
 			GUC_UNIT_S
 		},
@@ -2142,7 +2142,8 @@ static struct config_int ConfigureNamesInt[] =3D
 	},
 	{
 		{"post_auth_delay", PGC_BACKEND, DEVELOPER_OPTIONS,
-			gettext_noop("Waits N seconds on connection startup after authenticatio=
n."),
+			gettext_noop("Sets the amount of seconds to wait on connection "
+						 "startup after authentication."),
 			gettext_noop("This allows attaching a debugger to the process."),
 			GUC_NOT_IN_SAMPLE | GUC_UNIT_S
 		},
@@ -2762,7 +2763,8 @@ static struct config_int ConfigureNamesInt[] =3D
 	{
 		/* Not for general use */
 		{"pre_auth_delay", PGC_SIGHUP, DEVELOPER_OPTIONS,
-			gettext_noop("Waits N seconds on connection startup before authenticati=
on."),
+			gettext_noop("Sets the amount of seconds to wait on connection "
+						 "startup before authentication."),
 			gettext_noop("This allows attaching a debugger to the process."),
 			GUC_NOT_IN_SAMPLE | GUC_UNIT_S
 		},
@@ -2819,10 +2821,10 @@ static struct config_int ConfigureNamesInt[] =3D
=20
 	{
 		{"checkpoint_warning", PGC_SIGHUP, WAL_CHECKPOINTS,
-			gettext_noop("Enables warnings if checkpoint segments are filled more "
-						 "frequently than this."),
+			gettext_noop("Sets the maximum time before warning if checkpoints "
+						 "triggered by WAL volume happen too frequently."),
 			gettext_noop("Write a message to the server log if checkpoints "
-						 "caused by the filling of checkpoint segment files happens more "
+						 "caused by the filling of WAL segment files happens more "
 						 "frequently than this number of seconds. Zero turns off the warning=
=2E"),
 			GUC_UNIT_S
 		},
@@ -3006,7 +3008,8 @@ static struct config_int ConfigureNamesInt[] =3D
=20
 	{
 		{"log_parameter_max_length", PGC_SUSET, LOGGING_WHAT,
-			gettext_noop("When logging statements, limit logged parameter values to=
 first N bytes."),
+			gettext_noop("Sets the maximum amount of data logged for bind "
+						 "parameter values when logging statements."),
 			gettext_noop("-1 to print values in full."),
 			GUC_UNIT_BYTE
 		},
@@ -3017,7 +3020,8 @@ static struct config_int ConfigureNamesInt[] =3D
=20
 	{
 		{"log_parameter_max_length_on_error", PGC_USERSET, LOGGING_WHAT,
-			gettext_noop("When reporting an error, limit logged parameter values to=
 first N bytes."),
+			gettext_noop("Sets the maximum amount of data logged for bind "
+						 "parameter values when logging statements, on error."),
 			gettext_noop("-1 to print values in full."),
 			GUC_UNIT_BYTE
 		},
@@ -3143,7 +3147,8 @@ static struct config_int ConfigureNamesInt[] =3D
=20
 	{
 		{"log_rotation_age", PGC_SIGHUP, LOGGING_WHERE,
-			gettext_noop("Automatic log file rotation will occur after N minutes."),
+			gettext_noop("Sets the maximum amount of time to wait before "
+						 "forcing log file rotation."),
 			NULL,
 			GUC_UNIT_MIN
 		},
@@ -3154,7 +3159,8 @@ static struct config_int ConfigureNamesInt[] =3D
=20
 	{
 		{"log_rotation_size", PGC_SIGHUP, LOGGING_WHERE,
-			gettext_noop("Automatic log file rotation will occur after N kilobytes.=
"),
+			gettext_noop("Sets the maximum size of log file to reach before "
+						 "forcing log file rotation."),
 			NULL,
 			GUC_UNIT_KB
 		},

--FBEfDBVKUI45k/gh--

--iA3HINUSGLD+vE3p
Content-Type: application/pgp-signature; name="signature.asc"

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

iQIzBAABCgAdFiEEG72nH6vTowiyblFKnvQgOdbyQH0FAmGlxn0ACgkQnvQgOdby
QH0dqBAAnSFBY6rdjBERmRugdLXfhBXxH6KAU8JoZRR/fcSKq0k9u4lRgHl6md5Q
S4VguQANEF+8/IWMNzLmblMp/U1np6DtQutD2ddSYEra4D6lXKIid7zdM0TqzVav
IGxixFPCFI7oJ8Gpx5sjXJ27+UxYASnVVMGZpuCy6GUzVcMVWguvbxdK6pVLJwPO
mTqwPFaDWg0F0GCqoc8K32p4AbW5koVf6FenekoE32WC0vfuHJne1+Pve0fkkRab
AVKqn6oUp3vicMyFNZS8eNtgCOAmYIns5jfwMdvvgMVSCODSKQB0S2cAZd4sGygT
qGgDYO16kMdJMN57evbSJVZ3rnzJYLYYlbbVj0MJR2WYYiWA/RhzZlcj8zdD6uCE
jLHYZT+iHO8gx4dX8JNCW0qQmNa3LbMr/id/dYPFkoKU61+ebexZkUF1HTomq7cL
hnFD98YboPQ/iOV7KPbw/1z/13Vg6Vy28NEzk3On2wgDnjVtDW1Y6ieLBQP4woZK
w4PjeEPfdSRIRiHMvMrT3WctAYYOyuqEiYKeXTyot/bq1a32uAO7J7a+WhMori9T
Mtkn9vF+4N1g44CqZ/GQUiKDEDsktNPL9+dRrNeBCU+ia/ihyprUQOm1GkXLFg5S
0L3zUudy9YhA7hF/6dDL1CX5Ri8RYlh81WSnyjkFZpOrBNDEr/4=
=SMHd
-----END PGP SIGNATURE-----

--iA3HINUSGLD+vE3p--