Let's remove DSM_INPL_NONE.

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/[email protected]

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/[email protected]

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

https://www.postgresql.org/message-id/[email protected]

> 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

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/[email protected]

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

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/[email protected]
>
>
> > 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

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

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/[email protected]
> >
> >
> > > 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

On Thu, Feb 15, 2018 at 5:58 AM, Kyotaro HORIGUCHI
<horiguchi(dot)kyotaro(at)lab(dot)ntt(dot)co(dot)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

On Thu, Feb 15, 2018 at 1:00 PM, Andres Freund <andres(at)anarazel(dot)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

Robert Haas <robertmhaas(at)gmail(dot)com> writes:
> On Thu, Feb 15, 2018 at 1:00 PM, Andres Freund <andres(at)anarazel(dot)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

On 2018-02-27 14:41:47 -0500, Tom Lane wrote:
> Robert Haas <robertmhaas(at)gmail(dot)com> writes:
> > On Thu, Feb 15, 2018 at 1:00 PM, Andres Freund <andres(at)anarazel(dot)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

Andres Freund <andres(at)anarazel(dot)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

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

On Tue, Feb 27, 2018 at 02:53:55PM -0500, Tom Lane wrote:
> Andres Freund <andres(at)anarazel(dot)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

On Tue, Feb 27, 2018 at 2:50 PM, Andres Freund <andres(at)anarazel(dot)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

Robert Haas <robertmhaas(at)gmail(dot)com> writes:
> On Tue, Feb 27, 2018 at 2:50 PM, Andres Freund <andres(at)anarazel(dot)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

On Tue, Feb 27, 2018 at 11:30 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)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

At Wed, 28 Feb 2018 10:08:59 +0900, Michael Paquier <michael(at)paquier(dot)xyz> wrote in <20180228010859(dot)GB1476(at)paquier(dot)xyz>
> On Tue, Feb 27, 2018 at 02:53:55PM -0500, Tom Lane wrote:
> > Andres Freund <andres(at)anarazel(dot)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

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

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

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

At Mon, 9 Jul 2018 10:49:19 +0900, Michael Paquier <michael(at)paquier(dot)xyz> wrote in <20180709014919(dot)GH1467(at)paquier(dot)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(dot)eisentraut(at)2ndquadrant(dot)com> wrote in <07bdeea7-e11c-ab13-9aa1-999e13ce2e92(at)2ndquadrant(dot)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

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

Thank you for the notice.

At Mon, 9 Jul 2018 12:30:22 +0300, Arthur Zakirov <a(dot)zakirov(at)postgrespro(dot)ru> wrote in <20180709093021(dot)GA9309(at)zakirov(dot)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

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(dot)zakirov(at)postgrespro(dot)ru> wrote in <20180709093021(dot)GA9309(at)zakirov(dot)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

Hello.

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

Thank you for committing this!

regards.

--
Kyotaro Horiguchi
NTT Open Source Software Center