Thread: Let's remove DSM_INPL_NONE.

Let's remove DSM_INPL_NONE.

From
Kyotaro HORIGUCHI
Date:
Hello.

As in the other threads[1][2], we have had several good reasons
to remove DSM_IMPL_NONE in PG11. The attached patch doesn that.

[1] https://www.postgresql.org/message-id/CA+Tgmoa0e23YC3SCwB85Yf5oUTRyirV=Sq_rXYxaXABLdPpjoA@mail.gmail.com

[2] https://www.postgresql.org/message-id/20180214.103858.02713585.horiguchi.kyotaro@lab.ntt.co.jp


It is amost a just-to-delete work but I see two issues raised so
far.

1. That change can raise regression test failure on some
   buildfarm machines[3].

The reason is that initdb creates a database with
max_connection=10 from DSM failure caused by semaphore exhaustion
, even though regression test requires at least 20
connections. At that time it was "fixed" by setting
dynamic_shared_memory_type=none while detecting the number of
usable max_connections and shared buffers[4].  Regardless of
whether the probing was succeeded or not, initdb finally creats a
database on that any DSM implementation is activated, since
initdb doesn't choose "none". Finally the test items for parallel
query actually suceeded.

For the reason above, I believe just increasing the minimum
fallback value of max_connections to 20 will work[5]. Anyway it
is a problem to be fixed that initdb creates an inexecutable
database if it uses the fallback values of max_connections=10.

[3] https://www.postgresql.org/message-id/CA+TgmoYHiiGrcvSSJhmbSEBMoF2zX_9_9rWd75Cwvu99YrDxew@mail.gmail.com

[4] Commit: d41ab71712a4457ed39d5471b23949872ac91def

[5] https://www.postgresql.org/message-id/20180209.170823.42008365.horiguchi.kyotaro@lab.ntt.co.jp


2. Server may chooose unusable DSM implementation while initdb probing. 

https://www.postgresql.org/message-id/20180214033248.GA1993@paquier.xyz

> Feel free to.  Be just careful that the connection attempts in
> test_config_settings() should try to use the DSM implementation defined
> by choose_dsm_implementation().

Thank you for the advice. The probing fails with the default
setting if posix shared memory somehow fails on a platform like
Linux that is usually expected to have it.  It's enough to choose
the implementation before probing. Some messages from initdb are
transposed as the result.

|   creating directory /home/horiguti/data/data_ndsm ... ok
|   creating subdirectories ... ok
| + selecting dynamic shared memory implementation ... posix
|   selecting default max_connections ... 100
|   selecting default shared_buffers ... 128MB
| - selecting dynamic shared memory implementation ... posix

Even though probing can end with failure in the case of [3], it
will not be a problem with the patch [4].

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center
From 44411ee17e881d1ee42fb91106d62aa97a7e45c9 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                |  1 -
 8 files changed, 18 insertions(+), 40 deletions(-)

