Subject: Estimating HugePages Requirements?

From: Don Seiler <[email-masked]>
Date: Wed, 9 Jun 2021 11:41:52 -0500

Good day,

I'm trying to set up a chef recipe to reserve enough HugePages on a linux
system for our PG servers. A given VM will only host one PG cluster and
that will be the only thing on that host that uses HugePages. Blogs that
I've seen suggest that it would be as simple as taking the shared_buffers
setting and dividing that by 2MB (huge page size), however I found that I
needed some more.

In my test case, shared_buffers is set to 4003MB (calculated by chef) but
PG failed to start until I reserved a few hundred more MB. When I checked
VmPeak, it was 4321MB, so I ended up having to reserve over 2161 huge
pages, over a hundred more than I had originally thought.

I'm told other factors contribute to this additional memory requirement,
such as max_connections, wal_buffers, etc. I'm wondering if anyone has been
able to come up with a reliable method for determining the HugePages
requirements for a PG cluster based on the GUC values (that would be known
at deployment time).

Thanks,
Don.

-- 
Don Seiler
www.seiler.us

---

From: Julien Rouhaud <[email-masked]>
Date: Thu, 10 Jun 2021 01:23:28 +0800

On Thu, Jun 10, 2021 at 12:42 AM Don Seiler <[email-masked]> wrote:
>
> I'm told other factors contribute to this additional memory requirement, such as max_connections, wal_buffers, etc. I'm wondering if anyone has been able to come up with a reliable method for determining the HugePages requirements for a PG cluster based on the GUC values (that would be known at deployment time).

It also depends on modules like pg_stat_statements and their own
configuration.  I think that you can find the required size that your
current configuration will allocate with:

SELECT sum(allocated_size) FROM pg_shmem_allocations ;

---

From: Vijaykumar Jain <[email-masked]>
Date: Thu, 10 Jun 2021 00:15:40 +0530

Please ignore, if you have read the blog below, if not, at the end of it
there is a github repo which has mem specs for various tpcc benchmarks.
Ofcourse, your workload expectations may vary from the test scenarios used,
but just in case.

Settling the Myth of Transparent HugePages for Databases - Percona Database
Performance Blog
<https://www.percona.com/blog/2019/03/06/settling-the-myth-of-transparent-hugepages-for-databases/>

---

From: Don Seiler <[email-masked]>
Date: Wed, 9 Jun 2021 13:52:19 -0500

On Wed, Jun 9, 2021 at 1:45 PM Vijaykumar Jain <
[email-masked]> wrote:

> Ofcourse, your workload expectations may vary from the test scenarios
> used, but just in case.
>
> Settling the Myth of Transparent HugePages for Databases - Percona
> Database Performance Blog

That blog post is about transparent huge pages, which is different than
HugePages I'm looking at here. We already disable THP as a matter of course.

-- 
Don Seiler
www.seiler.us

---

From: Bruce Momjian <[email-masked]>
Date: Wed, 9 Jun 2021 15:01:40 -0400

On Wed, Jun  9, 2021 at 01:52:19PM -0500, Don Seiler wrote:
> On Wed, Jun 9, 2021 at 1:45 PM Vijaykumar Jain <[email-masked]
> > wrote:
> 

This blog post talks about sizing huge pages too:

	https://momjian.us/main/blogs/pgblog/2021.html#April_12_2021

-- 
  Bruce Momjian  <[email-masked]>        https://momjian.us
  EDB                                      https://enterprisedb.com

  If only the physical world exists, free will is an illusion.

---

From: Magnus Hagander <[email-masked]>
Date: Wed, 9 Jun 2021 21:07:09 +0200

> SELECT sum(allocated_size) FROM pg_shmem_allocations ;

I wonder how hard it would be to for example expose that through a
commandline switch or tool.

The point being that in order to run the query you suggest, the server
must already be running. There is no way to use this to estimate the
size that you're going to need after changing the value of
shared_buffers, which is a very common scenario. (You can change it,
restart without using huge pages because it fails, run that query,
change huge pages, and restart again -- but that's not exactly...
convenient)

-- 
 Magnus Hagander
 Me: https://www.hagander.net/
 Work: https://www.redpill-linpro.com/

---

From: Tom Lane <[email-masked]>
Date: Wed, 09 Jun 2021 15:15:37 -0400

> commandline switch or tool.

Just try to start the server and see if it complains.
For instance, with shared_buffers=10000000 I get

2021-06-09 15:08:56.821 EDT [1428121] FATAL:  could not map anonymous shared memory: Cannot allocate memory
2021-06-09 15:08:56.821 EDT [1428121] HINT:  This error usually means that PostgreSQL's request for a shared memory segment exceeded available memory, swap space, or huge pages. To reduce the request size (currently 83720568832 bytes), reduce PostgreSQL's shared memory usage, perhaps by reducing shared_buffers or max_connections.

Of course, if it *does* start, you can do the other thing.

Admittedly, we could make that easier somehow; but if it took
25 years for somebody to ask for this, I'm not sure it's
worth creating a feature to make it a shade easier.

			regards, tom lane

---

From: Magnus Hagander <[email-masked]>
Date: Wed, 9 Jun 2021 21:23:50 +0200

> Of course, if it *does* start, you can do the other thing.

Well, I have to *stop* the existing one first, most likely, otherwise
there won't be enough huge pages (or indeed memory) available. And if
then doesn't start, you're looking at extended downtime.

You can automate this to minimize it (set the value in the conf, stop
old, start new, if new doesn't start then stop new, reconfigure, start
old again), but it's *far* from friendly.

This process works when you're setting up a brand new server with
nobody using it. It doesn't work well, or at all, when you actually
have active users on it..

> worth creating a feature to make it a shade easier.

We haven't had huge page support for 25 years, "only" since 9.4 so
about 7 years.

And for every year that passes, huge pages become more interesting in
that in general memory sizes increase so the payoff of using them is
increased.

Using huge pages *should* be a trivial improvement to set up. But it's
in my experience complicated enough that many just skip it simply for
that reason.

---

From: Tom Lane <[email-masked]>
Date: Wed, 09 Jun 2021 15:28:45 -0400

> there won't be enough huge pages (or indeed memory) available.

I'm not following.  If you have a production server running, its
pg_shmem_allocations total should already be a pretty good guide
to what you need to configure HugePages for.  You need to know to
round that up, of course --- but if you aren't building a lot of
slop into the HugePages configuration anyway, you'll get burned
down the road.

			regards, tom lane

---

From: Magnus Hagander <[email-masked]>
Date: Wed, 9 Jun 2021 21:30:52 +0200

> slop into the HugePages configuration anyway, you'll get burned

I'm talking about the case when you want to *change* the value for
shared_buffers (or other parameters that would change the amount of
required huge pages), on a system where you're using huge pages.
pg_shmem_allocations will tell you what you need with the current
value, not what you need with the new value.

But yes, you can do some math around it and make a well educated
guess. But it would be very convenient to have the system able to do
that for you.

---

From: "Bossart, Nathan" <[email-masked]>
Date: Wed, 9 Jun 2021 20:52:47 +0000

moving to pgsql-hackers@

On 6/9/21, 9:41 AM, "Don Seiler" <[email-masked]> wrote:
> I'm trying to set up a chef recipe to reserve enough HugePages on a
> linux system for our PG servers. A given VM will only host one PG
> cluster and that will be the only thing on that host that uses
> HugePages. Blogs that I've seen suggest that it would be as simple
> as taking the shared_buffers setting and dividing that by 2MB (huge
> page size), however I found that I needed some more.
>
> In my test case, shared_buffers is set to 4003MB (calculated by
> chef) but PG failed to start until I reserved a few hundred more MB.
> When I checked VmPeak, it was 4321MB, so I ended up having to
> reserve over 2161 huge pages, over a hundred more than I had
> originally thought.
>
> I'm told other factors contribute to this additional memory
> requirement, such as max_connections, wal_buffers, etc. I'm
> wondering if anyone has been able to come up with a reliable method
> for determining the HugePages requirements for a PG cluster based on
> the GUC values (that would be known at deployment time).

In RDS, we've added a pg_ctl option that returns the amount of shared
memory required.  Basically, we start up postmaster just enough to get
an accurate value from CreateSharedMemoryAndSemaphores() and then shut
down.  The patch is quite battle-tested at this point (we first
started using it in 2017, and we've been enabling huge pages by
default since v10).  I'd be happy to clean it up and submit it for
discussion in pgsql-hackers@ if there is interest.

Nathan

---

From: Mark Dilger <[email-masked]>
Date: Wed, 9 Jun 2021 15:50:52 -0700

> On Jun 9, 2021, at 1:52 PM, Bossart, Nathan <[email-masked]> wrote:
> 
> I'd be happy to clean it up and submit it for

Yes, I'd like to see it.  Thanks for offering.

—
Mark Dilger
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

---

From: P C <[email-masked]>
Date: Thu, 10 Jun 2021 07:33:39 +0530

I agree, its confusing for many and that confusion arises from the fact
that you usually talk of shared_buffers in MB or GB whereas hugepages have
to be configured in units of 2mb. But once they understand they realize its
pretty simple.

Don, we have experienced the same not just with postgres but also with
oracle. I havent been able to get to the root of it, but what we usually do
is, we add another 100-200 pages and that works for us. If the SGA or
shared_buffers is high eg 96gb, then we add 250-500 pages. Those few
hundred MBs  may be wasted (because the moment you configure hugepages, the
operating system considers it as used and does not use it any more) but
nowadays, servers have 64 or 128 gb RAM easily and wasting that 500mb to
1gb does not hurt really.

HTH

---

From: "Bossart, Nathan" <[email-masked]>
Date: Thu, 10 Jun 2021 03:09:24 +0000

> Yes, I'd like to see it.  Thanks for offering.

Here's the general idea.  It still needs a bit of polishing, but I'm
hoping this is enough to spark some discussion on the approach.

Nathan

---

From: Don Seiler <[email-masked]>
Date: Wed, 9 Jun 2021 22:55:08 -0500

> 1gb does not hurt really.

I don't have a problem with the math, just wanted to know if it was
possible to better estimate what the actual requirements would be at
deployment time. My fallback will probably be you did and just pad with an
extra 512MB by default.

Don.

---

From: Justin Pryzby <[email-masked]>
Date: Thu, 10 Jun 2021 19:23:33 -0500

> deployment time. My fallback will probably be you did and just pad with an

It's because the huge allocation isn't just shared_buffers, but also
wal_buffers:

| The amount of shared memory used for WAL data that has not yet been written to disk.
| The default setting of -1 selects a size equal to 1/32nd (about 3%) of shared_buffers, ...

.. and other stuff:

src/backend/storage/ipc/ipci.c
	 * Size of the Postgres shared-memory block is estimated via
	 * moderately-accurate estimates for the big hogs, plus 100K for the
	 * stuff that's too small to bother with estimating.
	 *
	 * We take some care during this phase to ensure that the total size
	 * request doesn't overflow size_t.  If this gets through, we don't
	 * need to be so careful during the actual allocation phase.
	 */
	size = 100000;
	size = add_size(size, PGSemaphoreShmemSize(numSemas));
	size = add_size(size, SpinlockSemaSize());
	size = add_size(size, hash_estimate_size(SHMEM_INDEX_SIZE,
											 sizeof(ShmemIndexEnt)));
	size = add_size(size, dsm_estimate_size());
	size = add_size(size, BufferShmemSize());
	size = add_size(size, LockShmemSize());
	size = add_size(size, PredicateLockShmemSize());
	size = add_size(size, ProcGlobalShmemSize());
	size = add_size(size, XLOGShmemSize());
	size = add_size(size, CLOGShmemSize());
	size = add_size(size, CommitTsShmemSize());
	size = add_size(size, SUBTRANSShmemSize());
	size = add_size(size, TwoPhaseShmemSize());
	size = add_size(size, BackgroundWorkerShmemSize());
	size = add_size(size, MultiXactShmemSize());
	size = add_size(size, LWLockShmemSize());
	size = add_size(size, ProcArrayShmemSize());
	size = add_size(size, BackendStatusShmemSize());
	size = add_size(size, SInvalShmemSize());
	size = add_size(size, PMSignalShmemSize());
	size = add_size(size, ProcSignalShmemSize());
	size = add_size(size, CheckpointerShmemSize());
	size = add_size(size, AutoVacuumShmemSize());
	size = add_size(size, ReplicationSlotsShmemSize());
	size = add_size(size, ReplicationOriginShmemSize());
	size = add_size(size, WalSndShmemSize());
	size = add_size(size, WalRcvShmemSize());
	size = add_size(size, PgArchShmemSize());
	size = add_size(size, ApplyLauncherShmemSize());
	size = add_size(size, SnapMgrShmemSize());
	size = add_size(size, BTreeShmemSize());
	size = add_size(size, SyncScanShmemSize());
	size = add_size(size, AsyncShmemSize());
#ifdef EXEC_BACKEND
	size = add_size(size, ShmemBackendArraySize());
#endif

	/* freeze the addin request size and include it */
	addin_request_allowed = false;
	size = add_size(size, total_addin_request);

        /* might as well round it off to a multiple of a typical page size */
        size = add_size(size, 8192 - (size % 8192));

BTW, I think it'd be nice if this were a NOTICE:
| elog(DEBUG1, "mmap(%zu) with MAP_HUGETLB failed, huge pages disabled: %m", allocsize);

