Re: Let's remove DSM_INPL_NONE.

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