Re: Let's remove DSM_IMPL_NONE. - Mailing list pgsql-hackers

At Mon, 9 Jul 2018 10:49:19 +0900, Michael Paquier <michael@paquier.xyz> wrote in <20180709014919.GH1467@paquier.xyz>
> On Fri, Jul 06, 2018 at 11:08:02PM +0200, Peter Eisentraut wrote:
> > On 26.06.18 09:10, Kyotaro HORIGUCHI wrote:
> > I would avoid renumbering here.  It was kind of sensible to have NONE =
> > 0, so I'd keep the non-NONE ones as non-zero.
> 
> +1.

It is the result following your suggestion (;_;)..

Anyway, I'm willing to follow that :p
====


Thank you for the comments.

At Fri, 6 Jul 2018 23:08:02 +0200, Peter Eisentraut <peter.eisentraut@2ndquadrant.com> wrote in
<07bdeea7-e11c-ab13-9aa1-999e13ce2e92@2ndquadrant.com>
> +    /*
> +     * Server doesn't confirm that the server-default DSM implementation is
> +     * actually workable. Choose a fine one for probing then it is used on the
> +     * new database.
> +     */
..
> I don't understand that comment.  initdb does test whether dsm=posix
> works.  What more were you hoping for?

It is mentioning the fact that the test is performed *after* the
probing, and without '-c dynamic_sh..type=none', the
bootstrapping postgres uses hard-coded DSM implementation but it
may be unworkable. So we must spcify the workable one on probing
after losing the choice 'none'.

I revised the comment as below.

|  * The bootstrapping postgresql may use unworkable DSM
|  * implementation by default. Identify a workable one now and
|  * specify it in probing steps below.


> We could perhaps avoid some variability here by running the
> bootstrapping runs in initdb using hardcoded dsm settings of
> "sysv"/"windows".

I agree. Anyway the server won't start with any configuration on
which initdb using "sysv" or "windows" fails on probing. However,
since initdb is already finding the most preferable implement for
dynamic shared memory, I think that there's no reason not to use
it.


About removing "none", the only way to choose dsm_type=none on
servers in production is rewriting the configuration file in the
generated DB cluster. Even in the case, they are using at least
sysv (or may be using POSIX) shared memory and what is required
after this patch is at most just(!) increasing shmall/shmmax
settings as required for mandatory features using DSM (nothing
yet).


The new version v4 is changed in the following points.

- Don't renumber the DSM_IMPL symbols, just removed _NONE.

- Rewrote the pointed comment.

- Revised the commit message removing a mention to an
  already-committed patch.

- (and rebased)


regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center
From 444dd293e359c4165778fcefdd3b9686ff539b88 Mon Sep 17 00:00:00 2001
From: Kyotaro Horiguchi <horiguchi.kyotaro@lab.ntt.co.jp>
Date: Thu, 15 Feb 2018 11:22:50 +0900
Subject: [PATCH] Remove dynamic_shared_memroy_type=none

Nowadays PostgreSQL on all supported platform offers any kind of
dynamic shared memory feature. Having the choice of none hinder us
from using it in core features. It is used only in initdb probing for
max connections and we are assuming that any kind of DSM is available
in genrated dtabase cluster. This patch removes the choice at
all. Since just removing it can cause initdb failure in certain cases,
initdb now identifies an available DSM implementation prior to the
probing with this patch.
---
 doc/src/sgml/config.sgml                      |  6 +++---
 src/backend/access/transam/parallel.c         |  7 -------
 src/backend/optimizer/plan/planner.c          |  4 +---
 src/backend/storage/ipc/dsm.c                 | 15 ---------------
 src/backend/storage/ipc/dsm_impl.c            |  3 ---
 src/backend/utils/misc/postgresql.conf.sample |  1 -
 src/bin/initdb/initdb.c                       | 21 ++++++++++++++-------
 src/include/storage/dsm_impl.h                |  1 -
 8 files changed, 18 insertions(+), 40 deletions(-)

diff --git a/doc/src/sgml/config.sgml b/doc/src/sgml/config.sgml
index 5b913f00c1..e307bb4e8e 100644
--- a/doc/src/sgml/config.sgml
+++ b/doc/src/sgml/config.sgml
@@ -1662,9 +1662,9 @@ include_dir 'conf.d'
         should use.  Possible values are <literal>posix</literal> (for POSIX shared
         memory allocated using <literal>shm_open</literal>), <literal>sysv</literal>
         (for System V shared memory allocated via <literal>shmget</literal>),
