Date: Wed, 15 Sep 2021 12:05:21 +0900
From: Michael Paquier <michael@paquier.xyz>
To: "Bossart, Nathan" <bossartn@amazon.com>
Cc: Kyotaro Horiguchi <horikyota.ntt@gmail.com>,
	"tgl@sss.pgh.pa.us" <tgl@sss.pgh.pa.us>,
	"robertmhaas@gmail.com" <robertmhaas@gmail.com>,
	"masao.fujii@oss.nttdata.com" <masao.fujii@oss.nttdata.com>,
	"pryzby@telsasoft.com" <pryzby@telsasoft.com>,
	"andres@anarazel.de" <andres@anarazel.de>,
	"magnus@hagander.net" <magnus@hagander.net>,
	"mark.dilger@enterprisedb.com" <mark.dilger@enterprisedb.com>,
	"don@seiler.us" <don@seiler.us>,
	"pgsql-hackers@postgresql.org" <pgsql-hackers@postgresql.org>
Subject: Re: Estimating HugePages Requirements?
Message-ID: <YUFi8ekig9VRVCb8@paquier.xyz>
References: <AD880348-DEA9-4C81-B55B-DF28AC859ABA@amazon.com>
 <506431.1631564654@sss.pgh.pa.us>
 <AAE8A2A7-5A52-4C92-9E5D-4C6C7F7ABD32@amazon.com>
 <20210914.094936.565951707453489109.horikyota.ntt@gmail.com>
 <AD5509B0-F747-4E17-BC77-51C5EFFDE4F3@amazon.com>
MIME-Version: 1.0
Content-Type: multipart/signed; micalg=pgp-sha256;
	protocol="application/pgp-signature"; boundary="YXZXfOaaPB8ueePN"
Content-Disposition: inline
In-Reply-To: <AD5509B0-F747-4E17-BC77-51C5EFFDE4F3@amazon.com>
Archived-At: 
 <https://www.postgresql.org/message-id/YUFi8ekig9VRVCb8%40paquier.xyz>
Precedence: bulk


--YXZXfOaaPB8ueePN
Content-Type: multipart/mixed; boundary="Z8lH/WIjrDMrcKuf"
Content-Disposition: inline


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

On Tue, Sep 14, 2021 at 06:00:44PM +0000, Bossart, Nathan wrote:
> I think I see more support for shared_memory_size_in_huge_pages than
> for huge_pages_needed_for_shared_memory at the moment.  I'll update
> the patch set in the next day or two to use
> shared_memory_size_in_huge_pages unless something changes in the
> meantime.

I have been looking at the patch to add the new GUC flag and the new
sequence for postgres -C, and we could have some TAP tests.  There
were two places that made sense to me: pg_checksums/t/002_actions.pl
and recovery/t/017_shm.pl.  I have chosen the former as it has
coverage across more platforms, and used data_checksums for this
purpose, with an extra positive test to check for the case where a GUC
can be queried while the server is running.

There are four parameters that are updated here:
- shared_memory_size
- wal_segment_size
- data_checksums
- data_directory_mode
That looks sensible, after looking at the full set of GUCs.

Attached is a refreshed patch (commit message is the same as v9 for
now), with some minor tweaks and the tests.

Thoughts?
--
Michael

--Z8lH/WIjrDMrcKuf
Content-Type: text/plain; charset=us-ascii
Content-Disposition: attachment;
	filename="v10-0001-Provide-useful-values-for-postgres-C-with-runtim.patch"
Content-Transfer-Encoding: quoted-printable

=46rom e24d151efe45e1bc7048ec72ea66097c94f633be Mon Sep 17 00:00:00 2001
=46rom: Michael Paquier <michael@paquier.xyz>
Date: Wed, 15 Sep 2021 12:02:32 +0900
Subject: [PATCH v10] Provide useful values for 'postgres -C' with=20
 runtime-computed GUCs

The -C option is handled before a small subset of GUCs that are
computed at runtime are initialized.  Unfortunately, we cannot move
this handling to after they are initialized without disallowing
'postgres -C' on a running server.  One notable reason for this is
that loadable libraries' _PG_init() functions are called before all
runtime-computed GUCs are initialized, and this is not guaranteed
to be safe to do on running servers.