-- 
Justin

---

From: Don Seiler <[email-masked]>
Date: Mon, 14 Jun 2021 09:16:39 -0500

> | elog(DEBUG1, "mmap(%zu) with MAP_HUGETLB failed, huge pages disabled:

Great detail. I did some trial and error around just a few variables
(shared_buffers, wal_buffers, max_connections) and came up with a formula
that seems to be "good enough" for at least a rough default estimate.

The pseudo-code is basically:

ceiling((shared_buffers + 200 + (25 * shared_buffers/1024) +
10*(max_connections-100)/200 + wal_buffers-16)/2)

This assumes that all values are in MB and that wal_buffers is set to a
value other than the default of -1 obviously. I decided to default
wal_buffers to 16MB in our environments since that's what -1 should go to
based on the description in the documentation for an instance with
shared_buffers of the sizes in our deployments.

This formula did come up a little short (2MB) when I had a low
shared_buffers value at 2GB. Raising that starting 200 value to something
like 250 would take care of that. The limited testing I did based on
different values we see across our production deployments worked otherwise.
Please let me know what you folks think. I know I'm ignoring a lot of other
factors, especially given what Justin recently shared.

The remaining trick for me now is to calculate this in chef since
shared_buffers and wal_buffers attributes are strings with the unit ("MB")
in them, rather than just numerical values. Thinking of changing that
attribute to be just that and assume/require MB to make the calculations
easier.

-- 
Don Seiler
www.seiler.us

---

From: "Bossart, Nathan" <[email-masked]>
Date: Mon, 9 Aug 2021 22:57:18 +0000

> hoping this is enough to spark some discussion on the approach.

Here's a rebased version of the patch.

Nathan

---

From: Zhihong Yu <[email-masked]>
Date: Mon, 9 Aug 2021 16:10:27 -0700

On Mon, Aug 9, 2021 at 3:57 PM Bossart, Nathan <[email-masked]> wrote:

> Hi,

-extern void CreateSharedMemoryAndSemaphores(void);
+extern Size CreateSharedMemoryAndSemaphores(bool size_only);

Should the parameter be enum / bitmask so that future addition would not
change the method signature ?

Cheers

---

From: "Bossart, Nathan" <[email-masked]>
Date: Mon, 9 Aug 2021 23:48:34 +0000

On 8/9/21, 4:05 PM, "Zhihong Yu" <[email-masked]> wrote:
> Should the parameter be enum / bitmask so that future addition would not change the method signature ?

I don't have a strong opinion about this.  I don't feel that it's
really necessary, but if reviewers want a bitmask instead, I can
change it.

Nathan

---

From: Justin Pryzby <[email-masked]>
Date: Mon, 9 Aug 2021 18:58:53 -0500

> | The default setting of -1 selects a size equal to 1/32nd (about 3%) of shared_buffers, ...

I wonder if this shouldn't be solved the other way around:

Define shared_buffers as the exact size to be allocated/requested from the OS
(regardless of whether they're huge pages or not), and have postgres compute
everything else based on that.  So shared_buffers=2GB would end up being 1950MB
(or so) of buffer cache.  We'd have to check that after the other allocations,
there's still at least 128kB left for the buffer cache.  Maybe we'd have to
bump the minimum value of shared_buffers.

---

From: Andres Freund <[email-masked]>
Date: Mon, 9 Aug 2021 20:38:32 -0700

Hi,

> bump the minimum value of shared_buffers.

I don't like that. How much "other" shared memory we're going to need is
very hard to predict and depends on extensions, configuration options
like max_locks_per_transaction, max_connections to a significant
degree. This way the user ends up needing to guess at least as much as
before to get to a sensible shared_buffers.

Greetings,

Andres Freund

---

From: Andres Freund <[email-masked]>
Date: Mon, 9 Aug 2021 20:42:05 -0700

Hi,

On 2021-08-09 22:57:18 +0000, Bossart, Nathan wrote:

> @@ -1026,6 +1031,18 @@ PostmasterMain(int argc, char *argv[])
>  	 */
>  	InitializeMaxBackends();
>  
> +	if (output_shmem)
> +	{
> +		char output[64];
> +		Size size;
> +
> +		size = CreateSharedMemoryAndSemaphores(true);
> +		sprintf(output, "%zu", size);
> +
> +		puts(output);
> +		ExitPostmaster(0);
> +	}

I don't like putting this into PostmasterMain(). Either BootstrapMain()
(specifically checker mode) or GucInfoMain() seem like better places.

> -void
> -CreateSharedMemoryAndSemaphores(void)
> +Size
> +CreateSharedMemoryAndSemaphores(bool size_only)
>  {
>  	PGShmemHeader *shim = NULL;
>  
> @@ -161,6 +161,9 @@ CreateSharedMemoryAndSemaphores(void)
> +		if (size_only)
> +			return size;
> +
>  		elog(DEBUG3, "invoking IpcMemoryCreate(size=%zu)", size);
>  
>  		/*
> @@ -288,4 +291,6 @@ CreateSharedMemoryAndSemaphores(void)
>  	 */
>  	if (shmem_startup_hook)
>  		shmem_startup_hook();
> +
> +	return 0;
>  }

That seems like an ugly API to me. Why don't we split the size
determination and shmem creation functions into two?

Greetings,

Andres Freund

---

From: "Bossart, Nathan" <[email-masked]>
Date: Wed, 11 Aug 2021 23:23:52 +0000

> (specifically checker mode) or GucInfoMain() seem like better places.

I think BootstrapModeMain() makes the most sense.  It fits in nicely
with the --check logic that's already there.  With v3, the following
command can be used to retrieve the amount of shared memory required.

        postgres --output-shmem -D dir

While testing this new option, I noticed that you can achieve similar
results today with the following command, although this one will
actually try to create the shared memory, too.

        postgres --check -D dir -c log_min_messages=debug3 2> >(grep IpcMemoryCreate)

IMO the new option is still handy, but I can see the argument that it
might not be necessary.

> determination and shmem creation functions into two?

I did it this way in v3.

Nathan

---

From: Michael Paquier <[email-masked]>
Date: Fri, 27 Aug 2021 15:46:35 +0900

> actually try to create the shared memory, too.

That may not be the best option.

> IMO the new option is still handy, but I can see the argument that it

A separate option looks handy.  Wouldn't it be better to document it
in postgres-ref.sgml then?
--
Michael

---

From: Magnus Hagander <[email-masked]>
Date: Fri, 27 Aug 2021 16:40:27 +0200

> That may not be the best option.

I would say that can be a disastrous option.

First of all it would probably not work if you already have something
running -- especially when using huge pages. And if it does work, in
that or other scenarios, it can potentially have significant impact on
a running cluster to suddenly allocate many GB of more memory...

> in postgres-ref.sgml then?

I'd say a lot more than just handy. I don't think the workaround is
really all that useful.

(haven't looked at the actual patch yet, just commenting on the principle)

---

From: "Bossart, Nathan" <[email-masked]>
Date: Fri, 27 Aug 2021 18:16:01 +0000

> a running cluster to suddenly allocate many GB of more memory...

The v3 patch actually didn't work if the server was already running.
I removed that restriction in v4.

> I'd say a lot more than just handy. I don't think the workaround is

I added some documentation in v4.

Nathan

---

From: Andres Freund <[email-masked]>
Date: Fri, 27 Aug 2021 11:46:48 -0700

> I'd say a lot more than just handy. I don't think the workaround is

+1

---

From: "Bossart, Nathan" <[email-masked]>
Date: Fri, 27 Aug 2021 19:27:18 +0000

> I added some documentation in v4.

I realized that my attempt at documenting this new option was missing
some important context about the meaning of the return value when used
against a running server.  I added that in v5.

Nathan

---

From: Andres Freund <[email-masked]>
Date: Fri, 27 Aug 2021 12:38:13 -0700

Hi,

On 2021-08-27 19:27:18 +0000, Bossart, Nathan wrote:
> +     <varlistentry>
> +      <term><option>--output-shmem</option></term>
> +      <listitem>
> +       <para>
> +        Prints the amount of shared memory required for the given
> +        configuration and exits.  This can be used on a running server, but
> +        the return value reflects the amount of shared memory needed based
> +        on the current invocation.  It does not return the amount of shared
> +        memory in use by the running server.  This must be the first
> +        argument on the command line.
> +       </para>
> +
> +       <para>
> +        This option is useful for determining the number of huge pages
> +        needed for the server.  For more information, see
> +        <xref linkend="linux-huge-pages"/>.
> +       </para>
> +      </listitem>
> +     </varlistentry>
> +

One thing I wonder is if this wouldn't better be dealt with in a more generic
way. While this is the most problematic runtime computed GUC, it's not the
only one. What if we introduced a new shared_memory_size GUC, and made
--describe-config output it? Perhaps adding --describe-config=guc-name?

I also wonder if we should output the number of hugepages needed instead of
the "raw" bytes of shared memory. The whole business about figuring out the
huge page size, dividing the shared memory size by that and then rounding up
could be removed in that case. Due to huge_page_size it's not even immediately
obvious which huge page size one should use...

> diff --git a/src/backend/main/main.c b/src/backend/main/main.c
> index 3a2a0d598c..c141ae3d1c 100644
> --- a/src/backend/main/main.c
> +++ b/src/backend/main/main.c
> @@ -182,9 +182,11 @@ main(int argc, char *argv[])
>  	 */
>  
>  	if (argc > 1 && strcmp(argv[1], "--check") == 0)
> -		BootstrapModeMain(argc, argv, true);
> +		BootstrapModeMain(argc, argv, true, false);
> +	else if (argc > 1 && strcmp(argv[1], "--output-shmem") == 0)
> +		BootstrapModeMain(argc, argv, false, true);
>  	else if (argc > 1 && strcmp(argv[1], "--boot") == 0)
> -		BootstrapModeMain(argc, argv, false);
> +		BootstrapModeMain(argc, argv, false, false);
>  #ifdef EXEC_BACKEND
>  	else if (argc > 1 && strncmp(argv[1], "--fork", 6) == 0)
>  		SubPostmasterMain(argc, argv);

help() needs an update too.

> diff --git a/src/backend/storage/ipc/ipci.c b/src/backend/storage/ipc/ipci.c
> index 3e4ec53a97..b225b1ee70 100644
> --- a/src/backend/storage/ipc/ipci.c
> +++ b/src/backend/storage/ipc/ipci.c
> @@ -75,6 +75,87 @@ RequestAddinShmemSpace(Size size)
>  	total_addin_request = add_size(total_addin_request, size);
>  }
>  
> +/*
> + * CalculateShmemSize
> + *		Calculates the amount of shared memory and number of semaphores needed.
> + *
> + * If num_semaphores is not NULL, it will be set to the number of semaphores
> + * required.
> + *
> + * Note that this function freezes the additional shared memory request size
> + * from loadable modules.
> + */
> +Size
> +CalculateShmemSize(int *num_semaphores)
> +{

Can you split this into a separate commit? It feels fairy uncontroversial to
me, so I think we could just apply it soon?

Greetings,

Andres Freund

---

From: "Bossart, Nathan" <[email-masked]>
Date: Fri, 27 Aug 2021 20:16:40 +0000

> obvious which huge page size one should use...

I like both of these ideas.

> me, so I think we could just apply it soon?

I attached a patch for just the uncontroversial part, which is
unfortunately all I have time for today.

Nathan

---

From: Michael Paquier <[email-masked]>
Date: Sat, 28 Aug 2021 11:00:11 +0900

> I like both of these ideas.

That pretty much looks like -C in concept, isn't it?  Except that you
cannot get the actual total shared memory value because we'd do this
operation before loading shared_preload_libraries and miss any amount
asked by extensions.  There is a problem similar when attempting to do
postgres -C data_checksums, for example, which would output an
incorrect value even if the cluster has data checksums enabled.
--
Michael

---

From: Justin Pryzby <[email-masked]>
Date: Fri, 27 Aug 2021 22:57:22 -0500

> incorrect value even if the cluster has data checksums enabled.

Since we don't want to try to allocate the huge pages, and we also don't want
to compute based on shared_buffers alone, did anyone consider if pg_controldata
is the right place to put this ?

It includes a lot of related stuff:

max_connections setting:              100
max_worker_processes setting:         8
 - (added in 2013: 6bc8ef0b7f1f1df3998745a66e1790e27424aa0c)
max_wal_senders setting:              10
max_prepared_xacts setting:           2
max_locks_per_xact setting:           64

I'm not sure if there's any reason these aren't also shown (?)
autovacuum_max_workers - added in 2007: e2a186b03
max_predicate_locks_per_xact - added in 2011: dafaa3efb
max_logical_replication_workers
max_replication_slots

-- 
Justin

---

From: "Bossart, Nathan" <[email-masked]>
Date: Sat, 28 Aug 2021 05:36:37 +0000

> incorrect value even if the cluster has data checksums enabled.

Attached is a hacky attempt at adding a shared_memory_size GUC in a
way that could be used with -C.  This should include the amount of
shared memory requested by extensions, too.  As long as huge_page_size
is nonzero, it seems easy enough to provide the number of huge pages
needed as well.

Nathan

---

From: Michael Paquier <[email-masked]>
Date: Mon, 30 Aug 2021 16:29:19 +0900

> is nonzero, it seems easy enough to provide the number of huge pages

Yes, the implementation is not good.  The key thing is that by wanting
to support shared_memory_size with the -C switch of postgres, we need
to call process_shared_preload_libraries before output_config_variable. 
This additionally means to call ApplyLauncherRegister() before that so
as all the bgworker slots are not taken first.  Going through
_PG_init() also means that we'd better use ChangeToDataDir()
beforehand.

Attached is a WIP to show how the order of the operations could be
changed, as that's easier to grasp.  Even if we don't do that, having
the GUC and the refactoring of CalculateShmemSize() would still be
useful, as one could just query an existing instance for an estimation
of huge pages for a cloned one.

The GUC shared_memory_size should have GUC_NOT_IN_SAMPLE and
GUC_DISALLOW_IN_FILE, with some documentation, of course.  I added the
flags to the GUC, not the docs.   The code setting up the GUC is not
good either.  It would make sense to just have that in a small wrapper
of ipci.c, perhaps.
--
Michael
From e2345206dacec8b4454c3b429d0cfac01d5a0a58 Mon Sep 17 00:00:00 2001
From: Michael Paquier <[email-masked]>
Date: Mon, 30 Aug 2021 16:24:59 +0900
Subject: [PATCH] Add shared_memory_size GUC

---
 src/include/storage/ipc.h           |   1 +
 src/backend/postmaster/postmaster.c |  60 +++++++-----
 src/backend/storage/ipc/ipci.c      | 142 ++++++++++++++++------------
 src/backend/utils/misc/guc.c        |  13 +++
 4 files changed, 135 insertions(+), 81 deletions(-)

diff --git a/src/include/storage/ipc.h b/src/include/storage/ipc.h
index 753a6dd4d7..80e191d407 100644
--- a/src/include/storage/ipc.h
+++ b/src/include/storage/ipc.h
@@ -77,6 +77,7 @@ extern void check_on_shmem_exit_lists_are_empty(void);
 /* ipci.c */
 extern PGDLLIMPORT shmem_startup_hook_type shmem_startup_hook;
 
+extern Size CalculateShmemSize(int *num_semaphores);
 extern void CreateSharedMemoryAndSemaphores(void);
 
 #endif							/* IPC_H */
diff --git a/src/backend/postmaster/postmaster.c b/src/backend/postmaster/postmaster.c
index 9c2c98614a..0d3b4b1be0 100644
--- a/src/backend/postmaster/postmaster.c
+++ b/src/backend/postmaster/postmaster.c
@@ -893,6 +893,44 @@ PostmasterMain(int argc, char *argv[])
 	if (!SelectConfigFiles(userDoption, progname))
 		ExitPostmaster(2);
 
+	/* Verify that DataDir looks reasonable */
+	checkDataDir();
+
+	/* Check that pg_control exists */
+	checkControlFile();
+
+	/* And switch working directory into it */
+	ChangeToDataDir();
+
+	/*
+	 * Register the apply launcher.  Since it registers a background worker,
+	 * it needs to be called before InitializeMaxBackends(), and it's probably
+	 * a good idea to call it before any modules had chance to take the
+	 * background worker slots.
+	 */
+	ApplyLauncherRegister();
+
+	/*
+	 * Process any libraries that should be preloaded at postmaster start.
+	 * Thie happens before running -C, so as it is possible to get an
+	 * estimation of the total shared memory size allocated to this system,
+	 * accounting for the portion from loaded libraries.
+	 */
+	process_shared_preload_libraries();
+
+	/* XXX: this should be moved around */
+	{
+		char buf[64];
+		Size size;
+
+		size = CalculateShmemSize(NULL);
+		size += (1024 * 1024) - 1;
+		size /= (1024 * 1024);
+
+		sprintf(buf, "%lu MB", size);
+		SetConfigOption("shared_memory_size", buf, PGC_INTERNAL, PGC_S_OVERRIDE);
+	}
+
 	if (output_config_variable != NULL)
 	{
 		/*
@@ -907,15 +945,6 @@ PostmasterMain(int argc, char *argv[])
 		ExitPostmaster(0);
 	}
 
-	/* Verify that DataDir looks reasonable */
-	checkDataDir();
-
-	/* Check that pg_control exists */
-	checkControlFile();
-
-	/* And switch working directory into it */
-	ChangeToDataDir();
-
 	/*
 	 * Check for invalid combinations of GUC settings.
 	 */
@@ -996,19 +1025,6 @@ PostmasterMain(int argc, char *argv[])
 	 */
 	LocalProcessControlFile(false);
 
-	/*
-	 * Register the apply launcher.  Since it registers a background worker,
-	 * it needs to be called before InitializeMaxBackends(), and it's probably
-	 * a good idea to call it before any modules had chance to take the
-	 * background worker slots.
-	 */
-	ApplyLauncherRegister();
-
-	/*
-	 * process any libraries that should be preloaded at postmaster start
-	 */
-	process_shared_preload_libraries();
-
 	/*
 	 * Initialize SSL library, if specified.
 	 */
diff --git a/src/backend/storage/ipc/ipci.c b/src/backend/storage/ipc/ipci.c
index 3e4ec53a97..b225b1ee70 100644
--- a/src/backend/storage/ipc/ipci.c
+++ b/src/backend/storage/ipc/ipci.c
@@ -75,6 +75,87 @@ RequestAddinShmemSpace(Size size)
 	total_addin_request = add_size(total_addin_request, size);
 }
 
+/*
+ * CalculateShmemSize
+ *		Calculates the amount of shared memory and number of semaphores needed.
+ *
+ * If num_semaphores is not NULL, it will be set to the number of semaphores
+ * required.
+ *
+ * Note that this function freezes the additional shared memory request size
+ * from loadable modules.
+ */
+Size
+CalculateShmemSize(int *num_semaphores)
+{
+	Size size;
+	int numSemas;
+
+	/* Compute number of semaphores we'll need */
+	numSemas = ProcGlobalSemas();
+	numSemas += SpinlockSemas();
+
+	/* Return the number of semaphores if requested by the caller */
+	if (num_semaphores)
+		*num_semaphores = numSemas;
+
+	/*
+	 * Size of the Postgres shared-memory block is estimated via moderately-
+	 * accurate estimates for the big hogs, plus 100K for the stuff that's too
+	 * small to bother with estimating.
+	 *
+	 * We take some care to ensure that the total size request doesn't overflow
+	 * size_t.  If this gets through, we don't need to be so careful during the
+	 * actual allocation phase.
+	 */
+	size = 100000;
+	size = add_size(size, PGSemaphoreShmemSize(numSemas));
+	size = add_size(size, SpinlockSemaSize());
+	size = add_size(size, hash_estimate_size(SHMEM_INDEX_SIZE,
+											 sizeof(ShmemIndexEnt)));
+	size = add_size(size, dsm_estimate_size());
+	size = add_size(size, BufferShmemSize());
+	size = add_size(size, LockShmemSize());
+	size = add_size(size, PredicateLockShmemSize());
+	size = add_size(size, ProcGlobalShmemSize());
+	size = add_size(size, XLOGShmemSize());
+	size = add_size(size, CLOGShmemSize());
+	size = add_size(size, CommitTsShmemSize());
+	size = add_size(size, SUBTRANSShmemSize());
+	size = add_size(size, TwoPhaseShmemSize());
+	size = add_size(size, BackgroundWorkerShmemSize());
+	size = add_size(size, MultiXactShmemSize());
+	size = add_size(size, LWLockShmemSize());
+	size = add_size(size, ProcArrayShmemSize());
+	size = add_size(size, BackendStatusShmemSize());
+	size = add_size(size, SInvalShmemSize());
+	size = add_size(size, PMSignalShmemSize());
+	size = add_size(size, ProcSignalShmemSize());
+	size = add_size(size, CheckpointerShmemSize());
+	size = add_size(size, AutoVacuumShmemSize());
+	size = add_size(size, ReplicationSlotsShmemSize());
+	size = add_size(size, ReplicationOriginShmemSize());
+	size = add_size(size, WalSndShmemSize());
+	size = add_size(size, WalRcvShmemSize());
+	size = add_size(size, PgArchShmemSize());
+	size = add_size(size, ApplyLauncherShmemSize());
+	size = add_size(size, SnapMgrShmemSize());
+	size = add_size(size, BTreeShmemSize());
+	size = add_size(size, SyncScanShmemSize());
+	size = add_size(size, AsyncShmemSize());
+#ifdef EXEC_BACKEND
+	size = add_size(size, ShmemBackendArraySize());
+#endif
+
+	/* freeze the addin request size and include it */
+	addin_request_allowed = false;
+	size = add_size(size, total_addin_request);
+
+	/* might as well round it off to a multiple of a typical page size */
+	size = add_size(size, 8192 - (size % 8192));
+
+	return size;
+}
 
 /*
  * CreateSharedMemoryAndSemaphores
@@ -102,65 +183,8 @@ CreateSharedMemoryAndSemaphores(void)
 		Size		size;
 		int			numSemas;
 
-		/* Compute number of semaphores we'll need */
-		numSemas = ProcGlobalSemas();
-		numSemas += SpinlockSemas();
-
-		/*
-		 * Size of the Postgres shared-memory block is estimated via
-		 * moderately-accurate estimates for the big hogs, plus 100K for the
-		 * stuff that's too small to bother with estimating.
-		 *
-		 * We take some care during this phase to ensure that the total size
-		 * request doesn't overflow size_t.  If this gets through, we don't
-		 * need to be so careful during the actual allocation phase.
-		 */
-		size = 100000;
-		size = add_size(size, PGSemaphoreShmemSize(numSemas));
-		size = add_size(size, SpinlockSemaSize());
-		size = add_size(size, hash_estimate_size(SHMEM_INDEX_SIZE,
-												 sizeof(ShmemIndexEnt)));
-		size = add_size(size, dsm_estimate_size());
-		size = add_size(size, BufferShmemSize());
-		size = add_size(size, LockShmemSize());
-		size = add_size(size, PredicateLockShmemSize());
-		size = add_size(size, ProcGlobalShmemSize());
-		size = add_size(size, XLOGShmemSize());
-		size = add_size(size, CLOGShmemSize());
-		size = add_size(size, CommitTsShmemSize());
-		size = add_size(size, SUBTRANSShmemSize());
-		size = add_size(size, TwoPhaseShmemSize());
-		size = add_size(size, BackgroundWorkerShmemSize());
-		size = add_size(size, MultiXactShmemSize());
-		size = add_size(size, LWLockShmemSize());
-		size = add_size(size, ProcArrayShmemSize());
-		size = add_size(size, BackendStatusShmemSize());
-		size = add_size(size, SInvalShmemSize());
-		size = add_size(size, PMSignalShmemSize());
-		size = add_size(size, ProcSignalShmemSize());
-		size = add_size(size, CheckpointerShmemSize());
-		size = add_size(size, AutoVacuumShmemSize());
-		size = add_size(size, ReplicationSlotsShmemSize());
-		size = add_size(size, ReplicationOriginShmemSize());
-		size = add_size(size, WalSndShmemSize());
-		size = add_size(size, WalRcvShmemSize());
-		size = add_size(size, PgArchShmemSize());
-		size = add_size(size, ApplyLauncherShmemSize());
-		size = add_size(size, SnapMgrShmemSize());
-		size = add_size(size, BTreeShmemSize());
-		size = add_size(size, SyncScanShmemSize());
-		size = add_size(size, AsyncShmemSize());
-#ifdef EXEC_BACKEND
-		size = add_size(size, ShmemBackendArraySize());
-#endif
-
-		/* freeze the addin request size and include it */
-		addin_request_allowed = false;
-		size = add_size(size, total_addin_request);
-
-		/* might as well round it off to a multiple of a typical page size */
-		size = add_size(size, 8192 - (size % 8192));
-
+		/* Compute the size of the shared-memory block */
+		size = CalculateShmemSize(&numSemas);
 		elog(DEBUG3, "invoking IpcMemoryCreate(size=%zu)", size);
 
 		/*
diff --git a/src/backend/utils/misc/guc.c b/src/backend/utils/misc/guc.c
index 467b0fd6fe..872bdc1c8e 100644
--- a/src/backend/utils/misc/guc.c
+++ b/src/backend/utils/misc/guc.c
@@ -620,6 +620,8 @@ char	   *pgstat_temp_directory;
 
 char	   *application_name;
 
+int			shmem_size_mb;
+
 int			tcp_keepalives_idle;
 int			tcp_keepalives_interval;
 int			tcp_keepalives_count;
@@ -2337,6 +2339,17 @@ static struct config_int ConfigureNamesInt[] =
 		NULL, NULL, NULL
 	},
 
+	{
+		{"shared_memory_size", PGC_INTERNAL, RESOURCES_MEM,
+			gettext_noop("Shows the amount of shared memory allocated by the server (rounded up to the nearest MB)."),
+			NULL,
+			GUC_NOT_IN_SAMPLE | GUC_DISALLOW_IN_FILE | GUC_UNIT_MB
+		},
+		&shmem_size_mb,
+		0, 0, INT_MAX,
+		NULL, NULL, NULL
+	},
+
 	{
 		{"temp_buffers", PGC_USERSET, RESOURCES_MEM,
 			gettext_noop("Sets the maximum number of temporary buffers used by each session."),
-- 
2.33.0

---

From: "Bossart, Nathan" <[email-masked]>
Date: Tue, 31 Aug 2021 05:37:52 +0000

> good either.  It would make sense to just have that in a small wrapper

I moved the GUC calculation to ipci.c, adjusted the docs, and added a
huge_pages_required GUC.  It's still a little rough around the edges,
and I haven't tested it on Windows, but this seems like the direction
the patch is headed.

Nathan

---

From: Michael Paquier <[email-masked]>
Date: Wed, 1 Sep 2021 15:53:52 +0900

> and I haven't tested it on Windows, but this seems like the direction

Hmm.  I am not sure about the addition of huge_pages_required, knowing
that we would have shared_memory_size.  I'd rather let the calculation
part to the user with a scan of /proc/meminfo.

+#elif defined(WIN32)
+   hp_size = GetLargePageMinimum();
+#endif
+
+#if defined(MAP_HUGETLB) || defined(WIN32)
+   hp_required = (size_b / hp_size) + 1;
As of [1], there is the following description:
"If the processor does not support large pages, the return value is
zero."
So there is a problem here.

[1]: https://docs.microsoft.com/en-us/windows/win32/api/memoryapi/nf-memoryapi-getlargepageminimum
--
Michael

---

From: "Bossart, Nathan" <[email-masked]>
Date: Wed, 1 Sep 2021 18:28:21 +0000

> part to the user with a scan of /proc/meminfo.

I included this based on some feedback from Andres upthread [0].  I
went ahead and split the patch set into 3 pieces in case we end up
leaving it out.

> So there is a problem here.

I've fixed this in v4.

Nathan

[0] https://postgr.es/m/20210827193813.oqo5lamvyzahs35o%40alap3.anarazel.de

---

From: Michael Paquier <[email-masked]>
Date: Thu, 2 Sep 2021 16:50:52 +0900

> went ahead and split the patch set into 3 pieces in case we end up

Thanks.  Anyway, we don't really need huge_pages_required on Windows,
do we?  The following docs of Windows tell what do to when using large
pages:
https://docs.microsoft.com/en-us/windows/win32/memory/large-page-support

The backend code does that as in PGSharedMemoryCreate(), now that I
look at it.  And there is no way to change the minimum large page size
there as far as I can see because that's decided by the processor, no?
There is a case for shared_memory_size on Windows to be able to adjust
the sizing of the memory of the host, though.

>> So there is a problem here.

At the end it would be nice to not finish with two GUCs.  Both depend
on the reordering of the actions done by the postmaster, so I'd be
curious to hear the thoughts of others on this particular point.
--
Michael

---

From: "Bossart, Nathan" <[email-masked]>
Date: Thu, 2 Sep 2021 16:46:56 +0000

> the sizing of the memory of the host, though.

Yeah, huge_pages_required might not serve much purpose for Windows.
We could always set it to -1 for Windows if it seems like it'll do
more harm than good.

> curious to hear the thoughts of others on this particular point.

Of course.  It'd be great to hear others' thoughts on this stuff.

Nathan

---

From: Michael Paquier <[email-masked]>
Date: Fri, 3 Sep 2021 10:45:33 +0900

> We could always set it to -1 for Windows if it seems like it'll do

I'd be fine with this setup on environments where there is no need for
it.

> Of course.  It'd be great to hear others' thoughts on this stuff.

Just to be clear here, the ordering of HEAD is that for the
postmaster:
- Load configuration.
- Handle -C config_param.
- checkDataDir(), to check permissions of the data dir, etc.
- checkControlFile(), to see if the control file exists.
- Switch to data directory as work dir.
- Lock file creation.
- Initial read of the control file (where the GUC data_checksums is
set).
- Register apply launcher
- shared_preload_libraries

With 0002, we have that:
- Load configuration.
- checkDataDir(), to check permissions of the data dir, etc.
- checkControlFile(), to see if the control file exists.
- Switch to data directory as work dir.
- Register apply launcher
- shared_preload_libraries
- Calculate the shmem GUCs (new step)
- Handle -C config_param.
- Lock file creation.
- Initial read of the control file (where the GUC data_checksums is
set).

One thing that would be incorrect upon more review is that we'd still
have data_checksums wrong with -C, meaning that the initial read of
the control file should be moved further up, and getting the control
file checks done before registering workers would be better.  Keeping
the lock file at the end would be fine AFAIK, but should we worry
about the interactions with _PG_init() here?

0001, that refactors the calculation of the shmem size into a
different routine, is fine as-is, so I'd like to move on and apply
it.
--
Michael

---

From: Kyotaro Horiguchi <[email-masked]>
Date: Fri, 03 Sep 2021 14:12:06 +0900 (JST)

At Thu, 2 Sep 2021 16:46:56 +0000, "Bossart, Nathan" <[email-masked]> wrote in 
> We could always set it to -1 for Windows if it seems like it'll do

I agreed to this.

> Of course.  It'd be great to hear others' thoughts on this stuff.

Honestly, I would be satisfied if the following error message
contained required huge pages.

FATAL:  could not map anonymous shared memory: Cannot allocate memory
HINT:  This error usually means that PostgreSQL's request for a shared memory segment exceeded available memory, swap space, or huge pages. To reduce the request size (currently 148897792 bytes), reduce PostgreSQL's shared memory usage, perhaps by reducing shared_buffers or max_connections.

Or emit a different message if huge_pages=on.

FATAL: could not map anonymous shared memory from huge pages
HINT:  This usually means that PostgreSQL's request for huge pages more than available. The required 2048kB huge pages for the required memory size (currently 148897792 bytes) is 71 pages.

Returning to this feature, even if I am informed that via GUC, I won't
add memory by looking shared_memory_size.  Anyway since shard_buffers
occupies almost all portion of shared memory allocated to postgres, we
are not supposed to need such a precise adjustment of the required
size of shared memory.  On the other hand available number of huge
pages is configurable and we need to set it as required.  On the other
hand, it might seem to me a bit strange that there's only
huge_page_required and not shared_memory_size in the view of
comprehensiveness or completeness.  So my feeling at this point is "I
need only huge_pages_required but might want shared_memory_size just
for completeness".

By the way I noticed that postgres -C huge_page_size shows 0, which I
think should have the number used for the calculation if we show
huge_page_required.

I noticed that postgres -C shared_memory_size showed 137 (= 144703488)
whereas the error message above showed 148897792 bytes (142MB). So it
seems that something is forgotten while calculating
shared_memory_size.  As the consequence, launching postgres setting
huge_pages_required (69 pages) as vm.nr_hugepages ended up in the
"could not map anonymous shared memory" error.

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center

---

From: "Bossart, Nathan" <[email-masked]>
Date: Fri, 3 Sep 2021 17:36:43 +0000

> I'd be fine with this setup on environments where there is no need for

I did this in v5.

> about the interactions with _PG_init() here?

I think we can avoid so much reordering by moving the -C handling
instead.  That should also fix things like data_checksums.  I split
the reordering part out into its own patch in v5.

You bring up an interesting point about _PG_init().  Presently, you
can safely assume that the data directory is locked during _PG_init(),
so there's no need to worry about breaking something on a running
server.  I don't know how important this is.  Most _PG_init()
functions that I've seen will define some GUCs, request some shared
memory, register some background workers, and/or install some hooks.
Those all seem like safe things to do, but I wouldn't be at all
surprised to hear examples to the contrary.  In any case, it looks
like the current ordering of these two steps has been there for 15+
years.

If this is a concern, one option would be to disallow running "-C
shared_memory_size" on running servers.  That would have to extend to
GUCs like data_checksums and huge_pages_required, too.

> different routine, is fine as-is, so I'd like to move on and apply

Sounds good to me.

Nathan

---

From: "Bossart, Nathan" <[email-masked]>
Date: Fri, 3 Sep 2021 17:46:05 +0000

> think should have the number used for the calculation if we show

I would agree with this if huge_page_size was a runtime-computed GUC,
but since it's intended for users to explicitly request the huge page
size, it might be slightly confusing.  Perhaps another option would be
to create a new GUC for this.  Or maybe it's enough to note that the
value will be changed from 0 at runtime if huge pages are supported.
In any case, it might be best to handle this separately.

> "could not map anonymous shared memory" error.

Hm.  I'm not seeing this with the v5 patch set, so maybe I
inadvertently fixed it already.  Can you check this again with v5?

Nathan

---

From: Andres Freund <[email-masked]>
Date: Fri, 3 Sep 2021 13:20:59 -0700

Hi,

> part to the user with a scan of /proc/meminfo.

-1. We can easily do better, what do we gain by making the user do this stuff?
Especially because the right value also depends on huge_page_size.

Greetings,

Andres Freund

---

From: Michael Paquier <[email-masked]>
Date: Mon, 6 Sep 2021 11:27:35 +0900

> like the current ordering of these two steps has been there for 15+

Yeah.  What you are describing here matches what I have seen in the
past and what we do in core for _PG_init().  Now extensions developers
could do more fancy things, like dropping things on-disk to check the
load state, for whatever reasons.  And things could break in such
cases.  Perhaps people should not do that, but it is no fun either to
break code that has been working for years even if that's just a major
upgrade.

+        * We skip this step if we are just going to print a GUC's value and exit
+        * a few steps down.
         */
-       CreateDataDirLockFile(true);
+       if (output_config_variable == NULL)
+               CreateDataDirLockFile(true);

Anyway, 0002 gives me shivers.

> GUCs like data_checksums and huge_pages_required, too.

Just noting this bit from 0003 that would break without 0002:
-$ <userinput>pmap 4170 | awk '/rw-s/ &amp;&amp; /zero/ {print $2}'</userinput>
-6490428K
+$ <userinput>postgres -D $PGDATA -C shared_memory_size</userinput>
+6339

>> different routine, is fine as-is, so I'd like to move on and apply

Applied this one.

Without concluding on 0002 yet, another thing that we could do is to
just add the GUCs.  These sound rather useful on their own (mixed
feelings about huge_pages_required but I can see why it is useful to
avoid the setup steps and the backend already grabs this information),
particularly when it comes to cloned setups that share a lot of
properties.
--
Michael

---

From: "Bossart, Nathan" <[email-masked]>
Date: Mon, 6 Sep 2021 04:21:51 +0000

>>> different routine, is fine as-is, so I'd like to move on and apply

Thanks!

> particularly when it comes to cloned setups that share a lot of

I think this is a good starting point, but I'd like to follow up on
making them visible without completely starting the server.  The main
purpose for adding these GUCs is to be able to set up huge pages
before server startup.  Disallowing "-C huge_pages_required" on a
running server to enable this use-case seems like a modest tradeoff.

Anyway, I'll restructure the remaining patches to add the GUCs first
and then address the 0002 business separately.

Nathan

---

From: "Bossart, Nathan" <[email-masked]>
Date: Mon, 6 Sep 2021 23:55:42 +0000

> and then address the 0002 business separately.

Attached is a new patch set.  The first two patches just add the new
GUCs, and the third is an attempt at providing useful values for those
GUCs via -C.

Nathan

---

From: Michael Paquier <[email-masked]>
Date: Tue, 7 Sep 2021 13:00:08 +0900

> GUCs, and the third is an attempt at providing useful values for those

+   sprintf(buf, "%lu MB", size_mb);
+   SetConfigOption("shared_memory_size", buf, PGC_INTERNAL, PGC_S_OVERRIDE);
One small-ish comment about 0002: there is no need to add the unit
into the buffer set as GUC_UNIT_MB would take care of that.  The patch
looks fine.

+#ifndef WIN32
+#include <sys/mman.h>
+#endif
So, this is needed in ipci.c to check for MAP_HUGETLB.  I am not much
a fan of moving around platform-specific checks when these have
remained local to each shmem implementation.  Could it be cleaner to
add GetHugePageSize() to win32_shmem.c and make it always declared in
the SysV implementation?
--
Michael

---

From: Kyotaro Horiguchi <[email-masked]>
Date: Tue, 07 Sep 2021 15:24:21 +0900 (JST)

At Fri, 3 Sep 2021 17:46:05 +0000, "Bossart, Nathan" <[email-masked]> wrote in 
> In any case, it might be best to handle this separately.

(Sorry, I was confused, but) yeah, agreed.

> inadvertently fixed it already.  Can you check this again with v5?

Thanks! I confirmed that the numbers match with v5.

regards.

---

From: "Bossart, Nathan" <[email-masked]>
Date: Tue, 7 Sep 2021 17:08:43 +0000

> into the buffer set as GUC_UNIT_MB would take care of that.  The patch

I fixed this in v7.

> add GetHugePageSize() to win32_shmem.c and make it always declared in

I don't know if it's really all that much cleaner, but I did it this
way in v7.  IMO it's a little weird that GetHugePageSize() doesn't
return the value from GetLargePageMinimum(), but that's what we'd need
to do to avoid setting huge_pages_required for Windows without any
platform-specific checks.

Nathan

---

From: "Bossart, Nathan" <[email-masked]>
Date: Tue, 7 Sep 2021 17:09:08 +0000

> Thanks! I confirmed that the numbers match with v5.

Thanks for confirming.

Nathan

---

From: Michael Paquier <[email-masked]>
Date: Wed, 8 Sep 2021 12:50:08 +0900

>> into the buffer set as GUC_UNIT_MB would take care of that.  The patch

Switched the variable name to shared_memory_size_mb for easier
grepping, moved it to a more correct location with the other read-only
GUCS, and applied 0002.  Well, 0001 here.

> platform-specific checks.

Thanks.  Keeping MAP_HUGETLB within the SysV portions is an
improvement IMO.  That's subject to one's taste, perhaps.

After sleeping on it, I'd be fine to live with the logic based on the
new GUC flag called GUC_RUNTIME_COMPUTED to control if a parameter can
be looked at either an earlier or a later stage of the startup
sequences, with the later stage meaning that such parameters cannot be
checked if a server is running.  This case was originally broken
anyway, so it does not make it worse, just better.

+        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="guc-huge-pages-required"/>).
Perhaps we should add a couple of extra parameters here, like
shared_memory_size, and perhaps wal_segment_size?  The list does not
have to be complete, just meaningful enough.
--
Michael

---

From: Fujii Masao <[email-masked]>
Date: Wed, 8 Sep 2021 16:10:41 +0900

> GUCS, and applied 0002.  Well, 0001 here.

Thanks for adding useful feature!

When reading the applied code, I found the category of shared_memory_size
is RESOURCES_MEM. Why? This seems right because the parameter is related
to memory resource. But since its context is PGC_INTERNAL, PRESET_OPTIONS
is more proper as the category? BTW, the category of any other
PGC_INTERNAL parameters seems to be PRESET_OPTIONS.

Regards,

-- 
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION

---

From: "Bossart, Nathan" <[email-masked]>
Date: Wed, 8 Sep 2021 17:48:16 +0000

> GUCS, and applied 0002.  Well, 0001 here.

Thanks!  And thanks for cleaning up the small mistake in aa37a43.

> have to be complete, just meaningful enough.

Good idea.

Nathan

---

From: "Bossart, Nathan" <[email-masked]>
Date: Wed, 8 Sep 2021 17:52:33 +0000

> Thanks for adding useful feature!

:)

> PGC_INTERNAL parameters seems to be PRESET_OPTIONS.

Yeah, I did wonder about this.  We're even listing it in the "Preset
Options" section in the docs.  I updated this in the new patch set,
which is attached.

Nathan

---

From: Michael Paquier <[email-masked]>
Date: Thu, 9 Sep 2021 10:09:52 +0900

> PGC_INTERNAL parameters seems to be PRESET_OPTIONS.

Yes, that's an oversight from me.  I was looking at that yesterday,
noticed some exceptions in the GUC list with things not allowed in
files and just concluded that RESOURCES_MEM should be fine, but the
docs tell a different story.  Thanks, fixed.
--
Michael

---

From: Michael Paquier <[email-masked]>
Date: Thu, 9 Sep 2021 13:19:14 +0900

> Options" section in the docs.  I updated this in the new patch set,

I broke that again, so rebased as v9 attached.

FWIW, I don't have an environment at hand these days to test properly
0001, so this will have to wait a bit.  I really like the approach
taken by 0002, and it is independent of the other patch while
extending support for postgres -c to provide the correct runtime
values.  So let's wrap this part first.  No need to send a reorganized
patch set.
--
Michael

---

From: Michael Paquier <[email-masked]>
Date: Thu, 9 Sep 2021 13:23:10 +0900

> I broke that again, so rebased as v9 attached.

Well.
--
Michael
From 09b1f5a5e0e88a6d84bfc2f8a1bc410e21fb3775 Mon Sep 17 00:00:00 2001
From: Michael Paquier <[email-masked]>
Date: Thu, 9 Sep 2021 13:10:24 +0900
Subject: [PATCH v9 1/2] Introduce huge_pages_required GUC.

This runtime-computed GUC shows the number of huge pages required
for the server's main shared memory area.
---
 src/include/storage/pg_shmem.h |  1 +
 src/backend/port/sysv_shmem.c  | 16 +++++++++++-----
 src/backend/port/win32_shmem.c | 14 ++++++++++++++
 src/backend/storage/ipc/ipci.c | 14 ++++++++++++++
 src/backend/utils/misc/guc.c   | 12 ++++++++++++
 doc/src/sgml/config.sgml       | 21 +++++++++++++++++++++
 6 files changed, 73 insertions(+), 5 deletions(-)

diff --git a/src/include/storage/pg_shmem.h b/src/include/storage/pg_shmem.h
index 059df1b72c..518eb86065 100644
--- a/src/include/storage/pg_shmem.h
+++ b/src/include/storage/pg_shmem.h
@@ -87,5 +87,6 @@ extern PGShmemHeader *PGSharedMemoryCreate(Size size,
 										   PGShmemHeader **shim);
 extern bool PGSharedMemoryIsInUse(unsigned long id1, unsigned long id2);
 extern void PGSharedMemoryDetach(void);
+extern void GetHugePageSize(Size *hugepagesize, int *mmap_flags);
 
 #endif							/* PG_SHMEM_H */
diff --git a/src/backend/port/sysv_shmem.c b/src/backend/port/sysv_shmem.c
index 9de96edf6a..125e2d47ec 100644
--- a/src/backend/port/sysv_shmem.c
+++ b/src/backend/port/sysv_shmem.c
@@ -456,8 +456,6 @@ PGSharedMemoryAttach(IpcMemoryId shmId,
 	return shmStat.shm_nattch == 0 ? SHMSTATE_UNATTACHED : SHMSTATE_ATTACHED;
 }
 
-#ifdef MAP_HUGETLB
-
 /*
  * Identify the huge page size to use, and compute the related mmap flags.
  *
@@ -476,11 +474,19 @@ PGSharedMemoryAttach(IpcMemoryId shmId,
  * such as increasing shared_buffers to absorb the extra space.
  *
  * Returns the (real, assumed or config provided) page size into *hugepagesize,
- * and the hugepage-related mmap flags to use into *mmap_flags.
+ * and the hugepage-related mmap flags to use into *mmap_flags.  If huge pages
+ * is not supported, *hugepagesize and *mmap_flags will be set to 0.
  */
-static void
+void
 GetHugePageSize(Size *hugepagesize, int *mmap_flags)
 {
+#ifndef MAP_HUGETLB
+
+	*hugepagesize = 0;
+	*mmap_flags = 0;
+
+#else
+
 	Size		default_hugepagesize = 0;
 
 	/*
@@ -553,9 +559,9 @@ GetHugePageSize(Size *hugepagesize, int *mmap_flags)
 		*mmap_flags |= (shift & MAP_HUGE_MASK) << MAP_HUGE_SHIFT;
 	}
 #endif
-}
 
 #endif							/* MAP_HUGETLB */
+}
 
 /*
  * Creates an anonymous mmap()ed shared memory segment.
diff --git a/src/backend/port/win32_shmem.c b/src/backend/port/win32_shmem.c
index d7a71992d8..90de2ab4e1 100644
--- a/src/backend/port/win32_shmem.c
+++ b/src/backend/port/win32_shmem.c
@@ -605,3 +605,17 @@ pgwin32_ReserveSharedMemoryRegion(HANDLE hChild)
 
 	return true;
 }
+
+/*
+ * This function is provided for consistency with sysv_shmem.c and does not
+ * provide any useful information for Windows.  To obtain the large page size,
+ * use GetLargePageMinimum() instead.
+ *
+ * This always sets *hugepagesize and *mmap_flags to 0.
+ */
+void
+GetHugePageSize(Size *hugepagesize, int *mmap_flags)
+{
+	*hugepagesize = 0;
+	*mmap_flags = 0;
+}
diff --git a/src/backend/storage/ipc/ipci.c b/src/backend/storage/ipc/ipci.c
index 13f3926ff6..91490653cf 100644
--- a/src/backend/storage/ipc/ipci.c
+++ b/src/backend/storage/ipc/ipci.c
@@ -326,6 +326,9 @@ InitializeShmemGUCs(void)
 	char		buf[64];
 	Size		size_b;
 	Size		size_mb;
+	Size		hp_size;
+	Size		hp_required;
+	int			unused;
 
 	/*
 	 * Calculate the shared memory size and round up to the nearest megabyte.
@@ -334,4 +337,15 @@ InitializeShmemGUCs(void)
 	size_mb = add_size(size_b, (1024 * 1024) - 1) / (1024 * 1024);
 	sprintf(buf, "%zu", size_mb);
 	SetConfigOption("shared_memory_size", buf, PGC_INTERNAL, PGC_S_OVERRIDE);
+
+	/*
+	 * Calculate the number of huge pages required.
+	 */
+	GetHugePageSize(&hp_size, &unused);
+	if (hp_size != 0)
+	{
+		hp_required = (size_b / hp_size) + 1;
+		sprintf(buf, "%zu", hp_required);
+		SetConfigOption("huge_pages_required", buf, PGC_INTERNAL, PGC_S_OVERRIDE);
+	}
 }
