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(at)paquier(dot)xyz> wrote in <20180216040708(dot)GA1174(at)paquier(dot)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