-        <literal>windows</literal> (for Windows shared memory), <literal>mmap</literal>
-        (to simulate shared memory using memory-mapped files stored in the
-        data directory), and <literal>none</literal> (to disable this feature).
+        <literal>windows</literal> (for Windows shared memory),
+        and <literal>mmap</literal> (to simulate shared memory using
+        memory-mapped files stored in the data directory).
         Not all values are supported on all platforms; the first supported
         option is the default for that platform.  The use of the
         <literal>mmap</literal> option, which is not the default on any platform,
diff --git a/src/backend/access/transam/parallel.c b/src/backend/access/transam/parallel.c
index 1d631b7275..4e32cfff50 100644
--- a/src/backend/access/transam/parallel.c
+++ b/src/backend/access/transam/parallel.c
@@ -161,13 +161,6 @@ CreateParallelContext(const char *library_name, const char *function_name,
     /* Number of workers should be non-negative. */
     Assert(nworkers >= 0);
 
-    /*
-     * If dynamic shared memory is not available, we won't be able to use
-     * background workers.
-     */
-    if (dynamic_shared_memory_type == DSM_IMPL_NONE)
-        nworkers = 0;
-
     /*
      * If we are running under serializable isolation, we can't use parallel
      * workers, at least not until somebody enhances that mechanism to be
diff --git a/src/backend/optimizer/plan/planner.c b/src/backend/optimizer/plan/planner.c
index fd45c9767d..eeebf775a4 100644
--- a/src/backend/optimizer/plan/planner.c
+++ b/src/backend/optimizer/plan/planner.c
@@ -335,7 +335,6 @@ standard_planner(Query *parse, int cursorOptions, ParamListInfo boundParams)
      */
     if ((cursorOptions & CURSOR_OPT_PARALLEL_OK) != 0 &&
         IsUnderPostmaster &&
-        dynamic_shared_memory_type != DSM_IMPL_NONE &&
         parse->commandType == CMD_SELECT &&
         !parse->hasModifyingCTE &&
         max_parallel_workers_per_gather > 0 &&
@@ -6050,8 +6049,7 @@ plan_create_index_workers(Oid tableOid, Oid indexOid)
     double        allvisfrac;
 
     /* Return immediately when parallelism disabled */
-    if (dynamic_shared_memory_type == DSM_IMPL_NONE ||
-        max_parallel_maintenance_workers == 0)
+    if (max_parallel_maintenance_workers == 0)
         return 0;
 
     /* Set up largely-dummy planner state */
diff --git a/src/backend/storage/ipc/dsm.c b/src/backend/storage/ipc/dsm.c
index f1f75b73f5..9629f22f7a 100644
--- a/src/backend/storage/ipc/dsm.c
+++ b/src/backend/storage/ipc/dsm.c
@@ -150,10 +150,6 @@ dsm_postmaster_startup(PGShmemHeader *shim)
 
     Assert(!IsUnderPostmaster);
 
-    /* If dynamic shared memory is disabled, there's nothing to do. */
-    if (dynamic_shared_memory_type == DSM_IMPL_NONE)
-        return;
-
     /*
      * If we're using the mmap implementations, clean up any leftovers.
      * Cleanup isn't needed on Windows, and happens earlier in startup for
@@ -219,10 +215,6 @@ dsm_cleanup_using_control_segment(dsm_handle old_control_handle)
     uint32        i;
     dsm_control_header *old_control;
 
-    /* If dynamic shared memory is disabled, there's nothing to do. */
-    if (dynamic_shared_memory_type == DSM_IMPL_NONE)
-        return;
-
     /*
      * Try to attach the segment.  If this fails, it probably just means that
      * the operating system has been rebooted and the segment no longer
@@ -391,13 +383,6 @@ dsm_postmaster_shutdown(int code, Datum arg)
 static void
 dsm_backend_startup(void)
 {
-    /* If dynamic shared memory is disabled, reject this. */
-    if (dynamic_shared_memory_type == DSM_IMPL_NONE)
-        ereport(ERROR,
-                (errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE),
-                 errmsg("dynamic shared memory is disabled"),
-                 errhint("Set dynamic_shared_memory_type to a value other than \"none\".")));
-
 #ifdef EXEC_BACKEND
     {
         void       *control_address = NULL;
diff --git a/src/backend/storage/ipc/dsm_impl.c b/src/backend/storage/ipc/dsm_impl.c
index c6382ec031..77e1bab54b 100644
--- a/src/backend/storage/ipc/dsm_impl.c
+++ b/src/backend/storage/ipc/dsm_impl.c
@@ -106,7 +106,6 @@ const struct config_enum_entry dynamic_shared_memory_options[] = {
 #ifdef USE_DSM_MMAP
     {"mmap", DSM_IMPL_MMAP, false},
 #endif
-    {"none", DSM_IMPL_NONE, false},
     {NULL, 0, false}
 };
 
@@ -210,8 +209,6 @@ dsm_impl_can_resize(void)
 {
     switch (dynamic_shared_memory_type)
     {
-        case DSM_IMPL_NONE:
-            return false;
         case DSM_IMPL_POSIX:
             return true;
         case DSM_IMPL_SYSV:
diff --git a/src/backend/utils/misc/postgresql.conf.sample b/src/backend/utils/misc/postgresql.conf.sample
index 9e39baf466..657c3f81f8 100644
--- a/src/backend/utils/misc/postgresql.conf.sample
+++ b/src/backend/utils/misc/postgresql.conf.sample
@@ -133,7 +133,6 @@
                     #   sysv
                     #   windows
                     #   mmap
-                    # use none to disable dynamic shared memory
                     # (change requires restart)
 
 # - Disk -
diff --git a/src/bin/initdb/initdb.c b/src/bin/initdb/initdb.c
index ae22e7d9fb..75faac7d8c 100644
--- a/src/bin/initdb/initdb.c
+++ b/src/bin/initdb/initdb.c
@@ -984,6 +984,16 @@ test_config_settings(void)
                 ok_buffers = 0;
 
 
+    /*
+     * The bootstrapping postgresql may use unworkable DSM implementation by
+     * default. Identify a workable one now and specify it in the probing
+     * steps below.
+     */
+    printf(_("selecting dynamic shared memory implementation ... "));
+    fflush(stdout);
+    dynamic_shared_memory_type = choose_dsm_implementation();
+    printf("%s\n", dynamic_shared_memory_type);
+
     printf(_("selecting default max_connections ... "));
     fflush(stdout);
 
@@ -996,10 +1006,11 @@ test_config_settings(void)
                  "\"%s\" --boot -x0 %s "
                  "-c max_connections=%d "
                  "-c shared_buffers=%d "
-                 "-c dynamic_shared_memory_type=none "
+                 "-c dynamic_shared_memory_type=%s "
                  "< \"%s\" > \"%s\" 2>&1",
                  backend_exec, boot_options,
                  test_conns, test_buffs,
+                 dynamic_shared_memory_type,
                  DEVNULL, DEVNULL);
         status = system(cmd);
         if (status == 0)
@@ -1031,10 +1042,11 @@ test_config_settings(void)
                  "\"%s\" --boot -x0 %s "
                  "-c max_connections=%d "
                  "-c shared_buffers=%d "
-                 "-c dynamic_shared_memory_type=none "
+                 "-c dynamic_shared_memory_type=%s "
                  "< \"%s\" > \"%s\" 2>&1",
                  backend_exec, boot_options,
                  n_connections, test_buffs,
+                 dynamic_shared_memory_type,
                  DEVNULL, DEVNULL);
         status = system(cmd);
         if (status == 0)
@@ -1046,11 +1058,6 @@ test_config_settings(void)
         printf("%dMB\n", (n_buffers * (BLCKSZ / 1024)) / 1024);
     else
         printf("%dkB\n", n_buffers * (BLCKSZ / 1024));
-
-    printf(_("selecting dynamic shared memory implementation ... "));
-    fflush(stdout);
-    dynamic_shared_memory_type = choose_dsm_implementation();
-    printf("%s\n", dynamic_shared_memory_type);
 }
 
 /*
diff --git a/src/include/storage/dsm_impl.h b/src/include/storage/dsm_impl.h
index 0e5730f7c5..e7acdff355 100644
--- a/src/include/storage/dsm_impl.h
+++ b/src/include/storage/dsm_impl.h
@@ -14,7 +14,6 @@
 #define DSM_IMPL_H
 
 /* Dynamic shared memory implementations. */
-#define DSM_IMPL_NONE            0
 #define DSM_IMPL_POSIX            1
 #define DSM_IMPL_SYSV            2
 #define DSM_IMPL_WINDOWS        3
-- 
2.16.3