diff --git a/doc/src/sgml/config.sgml b/doc/src/sgml/config.sgml
index c45979d..c2d86b3 100644
--- a/doc/src/sgml/config.sgml
+++ b/doc/src/sgml/config.sgml
@@ -1601,9 +1601,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 9d4efc0..4d27241 100644
--- a/src/backend/access/transam/parallel.c
+++ b/src/backend/access/transam/parallel.c
@@ -162,13 +162,6 @@ CreateParallelContext(const char *library_name, const char *function_name,
     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
      * parallel-aware.  Utility statement callers may ask us to ignore this
diff --git a/src/backend/optimizer/plan/planner.c b/src/backend/optimizer/plan/planner.c
index 3e8cd14..368fec4 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 &&
@@ -5825,8 +5824,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 f1f75b7..9629f22 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 67e76b9..8239903 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 9a35355..0d60193 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 2efd3b7..d520a39 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 0e5730f..e7acdff 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.9.2


Re: Let's remove DSM_INPL_NONE.

From
Andres Freund
Date:
Hi,

On 2018-02-15 19:58:57 +0900, Kyotaro HORIGUCHI wrote:
> As in the other threads[1][2], we have had several good reasons
> to remove DSM_IMPL_NONE in PG11. The attached patch doesn that.
> 
> [1] https://www.postgresql.org/message-id/CA+Tgmoa0e23YC3SCwB85Yf5oUTRyirV=Sq_rXYxaXABLdPpjoA@mail.gmail.com
> 
> [2] https://www.postgresql.org/message-id/20180214.103858.02713585.horiguchi.kyotaro@lab.ntt.co.jp

Hm, I'm not quite convinced by this. Seems to make testing a bunch of
codepaths harder.  I think it's fine to say that pg doesn't work
correctly with them disabled though.

- Andres


Re: Let's remove DSM_INPL_NONE.

From
Michael Paquier
Date:
On Thu, Feb 15, 2018 at 07:58:57PM +0900, Kyotaro HORIGUCHI wrote:
> It is amost a just-to-delete work but I see two issues raised so
> far.

The patch looks good as-is.  This simplifies a couple of code paths
deciding if parallel queries can be used or not, so future features in
need of doing the same decision-making won't fall in the same trap as
the recent parellel btree builds.  So I am +1 for having the buildfarm
express its opinion about it.

> 1. That change can raise regression test failure on some
>    buildfarm machines[3].
>
> The reason is that initdb creates a database with
> max_connection=10 from DSM failure caused by semaphore exhaustion
> , even though regression test requires at least 20
> connections. At that time it was "fixed" by setting
> dynamic_shared_memory_type=none while detecting the number of
> usable max_connections and shared buffers[4].  Regardless of
> whether the probing was succeeded or not, initdb finally creats a
> database on that any DSM implementation is activated, since
> initdb doesn't choose "none". Finally the test items for parallel
> query actually suceeded.
>
> For the reason above, I believe just increasing the minimum
> fallback value of max_connections to 20 will work[5]. Anyway it
> is a problem to be fixed that initdb creates an inexecutable
> database if it uses the fallback values of max_connections=10.
>
> [3]
> https://www.postgresql.org/message-id/CA+TgmoYHiiGrcvSSJhmbSEBMoF2zX_9_9rWd75Cwvu99YrDxew@mail.gmail.com

Do actually buildfarm machines select max_connections = 10 now?  We
would have surely seen failures since max_wal_senders has its default
value set to 10 as well.  Hence this is a separate problem.

> [4] Commit: d41ab71712a4457ed39d5471b23949872ac91def
>
> [5] https://www.postgresql.org/message-id/20180209.170823.42008365.horiguchi.kyotaro@lab.ntt.co.jp
>
>
> > Feel free to.  Be just careful that the connection attempts in
> > test_config_settings() should try to use the DSM implementation defined
> > by choose_dsm_implementation().
>
> Thank you for the advice. The probing fails with the default
> setting if posix shared memory somehow fails on a platform like
> Linux that is usually expected to have it.  It's enough to choose
> the implementation before probing. Some messages from initdb are
> transposed as the result.
>
> |   creating directory /home/horiguti/data/data_ndsm ... ok
> |   creating subdirectories ... ok
> | + selecting dynamic shared memory implementation ... posix
> |   selecting default max_connections ... 100
> |   selecting default shared_buffers ... 128MB
> | - selecting dynamic shared memory implementation ... posix
>
> Even though probing can end with failure in the case of [3], it
> will not be a problem with the patch [4].

That's fine by me, as this is actually the purpose of your patch.

+++ 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
You may as well assign numbers from 0 here and reorder the whole set.
--
Michael

Attachment

Re: Let's remove DSM_INPL_NONE.

From
Michael Paquier
Date:
On Thu, Feb 15, 2018 at 10:00:39AM -0800, Andres Freund wrote:
> Hm, I'm not quite convinced by this. Seems to make testing a bunch of
> codepaths harder.  I think it's fine to say that pg doesn't work
> correctly with them disabled though.

Well, for what it's worth that's one thing less to be forgotten when
implementing features related to parallel query.  That's the lesson
88ef48c is telling us here, much unlikely anybody would disable it
though after an initdb.
--
Michael

Attachment

Re: Let's remove DSM_INPL_NONE.

From
Kyotaro HORIGUCHI
Date:
Hello, thank you for the comment.

At Fri, 16 Feb 2018 13:07:08 +0900, Michael Paquier <michael@paquier.xyz> wrote in <20180216040708.GA1174@paquier.xyz>
> On Thu, Feb 15, 2018 at 07:58:57PM +0900, Kyotaro HORIGUCHI wrote:
> > It is amost a just-to-delete work but I see two issues raised so
> > far.
> 
> The patch looks good as-is.  This simplifies a couple of code paths
> deciding if parallel queries can be used or not, so future features in
> need of doing the same decision-making won't fall in the same trap as
> the recent parellel btree builds.  So I am +1 for having the buildfarm
> express its opinion about it.

Agreed. I'd like to see how buildfarm machines respond to this.

> > 1. That change can raise regression test failure on some
> >    buildfarm machines[3].
> > 
> > The reason is that initdb creates a database with
> > max_connection=10 from DSM failure caused by semaphore exhaustion
> > , even though regression test requires at least 20
> > connections. At that time it was "fixed" by setting
> > dynamic_shared_memory_type=none while detecting the number of
> > usable max_connections and shared buffers[4].  Regardless of
> > whether the probing was succeeded or not, initdb finally creats a
> > database on that any DSM implementation is activated, since
> > initdb doesn't choose "none". Finally the test items for parallel
> > query actually suceeded.
> > 
> > For the reason above, I believe just increasing the minimum
> > fallback value of max_connections to 20 will work[5]. Anyway it
> > is a problem to be fixed that initdb creates an inexecutable
> > database if it uses the fallback values of max_connections=10.
> > 
> > [3]
> > https://www.postgresql.org/message-id/CA+TgmoYHiiGrcvSSJhmbSEBMoF2zX_9_9rWd75Cwvu99YrDxew@mail.gmail.com
> 
> Do actually buildfarm machines select max_connections = 10 now?  We
> would have surely seen failures since max_wal_senders has its default
> value set to 10 as well.  Hence this is a separate problem.

Even not being pretty confident, that's because the current
initdb runs probing with dynamic_s_m_type=none. So the same BF
animal can fail if initdb probes with dynamic_s_m_type=sysv and
semaphore is exchausted at the time.

> > [4] Commit: d41ab71712a4457ed39d5471b23949872ac91def
> > 
> > [5] https://www.postgresql.org/message-id/20180209.170823.42008365.horiguchi.kyotaro@lab.ntt.co.jp
> > 
> > 
> > > Feel free to.  Be just careful that the connection attempts in
> > > test_config_settings() should try to use the DSM implementation defined
> > > by choose_dsm_implementation().
> > 
> > Thank you for the advice. The probing fails with the default
> > setting if posix shared memory somehow fails on a platform like
> > Linux that is usually expected to have it.  It's enough to choose
> > the implementation before probing. Some messages from initdb are
> > transposed as the result.
> > 
> > |   creating directory /home/horiguti/data/data_ndsm ... ok
> > |   creating subdirectories ... ok
> > | + selecting dynamic shared memory implementation ... posix
> > |   selecting default max_connections ... 100
> > |   selecting default shared_buffers ... 128MB
> > | - selecting dynamic shared memory implementation ... posix
> > 
> > Even though probing can end with failure in the case of [3], it
> > will not be a problem with the patch [4].
> 
> That's fine by me, as this is actually the purpose of your patch.
> 
> +++ 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
> You may as well assign numbers from 0 here and reorder the whole set.

The reason of that is I prefered to leave 0 as unused default
value. But it doesn't have significance after a clean build. I'm
fine with reordering (or renumbering) them.

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center



Re: Let's remove DSM_INPL_NONE.

From
Robert Haas
Date:
On Thu, Feb 15, 2018 at 5:58 AM, Kyotaro HORIGUCHI
<horiguchi.kyotaro@lab.ntt.co.jp> wrote:
> It is amost a just-to-delete work but I see two issues raised so
> far.

I think the two issues you are pointing out are the same issue --
namely, if initdb probes for a max_connections setting with an invalid
DSM implementation configured, it will fail the test for every value
of max_connections and thus select the lowest possible value.  The
solution to this is presumably just to make sure we choose the DSM
implementation before we do the max_connections probing so that we can
pass through the correct value there, which I think is what your patch
does.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company


Re: Let's remove DSM_INPL_NONE.

From
Robert Haas
Date:
On Thu, Feb 15, 2018 at 1:00 PM, Andres Freund <andres@anarazel.de> wrote:
> Hm, I'm not quite convinced by this. Seems to make testing a bunch of
> codepaths harder.  I think it's fine to say that pg doesn't work
> correctly with them disabled though.

I'm not sure I understand this.  Do you mean we should just add a
disclaimer to the documentation?

As long as dynamic_shared_memory_type=none is a supported
configuration, we can't use DSM for anything critical, but there's a
desire to use it in things like AV and the stats collector, which are
pretty critical.  Instead of removing it, we could do something like
this:

<note>
If you configure dynamic_shared_memory_type=none, parallel query will
be disabled.  Also, the stats collector will fail to run, so there
will be no statistics to trigger autovacuum, and your database will
become horribly bloated.  Actually, that would be true anyway even if
the stats collector somehow managed to work, because autovacuum itself
will also fail.  Moreover, because we've decide that you really have
to have dynamic shared memory, there may probably be other things that
also fail, too, either now or in the future.  Basically, if you
configure dynamic_shared_memory_type=none, expect a swift and painful
death.
</note>

...but I'm not sure that's really a great option.  It seems like
leaving user-visible knobs lying around that break the entire world is
a bad plan.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company


Re: Let's remove DSM_INPL_NONE.

From
Tom Lane
Date:
Robert Haas <robertmhaas@gmail.com> writes:
> On Thu, Feb 15, 2018 at 1:00 PM, Andres Freund <andres@anarazel.de> wrote:
>> Hm, I'm not quite convinced by this. Seems to make testing a bunch of
>> codepaths harder.  I think it's fine to say that pg doesn't work
>> correctly with them disabled though.

> I'm not sure I understand this.  Do you mean we should just add a
> disclaimer to the documentation?

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.

            regards, tom lane


Re: Let's remove DSM_INPL_NONE.

From
Andres Freund
Date:
On 2018-02-27 14:41:47 -0500, Tom Lane wrote:
> Robert Haas <robertmhaas@gmail.com> writes:
> > On Thu, Feb 15, 2018 at 1:00 PM, Andres Freund <andres@anarazel.de> wrote:
> >> Hm, I'm not quite convinced by this. Seems to make testing a bunch of
> >> codepaths harder.  I think it's fine to say that pg doesn't work
> >> correctly with them disabled though.
> 
> > I'm not sure I understand this.  Do you mean we should just add a
> > disclaimer to the documentation?
> 
> 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.

Greetings,

Andres Freund


Re: Let's remove DSM_INPL_NONE.

From
Tom Lane
Date:
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.

            regards, tom lane


Re: Let's remove DSM_INPL_NONE.

From
Michael Paquier
Date:
On Tue, Feb 27, 2018 at 02:00:36PM -0500, Robert Haas wrote:
> I think the two issues you are pointing out are the same issue --
> namely, if initdb probes for a max_connections setting with an invalid
> DSM implementation configured, it will fail the test for every value
> of max_connections and thus select the lowest possible value.  The
> solution to this is presumably just to make sure we choose the DSM
> implementation before we do the max_connections probing so that we can
> pass through the correct value there, which I think is what your patch
> does.

Yes, that's what the thing does.  It moves the routine to find the DSM
implementation before computing max_connections.
--
Michael

Attachment

Re: Let's remove DSM_INPL_NONE.

From
Michael Paquier
Date:
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.

dynamic_shared_memory_type is used in six code paths where it is aimed
at doing sanity checks:
- Mute DSM initialization at postmaster startup.
- Mute cleanup of previous DSM segments from a past postmaster.
- Block backend startup if there is no DSM.
- Mute parallel query in planner.
- Mute parallel query for parallel btree builds.
- Mute creation of parallel contexts in executor.
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.
--
Michael

Attachment

Re: Let's remove DSM_INPL_NONE.

From
Robert Haas
Date:
On Tue, Feb 27, 2018 at 2:50 PM, Andres Freund <andres@anarazel.de> wrote:
> 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.

Well, it's not very hard to just hack the code to make dsm_create()
always fail, or fail X% of the time, if you're so inclined.  I agree
that -c dynamic_shared_memory_type=none is a little more convenient
than sticking something like that into the code, but I don't think
it's sufficiently more convenient to justify keeping an option we
don't otherwise want.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company


Re: Let's remove DSM_INPL_NONE.

From
Tom Lane
Date:
Robert Haas <robertmhaas@gmail.com> writes:
> On Tue, Feb 27, 2018 at 2:50 PM, Andres Freund <andres@anarazel.de> wrote:
>> 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.

> Well, it's not very hard to just hack the code to make dsm_create()
> always fail, or fail X% of the time, if you're so inclined.  I agree
> that -c dynamic_shared_memory_type=none is a little more convenient
> than sticking something like that into the code, but I don't think
> it's sufficiently more convenient to justify keeping an option we
> don't otherwise want.

I'd be in favor of having some normally-ifdef'd-out facility for causing
failures of this kind.  (As I mentioned upthread, I do not think "always
fail" is sufficient.)  That's very different from having a user-visible
option, though.  We don't expose a GUC that enables CLOBBER_FREED_MEMORY,
to take a relevant example.

            regards, tom lane


Re: Let's remove DSM_INPL_NONE.

From
Robert Haas
Date:
On Tue, Feb 27, 2018 at 11:30 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> I'd be in favor of having some normally-ifdef'd-out facility for causing
> failures of this kind.  (As I mentioned upthread, I do not think "always
> fail" is sufficient.)  That's very different from having a user-visible
> option, though.  We don't expose a GUC that enables CLOBBER_FREED_MEMORY,
> to take a relevant example.

Sure.  Feel free to propose something you like.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company


Re: Let's remove DSM_INPL_NONE.

From
Kyotaro HORIGUCHI
Date:
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


Let's remove DSM_IMPL_NONE.

From
Kyotaro HORIGUCHI
Date:
Hello.

# Subject is fixed.

I think this kind of thing is sutable for doing at the beginning
of a dev cycle.

Since it has been a long time since DSM was introduced and it is
turned on by default and I believe no one turns it off on
purpose. I'd like to propose to get rid of the choice of none.

The motive of this is there's a case where fallback path is
large(?) but scarcely expected to be used.


regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center
From 3322194ecf9c9dc02f1f96f71abc421cd65ff1eb 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 7bfbc87109..68ca92d591 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 1f09fb6e6a..86b7b74129 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 &&
@@ -6046,8 +6045,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..0233fd4edc 100644
--- a/src/bin/initdb/initdb.c
+++ b/src/bin/initdb/initdb.c
@@ -984,6 +984,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);
 
@@ -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..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.3


Re: Let's remove DSM_IMPL_NONE.

From
Peter Eisentraut
Date:
On 26.06.18 09:10, Kyotaro HORIGUCHI wrote:
> --- a/src/bin/initdb/initdb.c
> +++ b/src/bin/initdb/initdb.c
> @@ -984,6 +984,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);
>  

I don't understand that comment.  initdb does test whether dsm=posix
works.  What more were you hoping for?

> @@ -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)

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