diff --git a/src/backend/utils/misc/guc.c b/src/backend/utils/misc/guc.c
index fd4ca83be1..8edfd07340 100644
--- a/src/backend/utils/misc/guc.c
+++ b/src/backend/utils/misc/guc.c
@@ -665,6 +665,7 @@ static int	max_identifier_length;
 static int	block_size;
 static int	segment_size;
 static int	shared_memory_size_mb;
+static int	huge_pages_required;
 static int	wal_block_size;
 static bool data_checksums;
 static bool integer_datetimes;
@@ -2224,6 +2225,17 @@ static struct config_int ConfigureNamesInt[] =
 		NULL, NULL, NULL
 	},
 
+	{
+		{"huge_pages_required", PGC_INTERNAL, PRESET_OPTIONS,
+			gettext_noop("Shows the number of huge pages needed for the main shared memory area."),
+			gettext_noop("-1 indicates that the value could not be determined."),
+			GUC_NOT_IN_SAMPLE | GUC_DISALLOW_IN_FILE
+		},
+		&huge_pages_required,
+		-1, -1, INT_MAX,
+		NULL, NULL, NULL
+	},
+
 	{
 		/* This is PGC_SUSET to prevent hiding from log_lock_waits. */
 		{"deadlock_timeout", PGC_SUSET, LOCK_MANAGEMENT,
diff --git a/doc/src/sgml/config.sgml b/doc/src/sgml/config.sgml
index ef0e2a7746..b27d8aff15 100644
--- a/doc/src/sgml/config.sgml
+++ b/doc/src/sgml/config.sgml
@@ -10101,6 +10101,27 @@ dynamic_library_path = 'C:\tools\postgresql;H:\my_project\lib;$libdir'
       </listitem>
      </varlistentry>
 
+     <varlistentry id="guc-huge-pages-required" xreflabel="huge_pages_required">
+      <term><varname>huge_pages_required</varname> (<type>integer</type>)
+      <indexterm>
+       <primary><varname>huge_pages_required</varname> configuration parameter</primary>
+      </indexterm>
+      </term>
+      <listitem>
+       <para>
+        Reports the number of huge pages that are required for the main
+        shared memory area based on the specified
+        <xref linkend="guc-huge-page-size"/>.  If huge pages are not supported,
+        this will be <literal>-1</literal>.
+       </para>
+       <para>
+        This setting is supported only on Linux.  It is always set to
+        <literal>-1</literal> on other platforms.  For more details about using
+        huge pages on Linux, see <xref linkend="linux-huge-pages"/>.
+       </para>
+      </listitem>
+     </varlistentry>
+
      <varlistentry id="guc-integer-datetimes" xreflabel="integer_datetimes">
       <term><varname>integer_datetimes</varname> (<type>boolean</type>)
       <indexterm>
-- 
2.33.0

From f4763273a7f4649d4c50699b590c0b4b8077cb95 Mon Sep 17 00:00:00 2001
From: Michael Paquier <[email-masked]>
Date: Thu, 9 Sep 2021 13:10:38 +0900
Subject: [PATCH v9 2/2] Provide useful values for 'postgres -C' with
 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        | 10 +++---
 doc/src/sgml/ref/postgres-ref.sgml  | 12 +++++--
 doc/src/sgml/runtime.sgml           | 33 +++++++------------
 5 files changed, 75 insertions(+), 36 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
 
 #define GUC_EXPLAIN			  0x100000	/* include in explain */
 
+/*
+ * 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)

diff --git a/src/backend/postmaster/postmaster.c b/src/backend/postmaster/postmaster.c
index b2fe420c3c..f601b9e5d1 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 != 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 = GetConfigOption(output_config_variable,
-												 false, false);
+		int flags = GetConfigOptionFlags(output_config_variable, true);
 
-		puts(config_val ? config_val : "");
-		ExitPostmaster(0);
+		if ((flags & GUC_RUNTIME_COMPUTED) == 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 = GetConfigOption(output_config_variable,
+													 false, false);
+
+			puts(config_val ? config_val : "");
+			ExitPostmaster(0);
+		}
 	}
 
 	/* Verify that DataDir looks reasonable */
@@ -1033,6 +1049,26 @@ PostmasterMain(int argc, char *argv[])
 	 */
 	InitializeShmemGUCs();
 
+	/*
+	 * 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 != NULL)
+	{
+		const char *config_val = 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 8edfd07340..50e60b270b 100644
--- a/src/backend/utils/misc/guc.c
+++ b/src/backend/utils/misc/guc.c
@@ -1984,7 +1984,7 @@ static struct config_bool ConfigureNamesBool[] =
 		{"data_checksums", PGC_INTERNAL, PRESET_OPTIONS,
 			gettext_noop("Shows whether data checksums are turned on for this cluster."),
 			NULL,
-			GUC_NOT_IN_SAMPLE | GUC_DISALLOW_IN_FILE
+			GUC_NOT_IN_SAMPLE | GUC_DISALLOW_IN_FILE | GUC_RUNTIME_COMPUTED
 		},
 		&data_checksums,
 		false,
@@ -2229,7 +2229,7 @@ static struct config_int ConfigureNamesInt[] =
 		{"huge_pages_required", PGC_INTERNAL, PRESET_OPTIONS,
 			gettext_noop("Shows the number of huge pages needed for the main shared memory area."),
 			gettext_noop("-1 indicates that the value could not be determined."),
-			GUC_NOT_IN_SAMPLE | GUC_DISALLOW_IN_FILE
+			GUC_NOT_IN_SAMPLE | GUC_DISALLOW_IN_FILE | GUC_RUNTIME_COMPUTED
 		},
 		&huge_pages_required,
 		-1, -1, INT_MAX,
@@ -2354,7 +2354,7 @@ static struct config_int ConfigureNamesInt[] =
 		{"shared_memory_size", PGC_INTERNAL, PRESET_OPTIONS,
 			gettext_noop("Shows the size of the server's main shared memory area (rounded 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_COMPUTED
 		},
 		&shared_memory_size_mb,
 		0, 0, INT_MAX,
@@ -2419,7 +2419,7 @@ static struct config_int ConfigureNamesInt[] =
 						 "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,
@@ -3243,7 +3243,7 @@ static struct config_int ConfigureNamesInt[] =
 		{"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/doc/src/sgml/ref/postgres-ref.sgml b/doc/src/sgml/ref/postgres-ref.sgml
index 4aaa7abe1a..89cc3cdb4e 100644
--- a/doc/src/sgml/ref/postgres-ref.sgml
+++ b/doc/src/sgml/ref/postgres-ref.sgml
@@ -133,13 +133,21 @@ 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>
 
+       <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="guc-huge-pages-required"/>,
+        <xref linkend="guc-shared-memory-size"/>, and
+        <xref linkend="guc-wal-segment-size"/>).
+       </para>
+
        <para>
         This option is meant for other programs that interact with a server
         instance, such as <xref linkend="app-pg-ctl"/>, to query configuration
diff --git a/doc/src/sgml/runtime.sgml b/doc/src/sgml/runtime.sgml
index f1cbc1d9e9..d955639900 100644
--- a/doc/src/sgml/runtime.sgml
+++ b/doc/src/sgml/runtime.sgml
@@ -1442,32 +1442,21 @@ export PG_OOM_ADJUST_VALUE=0
     with <varname>CONFIG_HUGETLBFS=y</varname> and
     <varname>CONFIG_HUGETLB_PAGE=y</varname>. You will also have to configure
     the operating system to provide enough huge pages of the desired size.
-    To estimate the number of huge pages needed, start
-    <productname>PostgreSQL</productname> without huge pages enabled and check
-    the postmaster's anonymous shared memory segment size, as well as the
-    system's default and supported huge page sizes, using the
-    <filename>/proc</filename> and <filename>/sys</filename> file systems.
-    This might look like:
+    To estimate the number of huge pages needed, use the
+    <command>postgres</command> command to see the value of
+    <xref linkend="guc-huge-pages-required"/>.  This might look like:
 <programlisting>
-$ <userinput>head -1 $PGDATA/postmaster.pid</userinput>
-4170
-$ <userinput>pmap 4170 | awk '/rw-s/ &amp;&amp; /zero/ {print $2}'</userinput>
-6490428K
-$ <userinput>grep ^Hugepagesize /proc/meminfo</userinput>
-Hugepagesize:       2048 kB
-$ <userinput>ls /sys/kernel/mm/hugepages</userinput>
-hugepages-1048576kB  hugepages-2048kB
+$ <userinput>postgres -D $PGDATA -C huge_pages_required</userinput>
+3170
 </programlisting>
 
-     In this example the default is 2MB, but you can also explicitly request
-     either 2MB or 1GB with <xref linkend="guc-huge-page-size"/>.
+     Note that you can explicitly request either 2MB or 1GB huge pages with
+     <xref linkend="guc-huge-page-size"/>.
 
-     Assuming <literal>2MB</literal> huge pages,
-     <literal>6490428</literal> / <literal>2048</literal> gives approximately
-     <literal>3169.154</literal>, so in this example we need at
-     least <literal>3170</literal> huge pages.  A larger setting would be
-     appropriate if other programs on the machine also need huge pages.
-     We can set this with:
+     While we need at least <literal>3170</literal> huge pages in this
+     example, a larger setting would be appropriate if other programs on
+     the machine also need huge pages.  We can allocate the huge pages
+     with:
 <programlisting>
 # <userinput>sysctl -w vm.nr_hugepages=3170</userinput>
 </programlisting>
-- 
2.33.0

---

From: "Bossart, Nathan" <[email-masked]>
Date: Thu, 9 Sep 2021 21:53:22 +0000

> values.  So let's wrap this part first.  No need to send a reorganized

Sounds good.

For 0001, the biggest thing on my mind at the moment is the name of
the GUC.  "huge_pages_required" feels kind of ambiguous.  From the
name alone, it could mean either "the number of huge pages required"
or "huge pages are required for the server to run."  Also, the number
of huge pages required is not actually required if you don't want to
run the server with huge pages.  I think it might be clearer to
somehow indicate that the value is essentially the size of the main
shared memory area in terms of the huge page size, but I'm not sure
how to do that concisely.  Perhaps it is enough to just make sure the
description of "huge_pages_required" is detailed enough.

For 0002, I have two small concerns.  My first concern is that it
might be confusing to customers when the runtime GUCs cannot be
returned for a running server.  We have the note in the docs, but if
you're encountering it on the command line, it's not totally clear
what the problem is.

        $ postgres -D . -C log_min_messages
        warning
        $ postgres -D . -C shared_memory_size
        2021-09-09 18:51:21.617 UTC [7924] FATAL:  lock file "postmaster.pid" already exists
        2021-09-09 18:51:21.617 UTC [7924] HINT:  Is another postmaster (PID 7912) running in data directory "/local/home/bossartn/pgdata"?

My other concern is that by default, viewing the runtime-computed GUCs
will also emit a LOG.

        $ postgres -D . -C shared_memory_size
        142
        2021-09-09 18:53:25.194 UTC [8006] LOG:  database system is shut down

Running these commands with log_min_messages=debug5 emits way more
information for the runtime-computed GUCs than for others, but IMO
that is alright.  However, perhaps we should adjust the logging in
0002 to improve the default user experience.  I attached an attempt at
that.

With the attached patch, trying to view a runtime-computed GUC on a
running server will look like this:

        $ postgres -D . -C shared_memory_size
        2021-09-09 21:24:21.552 UTC [6224] FATAL:  lock file "postmaster.pid" already exists
        2021-09-09 21:24:21.552 UTC [6224] DETAIL:  Runtime-computed GUC "shared_memory_size" cannot be viewed on a running server.
        2021-09-09 21:24:21.552 UTC [6224] HINT:  Is another postmaster (PID 3628) running in data directory "/local/home/bossartn/pgdata"?

And viewing a runtime-computed GUC on a server that is shut down will
look like this:

        $ postgres -D . -C shared_memory_size
        142

I'm not tremendously happy with the patch, but I hope that it at least
helps with the discussion.

Nathan

---

From: Michael Paquier <[email-masked]>
Date: Fri, 10 Sep 2021 11:03:05 +0900

> you're encountering it on the command line, it's not totally clear

Yeah, that's true.  There are more unlikely-to-happen errors that
could be triggered and prevent the command to work.  I have never
tried using error_context_stack in a code path as early as that, to be
honest.

> 0002 to improve the default user experience.  I attached an attempt at

Registered bgworkers would generate a DEBUG entry, for one.

> helps with the discussion.

As far as the behavior is documented, I'd be fine with the approach to
keep the code in its simplest shape.  I agree that the message is
confusing, still it is not wrong either as we try to query a run-time
parameter, but we need the lock.
--
Michael

---

From: "Bossart, Nathan" <[email-masked]>
Date: Fri, 10 Sep 2021 02:26:10 +0000

> parameter, but we need the lock.

That seems alright to me.

Nathan

---

From: Robert Haas <[email-masked]>
Date: Fri, 10 Sep 2021 16:01:23 -0400

On Thu, Sep 9, 2021 at 5:53 PM Bossart, Nathan <[email-masked]> wrote:
> run the server with huge pages.

+1 to all of that.

> I think it might be clearer to

shared_memory_size_in_huge_pages? It's kinda long, but a long name
that you can understand without reading the docs is better than a
short one where you can't.

-- 
Robert Haas
EDB: http://www.enterprisedb.com

---

From: "Bossart, Nathan" <[email-masked]>
Date: Fri, 10 Sep 2021 23:43:40 +0000

> short one where you can't.

I think that's an improvement.  The only other idea I have at the
moment is num_huge_pages_required_for_shared_memory.

Nathan

---

From: Robert Haas <[email-masked]>
Date: Mon, 13 Sep 2021 11:49:49 -0400

> moment is num_huge_pages_required_for_shared_memory.

Hmm, that to me sounds like maybe only part of shared memory uses huge
pages and maybe we're just giving you the number required for that
part. I realize that it doesn't work that way but I don't know if
everyone will.

---

From: "Bossart, Nathan" <[email-masked]>
Date: Mon, 13 Sep 2021 18:49:16 +0000

> part. I realize that it doesn't work that way but I don't know if

Yeah, I agree.  What about
huge_pages_needed_for_shared_memory_size or
huge_pages_needed_for_main_shared_memory?  I'm still not stoked about
using "required" or "needed" in the name, as it sounds like huge pages
must be allocated for the server to run, which is only true if
huge_pages=on.  I haven't thought of a better word to use, though.

Nathan

---

From: Robert Haas <[email-masked]>
Date: Mon, 13 Sep 2021 16:20:00 -0400

> huge_pages=on.  I haven't thought of a better word to use, though.

I prefer the first of those to the second. I don't find it
particularly better or worse than my previous suggestion of
shared_memory_size_in_huge_pages.

---

From: Tom Lane <[email-masked]>
Date: Mon, 13 Sep 2021 16:24:14 -0400

"Bossart, Nathan" <[email-masked]> writes:
> huge_pages_needed_for_main_shared_memory?

Seems like "huge_pages_needed_for_shared_memory" would be sufficient.

			regards, tom lane

---

From: Michael Paquier <[email-masked]>
Date: Tue, 14 Sep 2021 09:08:47 +0900

> shared_memory_size_in_huge_pages.

I am not particularly fond of the use "needed" in this context, so I'd
be fine with your suggestion of "shared_memory_size_in_huge_pages.
Some other ideas I could think of:
- shared_memory_size_as_huge_pages
- huge_pages_for_shared_memory_size

Having shared_memory_size in the GUC name is kind of appealing though
in terms of grepping, and one gets the relationship with
shared_memory_size immediately.  If the consensus is
huge_pages_needed_for_shared_memory_size, I won't fight it, but IMO
that's too long.
--
Michael

---

From: "Bossart, Nathan" <[email-masked]>
Date: Tue, 14 Sep 2021 00:30:22 +0000

> Seems like "huge_pages_needed_for_shared_memory" would be sufficient.

I think we are down to either shared_memory_size_in_huge_pages or
huge_pages_needed_for_shared_memory.  Robert's argument against
huge_pages_needed_for_shared_memory was that it might sound like only
part of shared memory uses huge pages and we're only giving the number
required for that.  Speaking of which, isn't that technically true?
For shared_memory_size_in_huge_pages, the intent is to make it sound
like we are providing shared_memory_size in terms of the huge page
size, but I think it could also be interpreted as "the amount of
shared memory that is currently stored in huge pages."

I personally lean towards huge_pages_needed_for_shared_memory because
it feels the most clear and direct to me.  I'm not vehemently opposed
to shared_memory_size_in_huge_pages, though.  I don't think either one
is too misleading.

Nathan

---

From: Kyotaro Horiguchi <[email-masked]>
Date: Tue, 14 Sep 2021 09:49:36 +0900 (JST)

At Tue, 14 Sep 2021 00:30:22 +0000, "Bossart, Nathan" <[email-masked]> wrote in 
> to shared_memory_size_in_huge_pages, though.  I don't think either one

I like 'in' slightly than 'for' in this context. I stand by Michael
that that name looks somewhat too long especially considering that
that name won't be completed on shell command lines, but won't fight
it, too.  On the other hand the full-spelled name can be thought as
one can spell it out from memory easily than a name halfway shortened.

regards.

---

From: "Bossart, Nathan" <[email-masked]>
Date: Tue, 14 Sep 2021 18:00:44 +0000

> one can spell it out from memory easily than a name halfway shortened.

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.

Nathan

---

From: Michael Paquier <[email-masked]>
Date: Wed, 15 Sep 2021 12:05:21 +0900

> shared_memory_size_in_huge_pages unless something changes in the

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
From e24d151efe45e1bc7048ec72ea66097c94f633be Mon Sep 17 00:00:00 2001
From: Michael Paquier <[email-masked]>
Date: Wed, 15 Sep 2021 12:02:32 +0900
Subject: [PATCH v10] Provide useful values for 'postgres -C' with 
 runtime-computed GUCs

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/backend/postmaster/postmaster.c b/src/backend/postmaster/postmaster.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 != 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 = GetConfigOption(output_config_variable,
-												 false, false);
+		int			flags = GetConfigOptionFlags(output_config_variable, true);
 
-		puts(config_val ? config_val : "");
-		ExitPostmaster(0);
+		if ((flags & GUC_RUNTIME_COMPUTED) == 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 = GetConfigOption(output_config_variable,
+													 false, false);
+
+			puts(config_val ? config_val : "");
+			ExitPostmaster(0);
+		}
 	}

+	/*
+	 * 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 != NULL)
+	{
+		const char *config_val = 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[] =
 		{"data_checksums", PGC_INTERNAL, PRESET_OPTIONS,
 			gettext_noop("Shows whether data checksums are turned on for this cluster."),
 			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[] =
 		{"shared_memory_size", PGC_INTERNAL, PRESET_OPTIONS,
 			gettext_noop("Shows the size of the server's main shared memory area (rounded 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_COMPUTED
 		},
 		&shared_memory_size_mb,
 		0, 0, INT_MAX,
@@ -2407,7 +2407,7 @@ static struct config_int ConfigureNamesInt[] =
 						 "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[] =
 		{"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;
 
 use Fcntl qw(:seek);
-use Test::More tests => 63;
+use Test::More tests => 69;

 # 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");
 
+# 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=on 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");
 
+# 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');
 
index 4aaa7abe1a..02142ffab2 100644
@@ -133,13 +133,20 @@ PostgreSQL documentation
 
+       <para>
+        (e.g., <xref linkend="guc-shared-memory-size"/>, and

---

From: "Bossart, Nathan" <[email-masked]>
Date: Wed, 15 Sep 2021 22:31:20 +0000

> now), with some minor tweaks and the tests.

LGTM

nitpick: I think you can remove the comma before the "and" in the list
of examples.

Nathan

---

From: Michael Paquier <[email-masked]>
Date: Thu, 16 Sep 2021 11:41:55 +0900

> nitpick: I think you can remove the comma before the "and" in the list

Fixed that, and applied.  Could you rebase the last patch with the
name suggested for the last GUC, including the docs?  It looks like we
are heading for shared_memory_size_in_huge_pages.
--
Michael

---

From: "Bossart, Nathan" <[email-masked]>
Date: Thu, 16 Sep 2021 17:06:11 +0000

> are heading for shared_memory_size_in_huge_pages.

Thanks!  And done.

For the huge pages setup documentation, I considered sending stderr to
/dev/null to eliminate the LOG from the output, but I opted against
that.  That would've looked like this:

        postgres -D $PGDATA -C shared_memory_size_in_huge_pages 2> /dev/null

Otherwise, there aren't any significant changes in this version of the
patch besides the name change.

Nathan

---

From: Justin Pryzby <[email-masked]>
Date: Thu, 16 Sep 2021 12:14:27 -0500

nitpick: *are* not supported, as you say elsewhere.

Maybe this was already discussed, but "main" could be misleading.

To me that sounds like there might be huge pages needed for something other
than the "main" area and the reported value might turn out to be inadequate,
(which is exactly the issue these patch are trying to address).

In particular, this sounds like it's just going to report
shared_buffers/huge_page_size (since shared buffers is usually the "main" use
of shared memory) - rather than reporting the size of the entire shared memory
in units of huge_pages.

-- 
Justin

---

From: "Bossart, Nathan" <[email-masked]>
Date: Thu, 16 Sep 2021 21:26:56 +0000

> nitpick: *are* not supported, as you say elsewhere.

Updated.  I think I originally chose "is" because I was referring to
Huge Pages as a singular feature, but that sounds a bit awkward, and I
don't think there's any substantial difference either way.

> of shared memory) - rather than reporting the size of the entire shared memory

I'm not sure I agree on this one.  The documentation for huge_pages
[0] and shared_memory_type [1] uses the same phrasing multiple times,
and the new shared_memory_size GUC uses it as well [2].  I don't see
anything in the documentation that suggests that shared_buffers is the
only thing in the main shared memory area, and the documentation for
setting up huge pages makes no mention of any extra memory that needs
to be considered, either.

Furthermore, I'm not sure what else we'd call it.  I don't think it's
accurate to suggest that it's the entirety of shared memory for the
server, as it's possible to dynamically allocate more as needed (which
IIUC won't use any explicitly allocated huge pages).

Nathan

[0] https://www.postgresql.org/docs/devel/runtime-config-resource.html#GUC-HUGE-PAGES
[1] https://www.postgresql.org/docs/devel/runtime-config-resource.html#GUC-SHARED-MEMORY-TYPE
[2] https://www.postgresql.org/docs/devel/runtime-config-preset.html#GUC-SHARED-MEMORY-SIZE

---

From: Michael Paquier <[email-masked]>
Date: Fri, 17 Sep 2021 11:20:30 +0900

> to be considered, either.

Looks rather sane to me, FWIW.  I have not tested on Linux properly
yet (not tempted to take my bets on the buildfarm on a Friday,
either), but I should be able to handle that at the beginning of next
week.

+   GetHugePageSize(&hp_size, &unused);
+   if (hp_size != 0)
I'd rather change GetHugePageSize() to be able to accept NULL for the
parameter values, rather than declaring such variables.

+    To determine the number of huge pages needed, use the
+    <command>postgres</command> command to see the value of
+    <xref linkend="guc-shared-memory-size-in-huge-pages"/>.
We may want to say as well here that the server should be offline?
It would not hurt to duplicate this information with
postgres-ref.sgml.

+        This setting is supported only on Linux.  It is always set to
Nit: This paragraph is missing two <productname>s for Linux.  The docs
are random about that, but these are new entries.
--
Michael

---

From: "Bossart, Nathan" <[email-masked]>
Date: Fri, 17 Sep 2021 16:31:44 +0000

> parameter values, rather than declaring such variables.

Done.

> It would not hurt to duplicate this information with

Done.

> are random about that, but these are new entries.

Done.

Nathan

---

From: "Bossart, Nathan" <[email-masked]>
Date: Tue, 21 Sep 2021 00:08:22 +0000

Should we also initialize the shared memory GUCs in bootstrap and
single-user mode?  I think I missed this in bd17880.

Nathan

diff --git a/src/backend/bootstrap/bootstrap.c b/src/backend/bootstrap/bootstrap.c
index 48615c0ebc..4c4cf44871 100644
--- a/src/backend/bootstrap/bootstrap.c
+++ b/src/backend/bootstrap/bootstrap.c
@@ -324,6 +324,12 @@ BootstrapModeMain(int argc, char *argv[], bool check_only)

     InitializeMaxBackends();

+    /*
+     * Initialize runtime-computed GUCs that depend on the amount of shared
+     * memory required.
+     */
+    InitializeShmemGUCs();
+
     CreateSharedMemoryAndSemaphores();

     /*
diff --git a/src/backend/tcop/postgres.c b/src/backend/tcop/postgres.c
index 0775abe35d..cae0b079b9 100644
--- a/src/backend/tcop/postgres.c
+++ b/src/backend/tcop/postgres.c
@@ -3978,6 +3978,12 @@ PostgresSingleUserMain(int argc, char *argv[],
     /* Initialize MaxBackends */
     InitializeMaxBackends();

+    /*
+     * Initialize runtime-computed GUCs that depend on the amount of shared
+     * memory required.
+     */
+    InitializeShmemGUCs();
+
     CreateSharedMemoryAndSemaphores();

     /*

---

From: Michael Paquier <[email-masked]>
Date: Tue, 21 Sep 2021 10:48:11 +0900

> Done.

Thanks.  I have gone through the last patch this morning, did some
tests on all the platforms I have at hand (including Linux) and
finished by applying it after doing some small tweaks.  First, I have 
finished by extending GetHugePageSize() to accept NULL for its first
argument hugepagesize.  A second thing was in the docs, where it is
still useful IMO to keep the reference to /proc/meminfo and
/sys/kernel/mm/hugepages to let users know how the system impacts the
calculation of the new GUC.

Let's see what the buildfarm thinks about it.
--
Michael

---

From: Michael Paquier <[email-masked]>
Date: Tue, 21 Sep 2021 11:29:07 +0900

> single-user mode?  I think I missed this in bd17880.

Why would we need that for the bootstrap mode?

While looking at the patch for shared_memory_size, I have looked at
those code paths to note that some of the runtime GUCs would be set
thanks to the load of the control file, but supporting this case
sounded rather limited to me for --single when it came to shared
memory and huge page estimation and we don't load
shared_preload_libraries in this context either, which could lead to
wrong estimations.  Anyway, I am not going to fight hard if people
would like that for the --single mode, even if it may lead to an
underestimation of the shmem allocated.
--
Michael

---

From: "Bossart, Nathan" <[email-masked]>
Date: Tue, 21 Sep 2021 15:46:41 +0000

Thanks!

Nathan

---

From: "Bossart, Nathan" <[email-masked]>
Date: Tue, 21 Sep 2021 16:06:38 +0000

> underestimation of the shmem allocated.

I was looking at this from the standpoint of keeping the startup steps
consistent between the modes.  Looking again, I can't think of
a strong reason to add it to bootstrap mode.  I think the case for
adding it to single-user mode is a bit stronger, as commands like
"SHOW shared_memory_size;" currently return 0.  I lean in favor of
adding it for single-user mode, but it's probably fine either way.

Nathan

---

From: Michael Paquier <[email-masked]>
Date: Wed, 22 Sep 2021 12:53:26 +0900

> adding it for single-user mode, but it's probably fine either way.

I am not sure either as that's a tradeoff between an underestimation
and no information.  The argument of consistency indeed matters.
Let's see if others have any opinion to share on this point.
--
Michael

---

From: Robert Haas <[email-masked]>
Date: Thu, 23 Sep 2021 12:57:41 -0400

> Let's see if others have any opinion to share on this point.

Well, if we think the information won't be safe to use, it's better to
report nothing than a wrong value, I think.

---

From: Magnus Hagander <[email-masked]>
Date: Mon, 14 Mar 2022 16:26:43 +0100

>         $ postgres -D . -C shared_memory_size

Nothing fixing this ended up actually getting committed, right? That
is, we still get the extra log output?

And in fact, the command documented on
https://www.postgresql.org/docs/devel/kernel-resources.html doesn't
actually produce the output that the docs show, it also shows the log
line, in the default config? If we can't fix the extra logging we
should at least have our docs represent reality -- maybe by adding a
"2>/dev/null" entry? But it'd be better to have it not output that log
in the first place...

(Of course what I'd really want is to be able to run it on a cluster
that's running, but that was discussed downthread so I'm not bringing
that one up for changes now)

---

From: Nathan Bossart <[email-masked]>
Date: Mon, 14 Mar 2022 10:34:17 -0700

> is, we still get the extra log output?

Correct.

> "2>/dev/null" entry? But it'd be better to have it not output that log

I attached a patch to adjust the documentation for now.  This apparently
crossed my mind earlier [0], but I didn't follow through with it for some
reason.

> that one up for changes now)

I think it is worth revisiting the extra logging and the ability to view
runtime-computed GUCs on a running server.  Should this be an open item for
v15, or do you think it's alright to leave it for the v16 development
cycle?

[0] https://postgr.es/m/C45224E1-29C8-414C-A8E6-B718C07ACB94%40amazon.com

-- 
Nathan Bossart
Amazon Web Services: https://aws.amazon.com

---

From: Michael Paquier <[email-masked]>
Date: Tue, 15 Mar 2022 11:41:29 +0900

> crossed my mind earlier [0], but I didn't follow through with it for some

Another thing that we can add is -c log_min_messages=fatal, but my
method is more complicated than a simple redirection, of course :)

> v15, or do you think it's alright to leave it for the v16 development

Well, this is a completely new problem as it opens the door of
potential concurrent access issues with the data directory lock file
while reading values from the control file.  And that's not mandatory
to be able to get those estimations without having to allocate a large
chunk of memory, which was the primary goal discussed upthread as far
as I recall.  So I would leave that as an item to potentially tackle
in future versions.
--
Michael

---

From: Magnus Hagander <[email-masked]>
Date: Tue, 15 Mar 2022 23:02:37 +0100

> method is more complicated than a simple redirection, of course :)

Either does work, but yours has more characters :)

> as I recall.  So I would leave that as an item to potentially tackle

I think we're talking about two different things here.

I think the "avoid extra logging" would be worth seeing if we can
address for 15.

The "able to run on a live cluster" seems a lot bigger and more scary
and not 15 material.

---

From: Nathan Bossart <[email-masked]>
Date: Tue, 15 Mar 2022 15:44:39 -0700

> I think the "avoid extra logging" would be worth seeing if we can

A simple approach could be to just set log_min_messages to PANIC before
exiting.  I've attached a patch for this.  With this patch, we'll still see
a FATAL if we try to use 'postgres -C' for a runtime-computed GUC on a
running server, and there will be no extra output as long as the user sets
log_min_messages to INFO or higher (i.e., not a DEBUG* value).  For
comparison, 'postgres -C' for a non-runtime-computed GUC does not emit
extra output as long as the user sets log_min_messages to DEBUG2 or higher.

+1

---

From: Nathan Bossart <[email-masked]>
Date: Mon, 21 Mar 2022 15:12:05 -0700

> extra output as long as the user sets log_min_messages to DEBUG2 or higher.

I created a commitfest entry for this:

	https://commitfest.postgresql.org/38/3596/

---

From: Michael Paquier <[email-masked]>
Date: Wed, 23 Mar 2022 15:25:48 +0900

>  		puts(config_val ? config_val : "");
> +
> +		/* don't emit shutdown messages */
> +		SetConfigOption("log_min_messages", "PANIC", PGC_INTERNAL, PGC_S_OVERRIDE);
> +
>  		ExitPostmaster(0);

That's fancy, but I don't like that much.  And this would not protect
either against any messages generated before this code path, either,
even if that should be enough for the current HEAD .

My solution for the docs is perhaps too confusing for the end-user,
and we are talking about a Linux-only thing here anyway.  So, at the
end, I am tempted to just add the "2> /dev/null" as suggested upthread
by Nathan and call it a day.  Does that sound fine?
--
Michael

---

From: Michael Paquier <[email-masked]>
Date: Thu, 24 Mar 2022 21:10:29 +0900

On Wed, Mar 23, 2022 at 03:25:48PM +0900, Michael Paquier wrote:
> by Nathan and call it a day.

This still sounds like the best way to go for now, so done this way as
of bbd4951.
--
Michael

---

From: Magnus Hagander <[email-masked]>
Date: Thu, 24 Mar 2022 14:07:26 +0100

> either against any messages generated before this code path, either,

But neither would the suggestion of redirecting stderr to /dev/null.
In fact, doing the redirect it will *also* throw away any FATAL that
happens. In fact, using the 2>/dev/null method, we *also* remove the
message that says there's another postmaster running in this
directory, which is strictly worse than the override of
log_min_messages.

That said, the redirect can be removed without recompiling postgres,
so it is probably still hte better choice as a temporary workaround.
But we should really look into getting a better solution in place once
we start on 16.

> by Nathan and call it a day.  Does that sound fine?

What would be a linux only thing?

---

From: Nathan Bossart <[email-masked]>
Date: Thu, 24 Mar 2022 13:31:08 -0700

> But we should really look into getting a better solution in place once

A couple of other options to consider:

1) Always set log_min_messages to WARNING/ERROR/FATAL for 'postgres -C'.
We might need some special logic for handling the case where the user is
inspecting the log_min_messages parameter.  With this approach, you'd
probably never get extra output unless something was wrong (e.g., database
already running when inspecting a runtime-computed GUC).  Also, this would
silence any extra output that you might see today with non-runtime-computed
GUCs.

2) Add some way to skip just the shutdown message (e.g., a variable set
when output_config_variable is true).  With this approach, you wouldn't get
extra output by default, but you still might if log_min_messages is set to
something like DEBUG3.  This wouldn't impact any extra output that you see
today with non-runtime-computed GUCs.