In order to provide useful values for 'postgres -C' for runtime-
computed GUCs, this change adds a new section for handling just
these GUCs just before shared memory is initialized.  While users
won't be able to use -C for runtime-computed GUCs on running
servers, providing a useful value with this restriction seems
better than not providing a useful value at all.
---
 src/include/utils/guc.h               |  6 ++++
 src/backend/postmaster/postmaster.c   | 50 +++++++++++++++++++++++----
 src/backend/utils/misc/guc.c          |  8 ++---
 src/bin/pg_checksums/t/002_actions.pl | 21 ++++++++++-
 doc/src/sgml/ref/postgres-ref.sgml    | 11 ++++--
 5 files changed, 82 insertions(+), 14 deletions(-)

diff --git a/src/include/utils/guc.h b/src/include/utils/guc.h
index a7c3a4958e..aa18d304ac 100644
--- a/src/include/utils/guc.h
+++ b/src/include/utils/guc.h
@@ -229,6 +229,12 @@ typedef enum
=20
 #define GUC_EXPLAIN			  0x100000	/* include in explain */
=20
+/*
+ * GUC_RUNTIME_COMPUTED is intended for runtime-computed GUCs that are only
+ * available via 'postgres -C' if the server is not running.
+ */
+#define GUC_RUNTIME_COMPUTED  0x200000
+
 #define GUC_UNIT				(GUC_UNIT_MEMORY | GUC_UNIT_TIME)
=20
=20
diff --git a/src/backend/postmaster/postmaster.c b/src/backend/postmaster/p=
ostmaster.c
index b2fe420c3c..f5b91c7af7 100644
--- a/src/backend/postmaster/postmaster.c
+++ b/src/backend/postmaster/postmaster.c
@@ -896,15 +896,31 @@ PostmasterMain(int argc, char *argv[])
 	if (output_config_variable !=3D NULL)
 	{
 		/*
-		 * "-C guc" was specified, so print GUC's value and exit.  No extra
-		 * permission check is needed because the user is reading inside the
-		 * data dir.
+		 * If this is a runtime-computed GUC, it hasn't yet been initialized,
+		 * and the present value is not useful.  However, this is a convenient
+		 * place to print the value for most GUCs because it is safe to run
+		 * postmaster startup to this point even if the server is already
+		 * running.  For the handful of runtime-computed GUCs that we can't
+		 * provide meaningful values for yet, we wait until later in
+		 * postmaster startup to print the value.  We won't be able to use -C
+		 * on running servers for those GUCs, but otherwise this option is
+		 * unusable for them.
 		 */
-		const char *config_val =3D GetConfigOption(output_config_variable,
-												 false, false);
+		int			flags =3D GetConfigOptionFlags(output_config_variable, true);
=20
-		puts(config_val ? config_val : "");
-		ExitPostmaster(0);
+		if ((flags & GUC_RUNTIME_COMPUTED) =3D=3D 0)
+		{
+			/*
+			 * "-C guc" was specified, so print GUC's value and exit.  No
+			 * extra permission check is needed because the user is reading
+			 * inside the data dir.
+			 */
+			const char *config_val =3D GetConfigOption(output_config_variable,
+													 false, false);
+
+			puts(config_val ? config_val : "");
+			ExitPostmaster(0);
+		}
 	}
=20
 	/* Verify that DataDir looks reasonable */
@@ -1033,6 +1049,26 @@ PostmasterMain(int argc, char *argv[])
 	 */
 	InitializeShmemGUCs();
=20
+	/*
+	 * If -C was specified with a runtime-computed GUC, we held off printing
+	 * the value earlier, as the GUC was not yet initialized.  We handle -C
+	 * for most GUCs before we lock the data directory so that the option may
+	 * be used on a running server.  However, a handful of GUCs are runtime-
+	 * computed and do not have meaningful values until after locking the data
+	 * directory, and we cannot safely calculate their values earlier on a
+	 * running server.  At this point, such GUCs should be properly
+	 * initialized, and we haven't yet set up shared memory, so this is a good
+	 * time to handle the -C option for these special GUCs.
+	 */
+	if (output_config_variable !=3D NULL)
+	{
+		const char *config_val =3D GetConfigOption(output_config_variable,
+												 false, false);
+
+		puts(config_val ? config_val : "");
+		ExitPostmaster(0);
+	}
+
 	/*
 	 * Set up shared memory and semaphores.
 	 */