> --- 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

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.

-- 
Peter Eisentraut              http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services


Re: Let's remove DSM_IMPL_NONE.

From
Michael Paquier
Date:
On Fri, Jul 06, 2018 at 11:08:02PM +0200, Peter Eisentraut wrote:
> On 26.06.18 09:10, Kyotaro HORIGUCHI wrote:
>> --- 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
>
> 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.
--
Michael

Attachment

Re: Let's remove DSM_IMPL_NONE.

From
Kyotaro HORIGUCHI
Date:
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


Re: Let's remove DSM_IMPL_NONE.

From
Arthur Zakirov
Date:
Hello,

On Mon, Jul 09, 2018 at 06:07:24PM +0900, Kyotaro HORIGUCHI wrote:
> 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)

Just a little note. In parallel.sgml it is still mentioned that
dynamic_shared_memory_type accepts 'none' value:

> <xref linkend="guc-dynamic-shared-memory-type"/> must be set to a
> value other than <literal>none</literal>.

-- 
Arthur Zakirov
Postgres Professional: http://www.postgrespro.com
Russian Postgres Company


Re: Let's remove DSM_IMPL_NONE.

From
Kyotaro HORIGUCHI
Date:
Thank you for the notice.

At Mon, 9 Jul 2018 12:30:22 +0300, Arthur Zakirov <a.zakirov@postgrespro.ru> wrote in
<20180709093021.GA9309@zakirov.localdomain>
> Hello,
> 
> On Mon, Jul 09, 2018 at 06:07:24PM +0900, Kyotaro HORIGUCHI wrote:
> > 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)
> 
> Just a little note. In parallel.sgml it is still mentioned that
> dynamic_shared_memory_type accepts 'none' value:
> 
> > <xref linkend="guc-dynamic-shared-memory-type"/> must be set to a
> > value other than <literal>none</literal>.

