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

At Wed, 28 Feb 2018 10:08:59 +0900, Michael Paquier <michael@paquier.xyz> wrote in <20180228010859.GB1476@paquier.xyz>
> On Tue, Feb 27, 2018 at 02:53:55PM -0500, Tom Lane wrote:
> > Andres Freund <andres@anarazel.de> writes:
> >> On 2018-02-27 14:41:47 -0500, Tom Lane wrote:
> >>> What I didn't understand about it was what kind of testing this'd make
> >>> harder.  If we desupport dynamic_shared_memory_type=none, there aren't
> >>> any code paths that need to cope with the case, and we should just
> >>> remove any code that thereby becomes unreachable.
> > 
> >> What I'm concerned about isn't so much testing paths specific to
> >> dynamic_shared_memory_type=none, but paths where we currently need
> >> fallbacks for the case we couldn't actually allocate dynamic shared
> >> memory. Which I think we at least somewhat gracefully need to deal with.
> > 
> > Ah.  That's a fair point, but I do not think
> > dynamic_shared_memory_type=none is a good substitute for having a way to
> > provoke allocation failures.  That doesn't let you test recovery from
> > situations where your first allocation works and second one fails, for
> > example; and cleanup from that sort of case is likely to be more
> > complicated than the trivial case.

On the other hand, dynamic_shared_memory_type=none is not a
proper means to silence DSM failures. If this patch causes some
problems, exactly the same things would have happened for the
setting dynamic_shared_memory_type *!=* none.  If we need more
"graceful" behavior for some specific failure modes, it should be
amended separately.

> dynamic_shared_memory_type is used in six code paths where it is aimed
> at doing sanity checks:
...
> The point is that there are other control mechanisms for each one of
> them.  Mainly, for the executor portion, the number of workers is
> controlled by other GUCs on planner-side.

If this means just that there should be any means other than the
variable to turn off user-visible parallel features:

> - Mute DSM initialization at postmaster startup.
> - Mute cleanup of previous DSM segments from a past postmaster.
These should be allowed unconditionally and should succeed.

> - Block backend startup if there is no DSM.
This is the result of any parallel activity, so this also should
be allowed unconditionally and should succeed.

> - Mute parallel query in planner.
max_parallel_workers_per_gather=0 kills it.
(min_parallel_*_scan_size or parallel_*_cost effectively can do
the similar.)

> - Mute parallel query for parallel btree builds.
max_parallel_maintenance_workers = 0 does.

> - Mute creation of parallel contexts in executor.
No gahter or gather merge results in this. So
max_parallel_workers_per_gather=0 also forces this.


The attached is the rebased version, on which the symbols are
renumbered 0-based as per comments.

regards,

-- 
Kyotaro Horiguchi
NTT Open Source Software Center
From 0672b1d80a08b0fb0a33a26aad8b1cc77bd83083 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. Assuming none hinder us from using it
for core features.  Just removing the option leads to intermittent
failure of regression test on some platforms due to insufficient
max_connection on initdb. This patch requires the patch that increases
the minimum fallback value of max_connection of initdb.
---
 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                |  9 ++++-----
 8 files changed, 22 insertions(+), 44 deletions(-)

diff --git a/doc/src/sgml/config.sgml b/doc/src/sgml/config.sgml
index 3a8fc7d803..932ce11a58 100644
--- a/doc/src/sgml/config.sgml
+++ b/doc/src/sgml/config.sgml
@@ -1602,9 +1602,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 9d4efc0f8f..4d27241502 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 14b7becf3e..c4f14391a8 100644
--- a/src/backend/optimizer/plan/planner.c
+++ b/src/backend/optimizer/plan/planner.c
@@ -303,7 +303,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 &&
@@ -5867,8 +5866,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 67e76b98fe..8239903210 100644
--- a/src/backend/storage/ipc/dsm_impl.c
+++ b/src/backend/storage/ipc/dsm_impl.c
@@ -105,7 +105,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}
 };
 
@@ -209,8 +208,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 39272925fb..a3a678e4c1 100644
--- a/src/backend/utils/misc/postgresql.conf.sample
+++ b/src/backend/utils/misc/postgresql.conf.sample
@@ -131,7 +131,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 1f7f2aaa54..5896d99ae1 100644
--- a/src/bin/initdb/initdb.c
+++ b/src/bin/initdb/initdb.c
@@ -933,6 +933,16 @@ test_config_settings(void)
                 ok_buffers = 0;
 
 
+    /*
+     * 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.
+     */
+    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);
 
@@ -945,10 +955,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)
@@ -980,10 +991,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)
@@ -995,11 +1007,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..2ed1d686b3 100644
--- a/src/include/storage/dsm_impl.h
+++ b/src/include/storage/dsm_impl.h
@@ -14,11 +14,10 @@
 #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
-#define DSM_IMPL_MMAP            4
+#define DSM_IMPL_POSIX            0
+#define DSM_IMPL_SYSV            1
+#define DSM_IMPL_WINDOWS        2
+#define DSM_IMPL_MMAP            3
 
 /*
  * Determine which dynamic shared memory implementations will be supported
-- 
2.16.2