diff --git a/src/backend/utils/misc/guc.c b/src/backend/utils/misc/guc.c
index 23236fa4c3..a6e4fcc24e 100644
--- a/src/backend/utils/misc/guc.c
+++ b/src/backend/utils/misc/guc.c
@@ -1983,7 +1983,7 @@ static struct config_bool ConfigureNamesBool[] =3D
 		{"data_checksums", PGC_INTERNAL, PRESET_OPTIONS,
 			gettext_noop("Shows whether data checksums are turned on for this clust=
er."),
 			NULL,
-			GUC_NOT_IN_SAMPLE | GUC_DISALLOW_IN_FILE
+			GUC_NOT_IN_SAMPLE | GUC_DISALLOW_IN_FILE | GUC_RUNTIME_COMPUTED
 		},
 		&data_checksums,
 		false,
@@ -2342,7 +2342,7 @@ static struct config_int ConfigureNamesInt[] =3D
 		{"shared_memory_size", PGC_INTERNAL, PRESET_OPTIONS,
 			gettext_noop("Shows the size of the server's main shared memory area (r=
ounded up to the nearest MB)."),
 			NULL,
-			GUC_NOT_IN_SAMPLE | GUC_DISALLOW_IN_FILE | GUC_UNIT_MB
+			GUC_NOT_IN_SAMPLE | GUC_DISALLOW_IN_FILE | GUC_UNIT_MB | GUC_RUNTIME_CO=
MPUTED
 		},
 		&shared_memory_size_mb,
 		0, 0, INT_MAX,
@@ -2407,7 +2407,7 @@ static struct config_int ConfigureNamesInt[] =3D
 						 "in the form accepted by the chmod and umask system "
 						 "calls. (To use the customary octal format the number "
 						 "must start with a 0 (zero).)"),
-			GUC_NOT_IN_SAMPLE | GUC_DISALLOW_IN_FILE
+			GUC_NOT_IN_SAMPLE | GUC_DISALLOW_IN_FILE | GUC_RUNTIME_COMPUTED
 		},
 		&data_directory_mode,
 		0700, 0000, 0777,
@@ -3231,7 +3231,7 @@ static struct config_int ConfigureNamesInt[] =3D
 		{"wal_segment_size", PGC_INTERNAL, PRESET_OPTIONS,
 			gettext_noop("Shows the size of write ahead log segments."),
 			NULL,
-			GUC_UNIT_BYTE | GUC_NOT_IN_SAMPLE | GUC_DISALLOW_IN_FILE
+			GUC_UNIT_BYTE | GUC_NOT_IN_SAMPLE | GUC_DISALLOW_IN_FILE | GUC_RUNTIME_=
COMPUTED
 		},
 		&wal_segment_size,
 		DEFAULT_XLOG_SEG_SIZE,
diff --git a/src/bin/pg_checksums/t/002_actions.pl b/src/bin/pg_checksums/t=
/002_actions.pl
index a18c104a94..e98586c3f8 100644
--- a/src/bin/pg_checksums/t/002_actions.pl
+++ b/src/bin/pg_checksums/t/002_actions.pl
@@ -10,7 +10,7 @@ use PostgresNode;
 use TestLib;
=20
 use Fcntl qw(:seek);
-use Test::More tests =3D> 63;
+use Test::More tests =3D> 69;
=20
=20
 # Utility routine to create and check a table with corrupted checksums
@@ -178,11 +178,30 @@ command_fails(
 	[ 'pg_checksums', '--enable', '--filenode', '1234', '-D', $pgdata ],
 	"fails when relfilenodes are requested and action is --enable");