Oops! Thanks. Just removed it altogether and I didn't find
another instance.

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center
From 1ff3e3ca9cd971cc75922ecd1507304ee2c27fd4 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 +++---
 doc/src/sgml/parallel.sgml                    |  8 --------
 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 -
 9 files changed, 18 insertions(+), 48 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/doc/src/sgml/parallel.sgml b/doc/src/sgml/parallel.sgml
index dd7834a763..e9a015ecd3 100644
--- a/doc/src/sgml/parallel.sgml
+++ b/doc/src/sgml/parallel.sgml
@@ -124,14 +124,6 @@ EXPLAIN SELECT * FROM pgbench_accounts WHERE filler LIKE '%x%';
         configured via <varname>max_parallel_workers_per_gather</varname>.
       </para>
     </listitem>
-
-    <listitem>
-      <para>
-        <xref linkend="guc-dynamic-shared-memory-type"/> must be set to a
-        value other than <literal>none</literal>.  Parallel query requires dynamic
-        shared memory in order to pass data between cooperating processes.
-      </para>
-    </listitem>
   </itemizedlist>
 
   <para>
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


Re: Let's remove DSM_IMPL_NONE.

From
Peter Eisentraut
Date:
committed

On 10.07.18 08:49, Kyotaro HORIGUCHI wrote:
> Thank you for the notice.
> 
> At Mon, 9 Jul 2018 12:30:22 +0300, Arthur Zakirov <a.zakirov@postgrespro.ru> wrote in
<20180709093021.GA9309@zakirov.localdomain>
>> Hello,
>>
>> On Mon, Jul 09, 2018 at 06:07:24PM +0900, Kyotaro HORIGUCHI wrote:
>>> 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)
>>
>> Just a little note. In parallel.sgml it is still mentioned that
>> dynamic_shared_memory_type accepts 'none' value:
>>
>>> <xref linkend="guc-dynamic-shared-memory-type"/> must be set to a
>>> value other than <literal>none</literal>.
> 
> Oops! Thanks. Just removed it altogether and I didn't find
> another instance.
> 
> regards.
> 


-- 
Peter Eisentraut              http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services


Re: Let's remove DSM_IMPL_NONE.

From
Kyotaro HORIGUCHI
Date:
Hello.

At Tue, 10 Jul 2018 18:52:54 +0200, Peter Eisentraut <peter.eisentraut@2ndquadrant.com> wrote in
<fda5aa38-63d6-6365-4e0b-aada93957b16@2ndquadrant.com>
> committed

Thank you for committing this!

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center