---

From: Nathan Bossart <[email-masked]>
Date: Mon, 28 Mar 2022 10:35:03 -0700

> today with non-runtime-computed GUCs.

I've attached a first attempt at option 1.

---

From: Michael Paquier <[email-masked]>
Date: Wed, 20 Apr 2022 07:12:39 +0900

> directory, which is strictly worse than the override of

Well, we could also tweak more the command with a redirection of
stderr to a log file or such, and tell to look at it for errors.

> But we should really look into getting a better solution in place once

But do we really need a better or more invasive solution for already
running servers though?  A SHOW command would be able to do the work
already in this case.  This would lack consistency compared to the
offline case, but we are not without option either.  That leaves the
case where the server is running, has allocated memory but is not
ready to accept connections, like crash recovery, still this use case
looks rather thin to me. 

> What would be a linux only thing?

Perhaps not at some point in the future.  Now that's under a section
of the docs only for Linux.
--
Michael

---

From: Magnus Hagander <[email-masked]>
Date: Fri, 22 Apr 2022 09:49:34 +0200

> stderr to a log file or such, and tell to look at it for errors.

That's would be a pretty terrible ux though.

> ready to accept connections, like crash recovery, still this use case

I agree that thats a very narrow use case. And I'm nog sure the use case of
a running server is even that important here - it's really the offline one
that's important. Or rather, the really compelling one is when there is a
server running but I want to check the value offline because it will
change. SHOW doesn't help there because it shows the value based on the
currently running configuration, not the new one after a restart.