=20
+# Test postgres -C for an offline cluster.
+# Run-time GUCs are safe to query here.  Note that a lock file is created,
+# then unlinked, leading to an extra LOG entry showing in stderr.
+command_checks_all(
+	[ 'postgres', '-D', $pgdata, '-C', 'data_checksums' ],
+	0,
+	[qr/^on$/],
+	# LOG entry when unlinking lock file.
+	[qr/database system is shut down/],
+	'data_checksums=3Don is reported on an offline cluster');
+
 # Checks cannot happen with an online cluster
 $node->start;
 command_fails([ 'pg_checksums', '--check', '-D', $pgdata ],
 	"fails with online cluster");
=20
+# Test postgres -C on an online cluster.
+command_fails_like(
+	[ 'postgres', '-D', $pgdata, '-C', 'data_checksums' ],
+	qr/lock file .* already exists/,
+	'data_checksums is not reported on an online cluster');
+command_ok([ 'postgres', '-D', $pgdata, '-C', 'work_mem' ],
+	'non-runtime parameter is reported on an online cluster');
+
 # Check corruption of table on default tablespace.
 check_relation_corruption($node, 'corrupt1', 'pg_default');
=20
diff --git a/doc/src/sgml/ref/postgres-ref.sgml b/doc/src/sgml/ref/postgres=
-ref.sgml
index 4aaa7abe1a..02142ffab2 100644
--- a/doc/src/sgml/ref/postgres-ref.sgml
+++ b/doc/src/sgml/ref/postgres-ref.sgml
@@ -133,13 +133,20 @@ PostgreSQL documentation
       <listitem>
        <para>
         Prints the value of the named run-time parameter, and exits.
-        (See the <option>-c</option> option above for details.)  This can
-        be used on a running server, and returns values from
+        (See the <option>-c</option> option above for details.)  This
+        returns values from
         <filename>postgresql.conf</filename>, modified by any parameters
         supplied in this invocation.  It does not reflect parameters
         supplied when the cluster was started.
        </para>
=20
+       <para>
+        This can be used on a running server for most parameters.  However,
+        the server must be shut down for some runtime-computed parameters
+        (e.g., <xref linkend=3D"guc-shared-memory-size"/>, and
+        <xref linkend=3D"guc-wal-segment-size"/>).
+       </para>
+
        <para>
         This option is meant for other programs that interact with a server
         instance, such as <xref linkend=3D"app-pg-ctl"/>, to query configu=
ration
--=20
2.33.0


--Z8lH/WIjrDMrcKuf--

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

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

iQIzBAEBCAAdFiEEG72nH6vTowiyblFKnvQgOdbyQH0FAmFBYvEACgkQnvQgOdby
QH14Kg/+IM1vOW3kQofi6KkJc7QD085JqwmCjVATcqvWFyjdn0KcfyXqhtOrXPKb
lVHo8L9DZpdSVJaT3hd/8B1yDS8fqMlCgzcKks2KxgSnacLRJp5VtNayq4/av6l0
EamtdiXjPAo6gWmuwSUVjCf71RqAOFuWKFtP9DGv93asKA3CJem0gcGnSdEFmSKu
Oh6cUn7gCZbJa/4cfkmo2Y6M9Vev5aJrDGt1yhVcFVE1sE0JJa7jr5+cKgsTjchu
a6dTktRZ2SheQUV243KiFk+rr3v6NBwL7d5VPB4fXSOLmzcEESj5we1FBP/ChP6f
OFT9V8yJHVv7HI/0lSe9EfAgOj4ftoGZGc4IL7+niizGclQrOYAu9q566xutdAQW
1uq5sz0FVexaJoHaDcHqWT3Ex0nIsb9lCakZMnHRttT+NsIQUDqBadbvYdll7pJh
8KX5MTl+iVtgAC46dbIYLY2/Hj4g3vteJzLigoMV0qY2JavAx9YD33KGMKoGpSHd
De9Ytz9AQR+jaP1+v23CZJjJUc9NBhT6E7uXaS/F+D2u69I+DVnCNEtNl4NkS964
jNVbuMbu+JcanMSjqfn8aZ3zLfBFFRGAn+3f8yDqrkWFNL4rurY6V7kxzKXX5FwJ
u0x5Ld9r+qxrFQWJ1LXifYIIvG74kL9af7QM7YnjRrZKmzz9ot0=
=Q/cO
-----END PGP SIGNATURE-----

--YXZXfOaaPB8ueePN--