I don't agree that the redirect is a solution. It's a workaround.

> of the docs only for Linux.

Hmm. So what's the solution on windows? I guess maybe it's not as important
there because there is no limit on huge pages, but generally getting the
expected shared memory usage might be useful? Just significantly less
important.

/Magnus

---

From: Michael Paquier <[email-masked]>
Date: Mon, 25 Apr 2022 09:15:28 +0900

On Fri, Apr 22, 2022 at 09:49:34AM +0200, Magnus Hagander wrote:
> I agree that thats a very narrow use case. And I'm not sure the use case of

You mean the case of a server where one would directly change
postgresql.conf on a running server, and use postgres -C to see how
much the kernel settings need to be changed before the restart?

> expected shared memory usage might be useful? Just significantly less

Contrary to Linux, we don't need to care about the number of large
pages that are necessary because there is no equivalent of
vm.nr_hugepages on Windows (see [1]), do we?  If that were the case,
we'd have a use case for huge_page_size, additionally.

That's the case where shared_memory_size_in_huge_pages comes in
handy, as much as does huge_page_size, and note that
shared_memory_size works on WIN32.

[1]: https://docs.microsoft.com/en-us/windows/win32/memory/large-page-support
--
Michael

---

From: Magnus Hagander <[email-masked]>
Date: Mon, 25 Apr 2022 16:55:25 +0200

> much the kernel settings need to be changed before the restart?

Yes.

AIUI that was the original use-case for this feature. It certainly was for
me :)

> we'd have a use case for huge_page_size, additionally.

Right, for this one in particular -- that's what I meant with my comment
about there not being a limit. But this feature works for other settings as
well, not just the huge pages one.  Exactly what the use-cases are can
vary, but surely they would have the same problems wrt redirects?

-- 
 Magnus Hagander
 Me: https://www.hagander.net/ <http://www.hagander.net/>
 Work: https://www.redpill-linpro.com/ <http://www.redpill-linpro.com/>

---

From: Michael Paquier <[email-masked]>
Date: Tue, 26 Apr 2022 10:34:06 +0900

> AIUI that was the original use-case for this feature. It certainly was for

Perhaps we'd be fine with relaxing the requirements here knowing that
the control file should never be larger than PG_CONTROL_MAX_SAFE_SIZE
(aka the read should be atomic so it could be made lockless).  At the
end of the day, to be absolutely correct in the shmem size estimation,
I think that we are going to need what's proposed here or the sizing
may not be right depending on how extensions adjust GUCs after they
load their _PG_init():
https://www.postgresql.org/message-id/20220419154658.GA2487941@nathanxps13

That's a bit independent, but not completely unrelated either
depending on how exact you want your number of estimated huge pages to
be.  Just wanted to mention it.

> vary, but surely they would have the same problems wrt redirects?

Yes, the redirection issue would apply to all the run-time GUCs.
--
Michael

---

From: Nathan Bossart <[email-masked]>
Date: Fri, 6 May 2022 10:13:18 -0700

> Yes, the redirection issue would apply to all the run-time GUCs.

Should this be tracked as an open item for v15?  There was another recent
report about the extra log output [0].

[0] https://www.postgresql.org/message-id/YnARlI5nvbziobR4%40momjian.us

---

From: Michael Paquier <[email-masked]>
Date: Mon, 9 May 2022 15:53:24 +0900

> report about the extra log output [0].

That makes it for two complaints on two separate threads.  So an open
item seems adapted to adjust this behavior.

I have looked at the patch posted at [1], and I don't quite understand
why you need the extra dance with log_min_messages.  Why don't you
just set the GUC at the end of the code path in PostmasterMain() where
we print non-runtime-computed parameters?  I am not really worrying
about users deciding to set log_min_messages to PANIC in
postgresql.conf when it comes to postgres -C, TBH, as they'd miss the
FATAL messages if the command is attempted on a server already
starting.

Per se the attached.

[1]: https://www.postgresql.org/message-id/20220328173503.GA137769@nathanxps13
--
Michael
From 3b8a7f8079955cd59a5a318adf6938cdd3c29c6b Mon Sep 17 00:00:00 2001
From: Michael Paquier <[email-masked]>
Date: Mon, 9 May 2022 15:50:19 +0900
Subject: [PATCH v4] Silence extra logging with 'postgres -C'.

Presently, the server may emit a variety of extra log messages when
inspecting GUC values.  For example, when inspecting a runtime-computed
GUC, the server will always emit a "database system is shut down" LOG
(unless the user has set log_min_messages higher than LOG).  To avoid
these extra log messages, this change sets log_min_messages to FATAL
when -C is used (even if set to PANIC in postgresql.conf).  At FATAL,
the user will still receive messages explaining why a GUC's value cannot
be inspected.
---
 src/backend/postmaster/postmaster.c | 10 ++++++++++
 doc/src/sgml/runtime.sgml           |  2 +-
 2 files changed, 11 insertions(+), 1 deletion(-)

diff --git a/src/backend/postmaster/postmaster.c b/src/backend/postmaster/postmaster.c
index ce4007bb2c..38b63bc215 100644
--- a/src/backend/postmaster/postmaster.c
+++ b/src/backend/postmaster/postmaster.c
@@ -913,6 +913,16 @@ PostmasterMain(int argc, char *argv[])
 			puts(config_val ? config_val : "");
 			ExitPostmaster(0);
 		}
+
+		/*
+		 * A runtime-computed GUC will be printed later on.  As we initialize
+		 * a server startup sequence, silence any log messages that may show up
+		 * in the output generated.  FATAL and more severe messages are useful
+		 * to show, even if one would only expect at least PANIC.  LOG entries
+		 * are hidden.
+		 */
+		SetConfigOption("log_min_messages", "FATAL", PGC_INTERNAL,
+						PGC_S_OVERRIDE);
 	}
 
 	/* Verify that DataDir looks reasonable */
diff --git a/doc/src/sgml/runtime.sgml b/doc/src/sgml/runtime.sgml
index 4465c876b1..62cec614d3 100644
--- a/doc/src/sgml/runtime.sgml
+++ b/doc/src/sgml/runtime.sgml
@@ -1448,7 +1448,7 @@ export PG_OOM_ADJUST_VALUE=0
     server must be shut down to view this runtime-computed parameter.
     This might look like:
 <programlisting>
-$ <userinput>postgres -D $PGDATA -C shared_memory_size_in_huge_pages 2> /dev/null</userinput>
+$ <userinput>postgres -D $PGDATA -C shared_memory_size_in_huge_pages</userinput>
 3170
 $ <userinput>grep ^Hugepagesize /proc/meminfo</userinput>
 Hugepagesize:       2048 kB
-- 
2.36.0

---

From: Nathan Bossart <[email-masked]>
Date: Tue, 10 May 2022 09:12:49 -0700

On Mon, May 09, 2022 at 03:53:24PM +0900, Michael Paquier wrote:
> we print non-runtime-computed parameters?

The log_min_messages dance avoids extra output when inspecting
non-runtime-computed GUCs, like this:

	~/pgdata$ postgres -D . -C log_min_messages -c log_min_messages=debug5
	debug5
	2022-05-10 09:06:04.728 PDT [3715607] DEBUG:  shmem_exit(0): 0 before_shmem_exit callbacks to make
	2022-05-10 09:06:04.728 PDT [3715607] DEBUG:  shmem_exit(0): 0 on_shmem_exit callbacks to make
	2022-05-10 09:06:04.728 PDT [3715607] DEBUG:  proc_exit(0): 0 callbacks to make
	2022-05-10 09:06:04.728 PDT [3715607] DEBUG:  exit(0)

AFAICT you need to set log_min_messages to at least DEBUG3 to see extra
output for the non-runtime-computed GUCs, so it might not be worth the
added complexity.

> I am not really worrying

I don't have a strong opinion on this one.

---

From: Michael Paquier <[email-masked]>
Date: Wed, 11 May 2022 14:34:25 +0900

> output for the non-runtime-computed GUCs, so it might not be worth the

This set of messages is showing up for ages with zero complaints from
the field AFAIK, and nobody would use this level of logging except
developers.  One thing that overriding log_min_messages to FATAL does,
however, is to not show anymore those debug3 messages when querying a
runtime-computed GUC, but that's the kind of things we'd hide.  Your
patch would hide those entries in both cases.  Perhaps we could do
that, but at the end, I don't really see any need to complicate this
code path more than necessary, and this is enough to silence the logs
in the cases we care about basically all the time, even if the log
levels are reduced a bit on a given cluster.  Hence, I have applied
the simplest solution to just enforce a log_min_messages=FATAL when
requesting a runtime GUC.
--
Michael

---

From: Nathan Bossart <[email-masked]>
Date: Wed, 11 May 2022 08:57:19 -0700

On Wed, May 11, 2022 at 02:34:25PM +0900, Michael Paquier wrote:
> Hence, I have applied

Thanks!