Horton

Thread

Commits

  1. Cope with glibc too old to have epoll_create1().

  2. Make latch.c more paranoid about child-process cases.

  3. Allow multiple bgworkers to be launched per postmaster iteration.

  4. Revert "Use pselect(2) not select(2), if available, to wait in postmaster's loop."

  5. Use pselect(2) not select(2), if available, to wait in postmaster's loop.

  6. Run the postmaster's signal handlers without SA_RESTART.

  7. Fix postmaster's handling of fork failure for a bgworker process.

  8. Partially revert commit 536d47bd9d5fce8d91929bee3128fa1d08dbcc57.

  9. Avoid depending on non-POSIX behavior of fcntl(2).

  10. Remove long-obsolete catering for platforms without F_SETFD/FD_CLOEXEC.

  1. Unportable implementation of background worker start

    Tom Lane <tgl@sss.pgh.pa.us> — 2017-04-19T22:04:07Z

    I chanced to notice that on gaur/pademelon, the "select_parallel"
regression test sometimes takes a great deal longer than normal,
for no obvious reason.  It does eventually terminate, but sometimes
only after 10 to 20 minutes rather than the couple dozen seconds
that are typical on that slow machine.

After a fair amount of hair-pulling, I traced the problem to the
fact that maybe_start_bgworker() will only start at most one worker
per call; after that, it sets StartWorkerNeeded = true and returns,
opining in a comment that ServerLoop will make another call shortly.

Unfortunately, that's hogwash.  It happens to work reliably on Linux,
because according to signal(7)

       The following interfaces are never restarted after being interrupted by
       a signal handler, regardless of the use of SA_RESTART; they always fail
       with the error EINTR when interrupted by a signal handler:

           ... select(2) ...

However, that's a Linux-ism.  What POSIX 2008 says about it, in the
select(2) reference page, is that

	If SA_RESTART has been set for the interrupting signal, it is
	implementation-defined whether the function restarts or returns
	with [EINTR].

HPUX apparently adopts the "restart" definition, and as we've previously
found out, not only does select(2) not return immediately but it actually
seems to reset the timeout timer to its original value.  (Some googling
suggests that restarting occurs on SVR4-derived kernels but not
BSD-derived ones.)

So what happens, if several RegisterDynamicBackgroundWorker requests
arrive in a single iteration of the postmaster's sigusr1_handler,
is that the first one is serviced thanks to the maybe_start_bgworker
call appearing in sigusr1_handler, and then we return to the select()
call and sleep.  The next start request is serviced only when the
typically-60-second select timeout expires, or more usually when some
other interrupt arrives at the postmaster.  In the regression tests,
what seems to happen is that we get a PMSIGNAL_START_AUTOVAC_WORKER
from the autovac launcher every thirty seconds, allowing one more bgworker
to get launched each time we go through sigusr1_handler.

Here's an actual trace of one select_parallel.sql query trying to
launch four parallel workers; I added a bunch of elog(LOG) calls
to help diagnose what was going on:

2017-04-19 17:25:33.448 EDT [8827] LOG:  signaling postmaster for startup of slot 1
2017-04-19 17:25:33.448 EDT [8827] STATEMENT:  select count(*) from tenk1, tenk2 where tenk1.hundred > 1 and tenk2.thousand=0;
2017-04-19 17:25:33.448 EDT [8827] LOG:  signaling postmaster for startup of slot 2
2017-04-19 17:25:33.448 EDT [8827] STATEMENT:  select count(*) from tenk1, tenk2 where tenk1.hundred > 1 and tenk2.thousand=0;
2017-04-19 17:25:33.448 EDT [8827] LOG:  signaling postmaster for startup of slot 3
2017-04-19 17:25:33.448 EDT [8827] STATEMENT:  select count(*) from tenk1, tenk2 where tenk1.hundred > 1 and tenk2.thousand=0;
2017-04-19 17:25:33.448 EDT [8827] LOG:  signaling postmaster for startup of slot 4
2017-04-19 17:25:33.448 EDT [8827] STATEMENT:  select count(*) from tenk1, tenk2 where tenk1.hundred > 1 and tenk2.thousand=0;
2017-04-19 17:25:33.563 EDT [6456] LOG:  entering sigusr1_handler
2017-04-19 17:25:33.563 EDT [6456] LOG:  registering background worker "parallel worker for PID 8827"
2017-04-19 17:25:33.563 EDT [6456] LOG:  registering background worker "parallel worker for PID 8827"
2017-04-19 17:25:33.563 EDT [6456] LOG:  registering background worker "parallel worker for PID 8827"
2017-04-19 17:25:33.563 EDT [6456] LOG:  registering background worker "parallel worker for PID 8827"
2017-04-19 17:25:33.563 EDT [6456] LOG:  entered maybe_start_bgworker, StartWorkerNeeded=1, HaveCrashedWorker=1
2017-04-19 17:25:33.563 EDT [6456] LOG:  starting background worker process "parallel worker for PID 8827"
2017-04-19 17:25:33.566 EDT [8871] LOG:  starting parallel worker 3
2017-04-19 17:25:33.642 EDT [8871] LOG:  exiting parallel worker 3
2017-04-19 17:25:33.642 EDT [8871] STATEMENT:  select count(*) from tenk1, tenk2 where tenk1.hundred > 1 and tenk2.thousand=0;
2017-04-19 17:25:33.647 EDT [6456] LOG:  leaving sigusr1_handler
2017-04-19 17:25:33.647 EDT [6456] LOG:  entering reaper
2017-04-19 17:25:33.647 EDT [6456] LOG:  unregistering background worker "parallel worker for PID 8827"
2017-04-19 17:25:33.647 EDT [6456] LOG:  reaped bgworker pid 8871 status 0
2017-04-19 17:25:33.647 EDT [6456] LOG:  leaving reaper
2017-04-19 17:26:03.114 EDT [6456] LOG:  entering sigusr1_handler
2017-04-19 17:26:03.115 EDT [6456] LOG:  entered maybe_start_bgworker, StartWorkerNeeded=1, HaveCrashedWorker=1
2017-04-19 17:26:03.115 EDT [6456] LOG:  starting background worker process "parallel worker for PID 8827"
2017-04-19 17:26:03.118 EDT [8874] LOG:  starting parallel worker 2
2017-04-19 17:26:03.164 EDT [8874] LOG:  exiting parallel worker 2
2017-04-19 17:26:03.164 EDT [8874] STATEMENT:  select count(*) from tenk1, tenk2 where tenk1.hundred > 1 and tenk2.thousand=0;
2017-04-19 17:26:03.185 EDT [6456] LOG:  leaving sigusr1_handler
2017-04-19 17:26:03.185 EDT [6456] LOG:  entering reaper
2017-04-19 17:26:03.186 EDT [6456] LOG:  unregistering background worker "parallel worker for PID 8827"
2017-04-19 17:26:03.186 EDT [6456] LOG:  reaped bgworker pid 8874 status 0
2017-04-19 17:26:03.186 EDT [6456] LOG:  leaving reaper
2017-04-19 17:26:03.284 EDT [6456] LOG:  entering reaper
2017-04-19 17:26:03.284 EDT [6456] LOG:  leaving reaper
2017-04-19 17:26:33.378 EDT [6456] LOG:  entering sigusr1_handler
2017-04-19 17:26:33.378 EDT [6456] LOG:  entered maybe_start_bgworker, StartWorkerNeeded=1, HaveCrashedWorker=1
2017-04-19 17:26:33.378 EDT [6456] LOG:  starting background worker process "parallel worker for PID 8827"
2017-04-19 17:26:33.382 EDT [8876] LOG:  starting parallel worker 1
2017-04-19 17:26:33.428 EDT [8876] LOG:  exiting parallel worker 1
2017-04-19 17:26:33.428 EDT [8876] STATEMENT:  select count(*) from tenk1, tenk2 where tenk1.hundred > 1 and tenk2.thousand=0;
2017-04-19 17:26:33.452 EDT [6456] LOG:  leaving sigusr1_handler
2017-04-19 17:26:33.453 EDT [6456] LOG:  entering reaper
2017-04-19 17:26:33.453 EDT [6456] LOG:  unregistering background worker "parallel worker for PID 8827"
2017-04-19 17:26:33.453 EDT [6456] LOG:  reaped bgworker pid 8876 status 0
2017-04-19 17:26:33.453 EDT [6456] LOG:  leaving reaper
2017-04-19 17:26:33.560 EDT [6456] LOG:  entering reaper
2017-04-19 17:26:33.560 EDT [6456] LOG:  leaving reaper
2017-04-19 17:27:03.114 EDT [6456] LOG:  entering sigusr1_handler
2017-04-19 17:27:03.115 EDT [6456] LOG:  entered maybe_start_bgworker, StartWorkerNeeded=1, HaveCrashedWorker=1
2017-04-19 17:27:03.115 EDT [6456] LOG:  starting background worker process "parallel worker for PID 8827"
2017-04-19 17:27:03.118 EDT [8879] LOG:  starting parallel worker 0
2017-04-19 17:27:03.167 EDT [8879] LOG:  exiting parallel worker 0
2017-04-19 17:27:03.167 EDT [8879] STATEMENT:  select count(*) from tenk1, tenk2 where tenk1.hundred > 1 and tenk2.thousand=0;
2017-04-19 17:27:03.174 EDT [6456] LOG:  leaving sigusr1_handler
2017-04-19 17:27:03.174 EDT [6456] LOG:  entering reaper
2017-04-19 17:27:03.175 EDT [6456] LOG:  unregistering background worker "parallel worker for PID 8827"
2017-04-19 17:27:03.184 EDT [6456] LOG:  reaped bgworker pid 8879 status 0
2017-04-19 17:27:03.184 EDT [6456] LOG:  leaving reaper

While I haven't yet tested it, it seems like a fix might be as simple
as deleting these lines in maybe_start_bgworker:

            /*
             * Have ServerLoop call us again.  Note that there might not
             * actually *be* another runnable worker, but we don't care all
             * that much; we will find out the next time we run.
             */
            StartWorkerNeeded = true;
            return;

So I'm wondering what the design rationale was for only starting one
bgworker per invocation.

It also appears to me that do_start_bgworker's treatment of fork
failure is completely brain dead.  Did anyone really think about
that case?

			regards, tom lane

  2. Re: Unportable implementation of background worker start

    Alvaro Herrera <alvherre@2ndquadrant.com> — 2017-04-19T22:24:15Z

    Tom Lane wrote:

> While I haven't yet tested it, it seems like a fix might be as simple
> as deleting these lines in maybe_start_bgworker:
> 
>             /*
>              * Have ServerLoop call us again.  Note that there might not
>              * actually *be* another runnable worker, but we don't care all
>              * that much; we will find out the next time we run.
>              */
>             StartWorkerNeeded = true;
>             return;
> 
> So I'm wondering what the design rationale was for only starting one
> bgworker per invocation.

The rationale was that there may be other tasks waiting for postmaster
attention, and if there are many bgworkers needing to be started, the
other work may be delayed for a long time.  This is not the first time
that this rationale has been challenged, but so far there hasn't been
any good reason to change it.  One option is to just remove it as you
propose, but a different one is to stop using select(2) in ServerLoop,
because those behavior differences seem to make it rather unusable.

> It also appears to me that do_start_bgworker's treatment of fork
> failure is completely brain dead.  Did anyone really think about
> that case?

Hmm, I probably modelled it on autovacuum without giving that case much
additional consideration.

-- 
Álvaro Herrera                https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

  3. Re: Unportable implementation of background worker start

    Tom Lane <tgl@sss.pgh.pa.us> — 2017-04-19T22:37:22Z

    Alvaro Herrera <alvherre@2ndquadrant.com> writes:
> Tom Lane wrote:
>> So I'm wondering what the design rationale was for only starting one
>> bgworker per invocation.

> The rationale was that there may be other tasks waiting for postmaster
> attention, and if there are many bgworkers needing to be started, the
> other work may be delayed for a long time.  This is not the first time
> that this rationale has been challenged, but so far there hasn't been
> any good reason to change it.  One option is to just remove it as you
> propose, but a different one is to stop using select(2) in ServerLoop,
> because those behavior differences seem to make it rather unusable.

Hm.  Do you have a more-portable alternative?

			regards, tom lane

  4. Re: Unportable implementation of background worker start

    Alvaro Herrera <alvherre@2ndquadrant.com> — 2017-04-19T22:44:24Z

    Tom Lane wrote:
> Alvaro Herrera <alvherre@2ndquadrant.com> writes:
> > Tom Lane wrote:
> >> So I'm wondering what the design rationale was for only starting one
> >> bgworker per invocation.
> 
> > The rationale was that there may be other tasks waiting for postmaster
> > attention, and if there are many bgworkers needing to be started, the
> > other work may be delayed for a long time.  This is not the first time
> > that this rationale has been challenged, but so far there hasn't been
> > any good reason to change it.  One option is to just remove it as you
> > propose, but a different one is to stop using select(2) in ServerLoop,
> > because those behavior differences seem to make it rather unusable.
> 
> Hm.  Do you have a more-portable alternative?

I was thinking in a WaitEventSet from latch.c.

-- 
Álvaro Herrera                https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

  5. Re: Unportable implementation of background worker start

    Tom Lane <tgl@sss.pgh.pa.us> — 2017-04-19T22:56:26Z

    Alvaro Herrera <alvherre@2ndquadrant.com> writes:
> Tom Lane wrote:
>> Hm.  Do you have a more-portable alternative?

> I was thinking in a WaitEventSet from latch.c.

Yeah, some googling turns up the suggestion that a self-pipe is a portable
way to get consistent semantics from select(); latch.c has already done
that.  I suppose that using latch.c would be convenient in that we'd have
to write little new code, but it's a tad annoying to add the overhead of a
self-pipe on platforms where we don't need it (which seems to be most).

			regards, tom lane

  6. Re: Unportable implementation of background worker start

    Andres Freund <andres@anarazel.de> — 2017-04-19T23:52:41Z

    On 2017-04-19 18:56:26 -0400, Tom Lane wrote:
> Alvaro Herrera <alvherre@2ndquadrant.com> writes:
> > Tom Lane wrote:
> >> Hm.  Do you have a more-portable alternative?
> 
> > I was thinking in a WaitEventSet from latch.c.
> 
> Yeah, some googling turns up the suggestion that a self-pipe is a portable
> way to get consistent semantics from select(); latch.c has already done
> that.  I suppose that using latch.c would be convenient in that we'd have
> to write little new code, but it's a tad annoying to add the overhead of a
> self-pipe on platforms where we don't need it (which seems to be most).

FWIW, I'd wished before that we used something a bit more modern than
select() if available... It's nice to be able to listen to a larger
number of sockets without repeated O(sockets) overhead.

BTW, we IIRC had discussed removing the select() backed latch
implementation in this release.  I'll try to dig up that discussion.

- Andres

  7. Re: Unportable implementation of background worker start

    Tom Lane <tgl@sss.pgh.pa.us> — 2017-04-20T00:06:05Z

    Andres Freund <andres@anarazel.de> writes:
> FWIW, I'd wished before that we used something a bit more modern than
> select() if available... It's nice to be able to listen to a larger
> number of sockets without repeated O(sockets) overhead.

[ raised eyebrow... ]  Is anyone really running postmasters with enough
listen sockets for that to be meaningful?

> BTW, we IIRC had discussed removing the select() backed latch
> implementation in this release.  I'll try to dig up that discussion.

Might be sensible.  Even my pet dinosaurs have poll(2).  We should
check the buildfarm to see if the select() implementation is being
tested at all.

			regards, tom lane

  8. Re: Unportable implementation of background worker start

    Tom Lane <tgl@sss.pgh.pa.us> — 2017-04-20T04:50:13Z

    I wrote:
> Alvaro Herrera <alvherre@2ndquadrant.com> writes:
>> Tom Lane wrote:
>>> Hm.  Do you have a more-portable alternative?

>> I was thinking in a WaitEventSet from latch.c.

My first reaction was that that sounded like a lot more work than removing
two lines from maybe_start_bgworker and adjusting some comments.  But on
closer inspection, the slow-bgworker-start issue isn't the only problem
here.  On a machine that restarts select()'s timeout after an interrupt,
as (at least) HPUX does, the postmaster will actually never iterate
ServerLoop's loop except immediately after receiving a new connection
request.  The stream of interrupts from the autovac launcher is alone
sufficient to prevent the initial 60-second timeout from ever elapsing.
So if there are no new connection requests for awhile, none of the
housekeeping actions in ServerLoop get done.

Most of those actions are concerned with restarting failed background
tasks, which is something we could get by without --- it's unlikely
that those tasks would fail without causing a database-wide restart,
and then there really isn't going to be much need for them until at least
one new connection request has arrived.  But the last step in that loop is
concerned with touching the sockets and lock files to prevent aggressive
/tmp cleaners from removing them, and that's something that can't be let
slide, or we might as well not have the logic at all.

I've confirmed experimentally that on my HPUX box, a postmaster not
receiving new connections for an hour or more in fact fails to update
the mod times on the sockets and lock files.  So that problem isn't
hypothetical.

So it's looking to me like we do need to do something about this, and
ideally back-patch it all the way.  But WaitEventSet doesn't exist
before 9.6.  Do we have the stomach for back-patching that into
stable branches?

			regards, tom lane

  9. Re: Unportable implementation of background worker start

    Tom Lane <tgl@sss.pgh.pa.us> — 2017-04-20T23:23:28Z

    I wrote:
> Alvaro Herrera <alvherre@2ndquadrant.com> writes:
>> Tom Lane wrote:
>>> Hm.  Do you have a more-portable alternative?

>> I was thinking in a WaitEventSet from latch.c.

> My first reaction was that that sounded like a lot more work than removing
> two lines from maybe_start_bgworker and adjusting some comments.  But on
> closer inspection, the slow-bgworker-start issue isn't the only problem
> here.  On a machine that restarts select()'s timeout after an interrupt,
> as (at least) HPUX does, the postmaster will actually never iterate
> ServerLoop's loop except immediately after receiving a new connection
> request.

I had a go at making the postmaster use a WaitEventSet, and immediately
ran into problems: latch.c is completely unprepared to be used anywhere
except a postmaster child process.  I think we can get around the issue
for the self-pipe, as per the attached untested patch.  But there remains
a problem: we should do a FreeWaitEventSet() after forking a child
process to ensure that postmaster children aren't running around with
open FDs for the postmaster's stuff.  This is no big problem in a regular
Unix build; we can give ClosePostmasterPorts() the responsibility.
It *is* a problem in EXEC_BACKEND children, which won't have inherited
the WaitEventSet data structure.  Maybe we could ignore the problem for
Unix EXEC_BACKEND builds, since we consider those to be for debug
purposes only, not for production.  But I don't think we can get away
with it for Windows --- or are the HANDLEs in a Windows WaitEventSet
not inheritable resources?

			regards, tom lane

  10. Re: Unportable implementation of background worker start

    Andres Freund <andres@anarazel.de> — 2017-04-20T23:32:48Z

    On 2017-04-20 19:23:28 -0400, Tom Lane wrote:
> or are the HANDLEs in a Windows WaitEventSet not inheritable
> resources?

I think we have control over that. According to
https://msdn.microsoft.com/en-us/library/windows/desktop/ms724466(v=vs.85).aspx
CreateProcess() has to be called with bInheritHandles = true (which we
do for backends), and SECURITY_ATTRIBUTES.bInheritHandle has to be true
too.  The latter we already only do for InitSharedLatch(), but not for
InitLatch(), nor for the WSACreateEvent's created for sockets - those
apparently can never be inherited.

So that kind of sounds like it should be doable.

- Andres

  11. Re: Unportable implementation of background worker start

    Tom Lane <tgl@sss.pgh.pa.us> — 2017-04-20T23:39:15Z

    Andres Freund <andres@anarazel.de> writes:
> On 2017-04-20 19:23:28 -0400, Tom Lane wrote:
>> or are the HANDLEs in a Windows WaitEventSet not inheritable
>> resources?

> I think we have control over that. According to
> https://msdn.microsoft.com/en-us/library/windows/desktop/ms724466(v=vs.85).aspx
> CreateProcess() has to be called with bInheritHandles = true (which we
> do for backends), and SECURITY_ATTRIBUTES.bInheritHandle has to be true
> too.  The latter we already only do for InitSharedLatch(), but not for
> InitLatch(), nor for the WSACreateEvent's created for sockets - those
> apparently can never be inherited.

> So that kind of sounds like it should be doable.

Ah, good.  I'll add a comment about that and press on.

			regards, tom lane

  12. Re: Unportable implementation of background worker start

    Andres Freund <andres@anarazel.de> — 2017-04-20T23:44:20Z

    On 2017-04-20 00:50:13 -0400, Tom Lane wrote:
> I wrote:
> > Alvaro Herrera <alvherre@2ndquadrant.com> writes:
> >> Tom Lane wrote:
> >>> Hm.  Do you have a more-portable alternative?
> 
> >> I was thinking in a WaitEventSet from latch.c.
> 
> My first reaction was that that sounded like a lot more work than removing
> two lines from maybe_start_bgworker and adjusting some comments.  But on
> closer inspection, the slow-bgworker-start issue isn't the only problem
> here.  On a machine that restarts select()'s timeout after an interrupt,
> as (at least) HPUX does, the postmaster will actually never iterate
> ServerLoop's loop except immediately after receiving a new connection
> request.  The stream of interrupts from the autovac launcher is alone
> sufficient to prevent the initial 60-second timeout from ever elapsing.
> So if there are no new connection requests for awhile, none of the
> housekeeping actions in ServerLoop get done.

FWIW, I vaguely remember somewhat related issues on x86/linux too.  On
busy machines in autovacuum_freeze_max_age territory (pretty frequent
thing these days), the signalling frequency caused problems in
postmaster, but I unfortunately don't remember the symptoms very well.


> So it's looking to me like we do need to do something about this, and
> ideally back-patch it all the way.  But WaitEventSet doesn't exist
> before 9.6.  Do we have the stomach for back-patching that into
> stable branches?

Hm, that's not exactly no code - on the other hand it's (excepting
the select(2) path) reasonably well exercised.  What we could do is to
convert master, see how the beta process likes it, and then backpatch?
These don't like super pressing issues.

- Andres

  13. Re: Unportable implementation of background worker start

    Tom Lane <tgl@sss.pgh.pa.us> — 2017-04-20T23:53:02Z

    I wrote:
> Andres Freund <andres@anarazel.de> writes:
>> On 2017-04-20 19:23:28 -0400, Tom Lane wrote:
>>> or are the HANDLEs in a Windows WaitEventSet not inheritable
>>> resources?

>> So that kind of sounds like it should be doable.

> Ah, good.  I'll add a comment about that and press on.

So ... what would you say to replacing epoll_create() with
epoll_create1(EPOLL_CLOEXEC) ?  Then a WaitEventSet would not
represent inheritable-across-exec resources on any platform,
making it a lot easier to deal with the EXEC_BACKEND case.

AFAIK, both APIs are Linux-only, and epoll_create1() is not much
newer than epoll_create(), so it seems like we'd not be giving up
much portability if we insist on epoll_create1.

			regards, tom lane

  14. Re: Unportable implementation of background worker start

    Andres Freund <andres@anarazel.de> — 2017-04-20T23:59:19Z

    On 2017-04-20 19:53:02 -0400, Tom Lane wrote:
> I wrote:
> > Andres Freund <andres@anarazel.de> writes:
> >> On 2017-04-20 19:23:28 -0400, Tom Lane wrote:
> >>> or are the HANDLEs in a Windows WaitEventSet not inheritable
> >>> resources?
> 
> >> So that kind of sounds like it should be doable.
> 
> > Ah, good.  I'll add a comment about that and press on.
> 
> So ... what would you say to replacing epoll_create() with
> epoll_create1(EPOLL_CLOEXEC) ?  Then a WaitEventSet would not
> represent inheritable-across-exec resources on any platform,
> making it a lot easier to deal with the EXEC_BACKEND case.
> 
> AFAIK, both APIs are Linux-only, and epoll_create1() is not much
> newer than epoll_create(), so it seems like we'd not be giving up
> much portability if we insist on epoll_create1.

I'm generally quite in favor of using CLOEXEC as much as possible in our
tree.  I'm a bit concerned with epoll_create1's availability tho - the
glibc support for it was introduced in 2.9, whereas epoll_create is in
2.3.2.  On the other hand 2.9 was released 2008-11-13.   If we remain
concerned we could just fcntl(fd, F_SETFD, FD_CLOEXEC) instead - that
should only be like three lines more code or such, and should be
available for a lot longer.

- Andres

  15. Re: Unportable implementation of background worker start

    Tom Lane <tgl@sss.pgh.pa.us> — 2017-04-21T00:05:02Z

    Andres Freund <andres@anarazel.de> writes:
> On 2017-04-20 19:53:02 -0400, Tom Lane wrote:
>> So ... what would you say to replacing epoll_create() with
>> epoll_create1(EPOLL_CLOEXEC) ?  Then a WaitEventSet would not
>> represent inheritable-across-exec resources on any platform,
>> making it a lot easier to deal with the EXEC_BACKEND case.

> I'm generally quite in favor of using CLOEXEC as much as possible in our
> tree.  I'm a bit concerned with epoll_create1's availability tho - the
> glibc support for it was introduced in 2.9, whereas epoll_create is in
> 2.3.2.  On the other hand 2.9 was released 2008-11-13.

Also, if it's not there we'd fall back to using plain poll(), which is
not so awful that we need to work hard to avoid it.  I'd just as soon
keep the number of combinations down.

			regards, tom lane

  16. Re: Unportable implementation of background worker start

    Andres Freund <andres@anarazel.de> — 2017-04-21T00:07:21Z

    On 2017-04-20 20:05:02 -0400, Tom Lane wrote:
> Andres Freund <andres@anarazel.de> writes:
> > On 2017-04-20 19:53:02 -0400, Tom Lane wrote:
> >> So ... what would you say to replacing epoll_create() with
> >> epoll_create1(EPOLL_CLOEXEC) ?  Then a WaitEventSet would not
> >> represent inheritable-across-exec resources on any platform,
> >> making it a lot easier to deal with the EXEC_BACKEND case.
> 
> > I'm generally quite in favor of using CLOEXEC as much as possible in our
> > tree.  I'm a bit concerned with epoll_create1's availability tho - the
> > glibc support for it was introduced in 2.9, whereas epoll_create is in
> > 2.3.2.  On the other hand 2.9 was released 2008-11-13.
> 
> Also, if it's not there we'd fall back to using plain poll(), which is
> not so awful that we need to work hard to avoid it.  I'd just as soon
> keep the number of combinations down.

Just using fcntl(SET, CLOEXEC) wound't increase the number of
combinations?

- Andres

  17. Re: Unportable implementation of background worker start

    Tom Lane <tgl@sss.pgh.pa.us> — 2017-04-21T00:10:41Z

    Andres Freund <andres@anarazel.de> writes:
> On 2017-04-20 20:05:02 -0400, Tom Lane wrote:
>> Also, if it's not there we'd fall back to using plain poll(), which is
>> not so awful that we need to work hard to avoid it.  I'd just as soon
>> keep the number of combinations down.

> Just using fcntl(SET, CLOEXEC) wound't increase the number of
> combinations?

True, if you just did it that way unconditionally.  But doesn't that
require an extra kernel call per CreateWaitEventSet()?

			regards, tom lane

  18. Re: Unportable implementation of background worker start

    Andres Freund <andres@anarazel.de> — 2017-04-21T03:00:31Z

    On 2017-04-20 20:10:41 -0400, Tom Lane wrote:
> Andres Freund <andres@anarazel.de> writes:
> > On 2017-04-20 20:05:02 -0400, Tom Lane wrote:
> >> Also, if it's not there we'd fall back to using plain poll(), which is
> >> not so awful that we need to work hard to avoid it.  I'd just as soon
> >> keep the number of combinations down.
> 
> > Just using fcntl(SET, CLOEXEC) wound't increase the number of
> > combinations?
> 
> True, if you just did it that way unconditionally.  But doesn't that
> require an extra kernel call per CreateWaitEventSet()?

It does - the question is whether that matters much.  FE/BE uses a
persistent wait set, but unfortunately much of other latch users
don't. And some of them can be somewhat frequent - so I guess that'd
possibly be measurable.  Ok, so I'm on board with epoll1.

If somebody were to change more frequent latch users to use persistent
wait sets, that'd be good too.

- Andres

  19. Re: Unportable implementation of background worker start

    Tom Lane <tgl@sss.pgh.pa.us> — 2017-04-21T15:09:16Z

    Andres Freund <andres@anarazel.de> writes:
> On 2017-04-20 00:50:13 -0400, Tom Lane wrote:
>> My first reaction was that that sounded like a lot more work than removing
>> two lines from maybe_start_bgworker and adjusting some comments.  But on
>> closer inspection, the slow-bgworker-start issue isn't the only problem
>> here.

> FWIW, I vaguely remember somewhat related issues on x86/linux too.

After sleeping and thinking more, I've realized that the
slow-bgworker-start issue actually exists on *every* platform, it's just
harder to hit when select() is interruptable.  But consider the case
where multiple bgworker-start requests arrive while ServerLoop is
actively executing (perhaps because a connection request just came in).
The postmaster has signals blocked, so nothing happens for the moment.
When we go around the loop and reach

            PG_SETMASK(&UnBlockSig);

the pending SIGUSR1 is delivered, and sigusr1_handler reads all the
bgworker start requests, and services just one of them.  Then control
returns and proceeds to

            selres = select(nSockets, &rmask, NULL, NULL, &timeout);

But now there's no interrupt pending.  So the remaining start requests
do not get serviced until (a) some other postmaster interrupt arrives,
or (b) the one-minute timeout elapses.  They could be waiting awhile.

Bottom line is that any request for more than one bgworker at a time
faces a non-negligible risk of suffering serious latency.

I'm coming back to the idea that at least in the back branches, the
thing to do is allow maybe_start_bgworker to start multiple workers.
Is there any actual evidence for the claim that that might have
bad side effects?

			regards, tom lane

  20. Re: Unportable implementation of background worker start

    Alvaro Herrera <alvherre@2ndquadrant.com> — 2017-04-21T15:19:41Z

    Tom Lane wrote:

> After sleeping and thinking more, I've realized that the
> slow-bgworker-start issue actually exists on *every* platform, it's just
> harder to hit when select() is interruptable.  But consider the case
> where multiple bgworker-start requests arrive while ServerLoop is
> actively executing (perhaps because a connection request just came in).
> The postmaster has signals blocked, so nothing happens for the moment.
> When we go around the loop and reach
> 
>             PG_SETMASK(&UnBlockSig);
> 
> the pending SIGUSR1 is delivered, and sigusr1_handler reads all the
> bgworker start requests, and services just one of them.  Then control
> returns and proceeds to
> 
>             selres = select(nSockets, &rmask, NULL, NULL, &timeout);
> 
> But now there's no interrupt pending.  So the remaining start requests
> do not get serviced until (a) some other postmaster interrupt arrives,
> or (b) the one-minute timeout elapses.  They could be waiting awhile.
> 
> Bottom line is that any request for more than one bgworker at a time
> faces a non-negligible risk of suffering serious latency.

Interesting.  It's hard to hit, for sure.

> I'm coming back to the idea that at least in the back branches, the
> thing to do is allow maybe_start_bgworker to start multiple workers.
>
> Is there any actual evidence for the claim that that might have
> bad side effects?

Well, I ran tests with a few dozen thousand sample workers and the
neglect for other things (such as connection requests) was visible, but
that's probably not a scenario many servers run often currently.  I
don't strongly object to the idea of removing the "return" in older
branches, since it's evidently a problem.  However, as bgworkers start
to be used more, I think we should definitely have some protection.  In
a system with a large number of workers available for parallel queries,
it seems possible for a high velocity server to get stuck in the loop
for some time.  (I haven't actually verified this, though.  My
experiments were with the early kind, static bgworkers.)

-- 
Álvaro Herrera                https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

  21. Re: Unportable implementation of background worker start

    Tom Lane <tgl@sss.pgh.pa.us> — 2017-04-21T16:50:04Z

    Attached is a lightly-tested draft patch that converts the postmaster to
use a WaitEventSet for waiting in ServerLoop.  I've got mixed emotions
about whether this is the direction to proceed, though.  It adds at least
a couple of kernel calls per postmaster signal delivery, and probably to
every postmaster connection acceptance (ServerLoop iteration), to fix
problems that are so corner-casey that we've never even known we had them
till now.

I'm looking longingly at pselect(2), ppoll(2), and epoll_pwait(2), which
would solve the problem without need for a self-pipe.  But the latter two
are Linux-only, and while pselect does exist in recent POSIX editions,
it's nonetheless got portability issues.  Googling suggests that on a
number of platforms, pselect is non-atomic, ie it's nothing but a
wrapper for sigprocmask/select/sigprocmask, which would mean that the
race condition I described in my previous message still exists.

Despite that, a pretty attractive proposal is to do, essentially,

#ifdef HAVE_PSELECT
        selres = pselect(nSockets, &rmask, NULL, NULL, &timeout, &UnBlockSig);
#else
        PG_SETMASK(&UnBlockSig);
        selres = select(nSockets, &rmask, NULL, NULL, &timeout);
        PG_SETMASK(&BlockSig);
#endif

This fixes the race on platforms where pselect exists and is correctly
implemented, and we're no worse off than before where that's not true.

The other component of the problem is the possibility that select() will
restart if the signal is marked SA_RESTART.  (Presumably that would apply
to pselect too.)  I am thinking that maybe the answer is "if it hurts,
don't do it" --- that is, in the postmaster maybe we shouldn't use
SA_RESTART, at least not for these signals.

A different line of thought is to try to provide a bulletproof solution,
but push the implementation problems down into latch.c --- that is, the
goal would be to provide a pselect-like variant of WaitEventSetWait that
is guaranteed to return if interrupted, as opposed to the current behavior
where it's guaranteed not to.  But that seems like quite a bit of work.

Whether or not we decide to change over the postmaster.c code, I think
it'd likely be a good idea to apply most or all of the attached changes
in latch.c.  Setting CLOEXEC on the relevant FDs is clearly a good thing,
and the other changes will provide some safety if some preloaded extension
decides to create a latch in the postmaster process.

			regards, tom lane

  22. Re: Unportable implementation of background worker start

    Andres Freund <andres@anarazel.de> — 2017-04-21T17:20:44Z

    Hi,

On 2017-04-21 12:50:04 -0400, Tom Lane wrote:
> Attached is a lightly-tested draft patch that converts the postmaster to
> use a WaitEventSet for waiting in ServerLoop.  I've got mixed emotions
> about whether this is the direction to proceed, though.  It adds at least
> a couple of kernel calls per postmaster signal delivery, and probably to
> every postmaster connection acceptance (ServerLoop iteration), to fix
> problems that are so corner-casey that we've never even known we had them
> till now.

I'm not concerned much about the signal delivery paths, and I can't
quite imagine that another syscall in the accept path is going to be
measurable - worth ensuring though.

I do agree that it's a bit of a big stick for the back-branches...


> A different line of thought is to try to provide a bulletproof solution,
> but push the implementation problems down into latch.c --- that is, the
> goal would be to provide a pselect-like variant of WaitEventSetWait that
> is guaranteed to return if interrupted, as opposed to the current behavior
> where it's guaranteed not to.  But that seems like quite a bit of work.

Seems like a sane idea to me.  The use of latches has already grown due
to parallelism and it'll likely grow further - some of them seem likely
to also have to deal with such concerns.  I'd much rather centralize
things down to a common place.

On the other hand most types of our processes do SetLatch() in just
nearly all the relevant signal handlers anyway, so they're pretty close
to behaviour already.



> Whether or not we decide to change over the postmaster.c code, I think
> it'd likely be a good idea to apply most or all of the attached changes
> in latch.c.  Setting CLOEXEC on the relevant FDs is clearly a good thing,
> and the other changes will provide some safety if some preloaded extension
> decides to create a latch in the postmaster process.

On the principle, I agree.  Reading through the changes now.


> @@ -1667,76 +1656,64 @@ ServerLoop(void)
>  		 * do nontrivial work.
>  		 *
>  		 * If we are in PM_WAIT_DEAD_END state, then we don't want to accept
> -		 * any new connections, so we don't call select(), and just sleep.
> +		 * any new connections, so we don't call WaitEventSetWait(), and just
> +		 * sleep.  XXX not ideal
>  		 */

Couldn't we just deactive the sockets in the set instead?


>  /*
> - * Initialise the masks for select() for the ports we are listening on.
> - * Return the number of sockets to listen on.
> + * Create a WaitEventSet for ServerLoop() to wait on.  This includes the
> + * sockets we are listening on, plus the PostmasterLatch.
>   */
> -static int
> -initMasks(fd_set *rmask)
> +static void
> +initServerWaitSet(void)
>  {
> -	int			maxsock = -1;
>  	int			i;
>  
> -	FD_ZERO(rmask);
> +	ServerWaitSet = CreateWaitEventSet(PostmasterContext, MAXLISTEN + 1);

Why are we using MAXLISTEN, rather than the actual number of things to
listen to?  The only benefit of this seems to be that we could
theoretically allow dynamic reconfiguration of the sockets a bit more
easily in the future, but that could just as well be done by recreating the set.

Random note: Do we actually have any code that errors out if too many
sockets are being listened to?


> @@ -2553,6 +2543,9 @@ SIGHUP_handler(SIGNAL_ARGS)
>  #endif
>  	}
>  
> +	/* Force ServerLoop to iterate */
> +	SetLatch(&PostmasterLatch);
> +
>  	PG_SETMASK(&UnBlockSig);
>  
>  	errno = save_errno;
> @@ -2724,6 +2717,9 @@ pmdie(SIGNAL_ARGS)
>  			break;
>  	}
>  
> +	/* Force ServerLoop to iterate */
> +	SetLatch(&PostmasterLatch);
> +
>  	PG_SETMASK(&UnBlockSig);
>  
>  	errno = save_errno;
> @@ -3037,6 +3033,9 @@ reaper(SIGNAL_ARGS)
>  	 */
>  	PostmasterStateMachine();
>  
> +	/* Force ServerLoop to iterate */
> +	SetLatch(&PostmasterLatch);
> +
>  	/* Done with signal handler */
>  	PG_SETMASK(&UnBlockSig);
>  
> @@ -5078,6 +5077,9 @@ sigusr1_handler(SIGNAL_ARGS)
>  		signal_child(StartupPID, SIGUSR2);
>  	}
>  
> +	/* Force ServerLoop to iterate */
> +	SetLatch(&PostmasterLatch);
> +
>  	PG_SETMASK(&UnBlockSig);
>  
>  	errno = save_errno;

I kind of would like, in master, take a chance of replace all the work
done in signal handlers, by just a SetLatch(), and do it outside of
signal handlers instead.  Forking from signal handlers is just plain
weird.



> diff --git a/src/backend/storage/ipc/latchindex 4798370..92d2ff0 100644
> --- a/src/backend/storage/ipc/latch.c
> +++ b/src/backend/storage/ipc/latch.c
> @@ -62,6 +62,10 @@
>  #include "storage/pmsignal.h"
>  #include "storage/shmem.h"
>  
> +#ifndef FD_CLOEXEC
> +#define FD_CLOEXEC 1
> +#endif

Hm? Are we sure this is portable?  Is there really cases that have
F_SETFD, but not CLOEXEC?

Greetings,

Andres Freund

  23. Re: Unportable implementation of background worker start

    Tom Lane <tgl@sss.pgh.pa.us> — 2017-04-21T17:38:32Z

    Alvaro Herrera <alvherre@2ndquadrant.com> writes:
> Tom Lane wrote:
>> I'm coming back to the idea that at least in the back branches, the
>> thing to do is allow maybe_start_bgworker to start multiple workers.
>> 
>> Is there any actual evidence for the claim that that might have
>> bad side effects?

> Well, I ran tests with a few dozen thousand sample workers and the
> neglect for other things (such as connection requests) was visible, but
> that's probably not a scenario many servers run often currently.

Indeed.  I'm pretty skeptical that that's an interesting case, and if it
is, the current coding is broken anyway, because with that many workers
you are going to start noticing that running maybe_start_bgworker over
again for each worker is an O(N^2) proposition.  Admittedly, iterating
the loop in maybe_start_bgworker is really cheap compared to a fork(),
but eventually the big-O problem is going to eat your lunch.

> I don't strongly object to the idea of removing the "return" in older
> branches, since it's evidently a problem.  However, as bgworkers start
> to be used more, I think we should definitely have some protection.  In
> a system with a large number of workers available for parallel queries,
> it seems possible for a high velocity server to get stuck in the loop
> for some time.  (I haven't actually verified this, though.  My
> experiments were with the early kind, static bgworkers.)

It might be sensible to limit the number of workers launched per call,
but I think the limit should be quite a bit higher than 1 ... something
like 100 or 1000 might be appropriate.

			regards, tom lane

  24. Re: Unportable implementation of background worker start

    Tom Lane <tgl@sss.pgh.pa.us> — 2017-04-21T17:49:27Z

    Andres Freund <andres@anarazel.de> writes:
> On 2017-04-21 12:50:04 -0400, Tom Lane wrote:
>> Attached is a lightly-tested draft patch that converts the postmaster to
>> use a WaitEventSet for waiting in ServerLoop.  I've got mixed emotions
>> about whether this is the direction to proceed, though.  It adds at least
>> a couple of kernel calls per postmaster signal delivery, and probably to
>> every postmaster connection acceptance (ServerLoop iteration), to fix
>> problems that are so corner-casey that we've never even known we had them
>> till now.

> I'm not concerned much about the signal delivery paths, and I can't
> quite imagine that another syscall in the accept path is going to be
> measurable - worth ensuring though.
> ...
> On the other hand most types of our processes do SetLatch() in just
> nearly all the relevant signal handlers anyway, so they're pretty close
> to behaviour already.

True.  Maybe I'm being too worried.

>> * If we are in PM_WAIT_DEAD_END state, then we don't want to accept
>> -		 * any new connections, so we don't call select(), and just sleep.
>> +		 * any new connections, so we don't call WaitEventSetWait(), and just
>> +		 * sleep.  XXX not ideal
>> */

> Couldn't we just deactive the sockets in the set instead?

Yeah, I think it'd be better to do something like that.  The pg_usleep
call has the same issue of possibly not responding to interrupts.  The
risks are a lot less, since it's a much shorter wait, but I would rather
eliminate the separate code path in favor of doing it honestly.  Didn't
seem like something to fuss over in the first draft though.

>> +	ServerWaitSet = CreateWaitEventSet(PostmasterContext, MAXLISTEN + 1);

> Why are we using MAXLISTEN, rather than the actual number of things to
> listen to?

It'd take more code (ie, an additional scan of the array) to predetermine
that.  I figured the space-per-item in the WaitEventSet wasn't enough to
worry about ... do you think differently?

> Random note: Do we actually have any code that errors out if too many
> sockets are being listened to?

Yes, see StreamServerPort, about line 400.

> I kind of would like, in master, take a chance of replace all the work
> done in signal handlers, by just a SetLatch(), and do it outside of
> signal handlers instead.  Forking from signal handlers is just plain
> weird.

Yeah, maybe it's time.  But in v11, and not for back-patch.

>> +#ifndef FD_CLOEXEC
>> +#define FD_CLOEXEC 1
>> +#endif

> Hm? Are we sure this is portable?  Is there really cases that have
> F_SETFD, but not CLOEXEC?

Copied-and-pasted from our only existing use of FD_CLOEXEC, in libpq.
Might well be obsolete but I see no particular reason not to do it.

			regards, tom lane

  25. Re: Unportable implementation of background worker start

    Tom Lane <tgl@sss.pgh.pa.us> — 2017-04-21T18:08:21Z

    I wrote:
> Andres Freund <andres@anarazel.de> writes:
>> On 2017-04-21 12:50:04 -0400, Tom Lane wrote:
>>> +#ifndef FD_CLOEXEC
>>> +#define FD_CLOEXEC 1
>>> +#endif

>> Hm? Are we sure this is portable?  Is there really cases that have
>> F_SETFD, but not CLOEXEC?

> Copied-and-pasted from our only existing use of FD_CLOEXEC, in libpq.

Looking closer, that code dates to 

    Author: Tom Lane <tgl@sss.pgh.pa.us>
    Branch: master Release: REL8_0_BR [7627b91cd] 2004-10-21 20:23:19 +0000

    Set the close-on-exec flag for libpq's socket to the backend, to avoid
    any possible problems from child programs executed by the client app.
    Per suggestion from Elliot Lee of Red Hat.

and while the public discussion about it

https://www.postgresql.org/message-id/flat/18172.1098382248%40sss.pgh.pa.us

doesn't really say, I suspect the specific coding was Elliot's suggestion
as well.  It probably had some value at one time ... but I see that SUSv2
mandates that fcntl.h provide both F_SETFD and FD_CLOEXEC, so by our own
coding rules it ought to be okay to assume they're there.  I'm tempted to
rip out the quoted bit, as well as the #ifdef F_SETFD, from libpq and see
if anything in the buildfarm complains.

			regards, tom lane

  26. Re: Unportable implementation of background worker start

    Andres Freund <andres@anarazel.de> — 2017-04-21T18:21:24Z

    On 2017-04-21 14:08:21 -0400, Tom Lane wrote:
> but I see that SUSv2
> mandates that fcntl.h provide both F_SETFD and FD_CLOEXEC, so by our own
> coding rules it ought to be okay to assume they're there.  I'm tempted to
> rip out the quoted bit, as well as the #ifdef F_SETFD, from libpq and see
> if anything in the buildfarm complains.

+1

- Andres

  27. Re: Unportable implementation of background worker start

    Andres Freund <andres@anarazel.de> — 2017-04-21T18:40:06Z

    Tom,

On 2017-04-21 13:49:27 -0400, Tom Lane wrote:
> >> * If we are in PM_WAIT_DEAD_END state, then we don't want to accept
> >> -		 * any new connections, so we don't call select(), and just sleep.
> >> +		 * any new connections, so we don't call WaitEventSetWait(), and just
> >> +		 * sleep.  XXX not ideal
> >> */
> 
> > Couldn't we just deactive the sockets in the set instead?
> 
> Yeah, I think it'd be better to do something like that.  The pg_usleep
> call has the same issue of possibly not responding to interrupts.  The
> risks are a lot less, since it's a much shorter wait, but I would rather
> eliminate the separate code path in favor of doing it honestly.  Didn't
> seem like something to fuss over in the first draft though.

Ok, cool.


> >> +	ServerWaitSet = CreateWaitEventSet(PostmasterContext, MAXLISTEN + 1);
> 
> > Why are we using MAXLISTEN, rather than the actual number of things to
> > listen to?
> 
> It'd take more code (ie, an additional scan of the array) to predetermine
> that.  I figured the space-per-item in the WaitEventSet wasn't enough to
> worry about ... do you think differently?

I'm not sure.  We do create an epoll handler with enough space, and that
has some overhead. Don't know whether that's worthwhile to care about.


> > I kind of would like, in master, take a chance of replace all the work
> > done in signal handlers, by just a SetLatch(), and do it outside of
> > signal handlers instead.  Forking from signal handlers is just plain
> > weird.
> 
> Yeah, maybe it's time.  But in v11, and not for back-patch.

Agreed.


- Andres

  28. Re: Unportable implementation of background worker start

    Tom Lane <tgl@sss.pgh.pa.us> — 2017-04-21T18:54:40Z

    Andres Freund <andres@anarazel.de> writes:
> On 2017-04-21 14:08:21 -0400, Tom Lane wrote:
>> but I see that SUSv2
>> mandates that fcntl.h provide both F_SETFD and FD_CLOEXEC, so by our own
>> coding rules it ought to be okay to assume they're there.  I'm tempted to
>> rip out the quoted bit, as well as the #ifdef F_SETFD, from libpq and see
>> if anything in the buildfarm complains.

> +1

Done, we'll soon see what happens.

In the same area, I noticed that POSIX does not say that the success
result for fcntl(F_SETFD) and related cases is 0.  It says that the
failure result is -1 and the success result is some other value.
We seem to have this right in most places, but e.g. port/noblock.c
gets it wrong.  The lack of field complaints implies that just about
everybody actually does return 0 on success, but I still think it
would be a good idea to run around and make all the calls test
specifically for -1.

			regards, tom lane

  29. Re: Unportable implementation of background worker start

    Tom Lane <tgl@sss.pgh.pa.us> — 2017-04-21T20:13:55Z

    Tom Lane <tgl@sss.pgh.pa.us> writes:
> Andres Freund <andres@anarazel.de> writes:
>> On 2017-04-21 14:08:21 -0400, Tom Lane wrote:
>>> but I see that SUSv2
>>> mandates that fcntl.h provide both F_SETFD and FD_CLOEXEC, so by our own
>>> coding rules it ought to be okay to assume they're there.  I'm tempted to
>>> rip out the quoted bit, as well as the #ifdef F_SETFD, from libpq and see
>>> if anything in the buildfarm complains.

>> +1

> Done, we'll soon see what happens.

Should have seen this coming, I guess: some of the Windows critters are
falling over, apparently because they lack fcntl() altogether.  So the
#ifdef F_SETFD was really acting as a proxy for "#ifdef HAVE_FCNTL".

There's no HAVE_FCNTL test in configure ATM, and I'm thinking it would
be pretty silly to add one, since surely it would succeed on anything
Unix-y enough to run the configure script.

I'm guessing the best thing to do is put back #ifdef F_SETFD;
alternatively we might spell it like "#ifndef WIN32", but I'm unsure
if that'd do what we want on Cygwin or MinGW.

In non-Windows code paths in latch.c, we probably wouldn't need to
bother with #ifdef F_SETFD.

Hopefully we can leave in the removal of "#define FD_CLOEXEC".
Will wait a bit longer for more results.

			regards, tom lane

  30. Re: Unportable implementation of background worker start

    Tom Lane <tgl@sss.pgh.pa.us> — 2017-04-21T22:28:47Z

    Alvaro Herrera <alvherre@2ndquadrant.com> writes:
> Tom Lane wrote:
>> It also appears to me that do_start_bgworker's treatment of fork
>> failure is completely brain dead.  Did anyone really think about
>> that case?

> Hmm, I probably modelled it on autovacuum without giving that case much
> additional consideration.

Attached is a proposed patch that should make fork failure behave
sanely, ie it works much the same as a worker crash immediately after
launch.  I also refactored things a bit to make do_start_bgworker
fully responsible for updating the RegisteredBgWorker's state,
rather than doing just some of it as before.

I tested this by hot-wiring the fork_process call to fail some of
the time, which showed that the postmaster now seems to recover OK,
but parallel.c's logic is completely innocent of the idea that
worker-startup failure is possible.  The leader backend just freezes,
and nothing short of kill -9 on that backend will get you out of it.
Fixing that seems like material for a separate patch though.

			regards, tom lane

  31. Re: Unportable implementation of background worker start

    Tom Lane <tgl@sss.pgh.pa.us> — 2017-04-22T03:50:41Z

    I wrote:
> Attached is a lightly-tested draft patch that converts the postmaster to
> use a WaitEventSet for waiting in ServerLoop.  I've got mixed emotions
> about whether this is the direction to proceed, though.

Attached are a couple of patches that represent a plausible Plan B.
The first one changes the postmaster to run its signal handlers without
specifying SA_RESTART.  I've confirmed that that seems to fix the
select_parallel-test-takes-a-long-time problem on gaur/pademelon.
The second one uses pselect, if available, to replace the unblock-signals/
select()/block-signals dance in ServerLoop.  On platforms where pselect
exists and works properly, that should fix the race condition I described
previously.  On platforms where it doesn't, we're no worse off than
before.

As mentioned in the comments for the second patch, even if we don't
have working pselect(), the only problem is that ServerLoop's response
to an interrupt might be delayed by as much as the up-to-1-minute timeout.
The only existing case where that's really bad is launching multiple
bgworkers.  I would therefore advocate also changing maybe_start_bgworker
to start up to N bgworkers per call, where N is large enough to pretty
much always satisfy simultaneously-arriving requests.  I'd pick 100 or
so, but am willing to negotiate.

I think that these patches represent something we could back-patch
without a lot of trepidation, unlike the WaitEventSet-based approach.
Therefore, my proposal is to apply and backpatch these changes, and
call it good for v10.  For v11, we could work on changing the postmaster
to not do work in signal handlers, as discussed upthread.  That would
supersede these two patches completely, though I'd still advocate for
keeping the change in maybe_start_bgworker.

Note: for testing purposes, these patches are quite independent; just
ignore the hunk in the second patch that changes a comment added by
the first one.

			regards, tom lane

  32. Re: Unportable implementation of background worker start

    Andres Freund <andres@anarazel.de> — 2017-04-22T05:54:44Z

    On 2017-04-21 23:50:41 -0400, Tom Lane wrote:
> Attached are a couple of patches that represent a plausible Plan B.
> The first one changes the postmaster to run its signal handlers without
> specifying SA_RESTART.  I've confirmed that that seems to fix the
> select_parallel-test-takes-a-long-time problem on gaur/pademelon.

> The second one uses pselect, if available, to replace the unblock-signals/
> select()/block-signals dance in ServerLoop.  On platforms where pselect
> exists and works properly, that should fix the race condition I described
> previously.  On platforms where it doesn't, we're no worse off than
> before.

We probably should note somewhere prominently that pselect isn't
actually race-free on a number of platforms.


> I think that these patches represent something we could back-patch
> without a lot of trepidation, unlike the WaitEventSet-based approach.
> Therefore, my proposal is to apply and backpatch these changes, and
> call it good for v10.  For v11, we could work on changing the postmaster
> to not do work in signal handlers, as discussed upthread.  That would
> supersede these two patches completely, though I'd still advocate for
> keeping the change in maybe_start_bgworker.

Not yet having looked at your patches, that sounds like a reasonable
plan.  I'd still like to get something like your CLOEXEC patch applied
independently however.


< patches >

Looks reasonable on a quick skim.


Greetings,

Andres Freund

  33. Re: Unportable implementation of background worker start

    Andres Freund <andres@anarazel.de> — 2017-04-24T20:16:44Z

    On 2017-04-21 23:50:41 -0400, Tom Lane wrote:
> I wrote:
> > Attached is a lightly-tested draft patch that converts the postmaster to
> > use a WaitEventSet for waiting in ServerLoop.  I've got mixed emotions
> > about whether this is the direction to proceed, though.
> 
> Attached are a couple of patches that represent a plausible Plan B.
> The first one changes the postmaster to run its signal handlers without
> specifying SA_RESTART.  I've confirmed that that seems to fix the
> select_parallel-test-takes-a-long-time problem on gaur/pademelon.
> The second one uses pselect, if available, to replace the unblock-signals/
> select()/block-signals dance in ServerLoop.  On platforms where pselect
> exists and works properly, that should fix the race condition I described
> previously.  On platforms where it doesn't, we're no worse off than
> before.
> 
> As mentioned in the comments for the second patch, even if we don't
> have working pselect(), the only problem is that ServerLoop's response
> to an interrupt might be delayed by as much as the up-to-1-minute timeout.
> The only existing case where that's really bad is launching multiple
> bgworkers.  I would therefore advocate also changing maybe_start_bgworker
> to start up to N bgworkers per call, where N is large enough to pretty
> much always satisfy simultaneously-arriving requests.  I'd pick 100 or
> so, but am willing to negotiate.
> 
> I think that these patches represent something we could back-patch
> without a lot of trepidation, unlike the WaitEventSet-based approach.
> Therefore, my proposal is to apply and backpatch these changes, and
> call it good for v10.  For v11, we could work on changing the postmaster
> to not do work in signal handlers, as discussed upthread.  That would
> supersede these two patches completely, though I'd still advocate for
> keeping the change in maybe_start_bgworker.
> 
> Note: for testing purposes, these patches are quite independent; just
> ignore the hunk in the second patch that changes a comment added by
> the first one.

Unclear if related, but
https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=gharial&dt=2017-04-24%2019%3A30%3A42
has a suspicious timing of failing in a weird way.

- Andres

  34. Re: Unportable implementation of background worker start

    Andres Freund <andres@anarazel.de> — 2017-04-24T20:24:16Z

    On 2017-04-24 13:16:44 -0700, Andres Freund wrote:
> Unclear if related, but
> https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=gharial&dt=2017-04-24%2019%3A30%3A42
> has a suspicious timing of failing in a weird way.

Given that gharial is also failing on 9.6 (same set of commits) and
coypu fails (again same set) on 9.6
https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=coypu&dt=2017-04-24%2018%3A20%3A33

I'm afraid it's more likely to be related.

gharial & coypu owners, any chance you could try starting postgres with
log_min_messages=debug5 on one of the affected machines?

- Andres

  35. Re: Unportable implementation of background worker start

    Tom Lane <tgl@sss.pgh.pa.us> — 2017-04-24T21:33:39Z

    Andres Freund <andres@anarazel.de> writes:
> On 2017-04-24 13:16:44 -0700, Andres Freund wrote:
>> Unclear if related, but
>> https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=gharial&dt=2017-04-24%2019%3A30%3A42
>> has a suspicious timing of failing in a weird way.

> Given that gharial is also failing on 9.6 (same set of commits) and
> coypu fails (again same set) on 9.6
> https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=coypu&dt=2017-04-24%2018%3A20%3A33

coypu's problem is unrelated:

running bootstrap script ... 2017-04-24 23:04:53.084 CEST [21114] FATAL:  could not create semaphores: No space left on device
2017-04-24 23:04:53.084 CEST [21114] DETAIL:  Failed system call was semget(1, 17, 03600).

but it does seem likely that one of these patches broke gharial.
That's pretty annoying :-(

			regards, tom lane

  36. Re: Unportable implementation of background worker start

    Andres Freund <andres@anarazel.de> — 2017-04-24T21:38:07Z

    On 2017-04-24 17:33:39 -0400, Tom Lane wrote:
> Andres Freund <andres@anarazel.de> writes:
> > On 2017-04-24 13:16:44 -0700, Andres Freund wrote:
> >> Unclear if related, but
> >> https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=gharial&dt=2017-04-24%2019%3A30%3A42
> >> has a suspicious timing of failing in a weird way.
> 
> > Given that gharial is also failing on 9.6 (same set of commits) and
> > coypu fails (again same set) on 9.6
> > https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=coypu&dt=2017-04-24%2018%3A20%3A33
> 
> coypu's problem is unrelated:
> 
> running bootstrap script ... 2017-04-24 23:04:53.084 CEST [21114] FATAL:  could not create semaphores: No space left on device
> 2017-04-24 23:04:53.084 CEST [21114] DETAIL:  Failed system call was semget(1, 17, 03600).

Note I was linking the 9.6 report form coypu, not HEAD. Afaics the 9.6
failure is the same as gharial's mode of failure.

- Andres

  37. Re: Unportable implementation of background worker start

    Tom Lane <tgl@sss.pgh.pa.us> — 2017-04-24T22:14:41Z

    Andres Freund <andres@anarazel.de> writes:
> On 2017-04-24 17:33:39 -0400, Tom Lane wrote:
>> coypu's problem is unrelated:

> Note I was linking the 9.6 report form coypu, not HEAD. Afaics the 9.6
> failure is the same as gharial's mode of failure.

[ looks closer... ]  Oh: the 9.6 run occurred first, and the failures on
HEAD and 9.5 are presumably follow-on damage because the stuck postmaster
hasn't released semaphores.

A bit of googling establishes that NetBSD 5.1 has a broken pselect
implementation:

http://gnats.netbsd.org/cgi-bin/query-pr-single.pl?number=43625

That says they fixed it in later versions but not 5.1 :-(

I can't find any similar smoking gun on the web for HPUX, but
I'd fully expect their bug database to be behind a paywall.

What I'm inclined to do is to revert the pselect change but not the other,
to see if that fixes these two animals.  If it does, we could look into
blacklisting these particular platforms when choosing pselect.

			regards, tom lane

  38. Re: Unportable implementation of background worker start

    Andres Freund <andres@anarazel.de> — 2017-04-24T22:19:01Z

    On 2017-04-24 18:14:41 -0400, Tom Lane wrote:
> Andres Freund <andres@anarazel.de> writes:
> > On 2017-04-24 17:33:39 -0400, Tom Lane wrote:
> >> coypu's problem is unrelated:
> 
> > Note I was linking the 9.6 report form coypu, not HEAD. Afaics the 9.6
> > failure is the same as gharial's mode of failure.
> 
> [ looks closer... ]  Oh: the 9.6 run occurred first, and the failures on
> HEAD and 9.5 are presumably follow-on damage because the stuck postmaster
> hasn't released semaphores.
> 
> A bit of googling establishes that NetBSD 5.1 has a broken pselect
> implementation:
> 
> http://gnats.netbsd.org/cgi-bin/query-pr-single.pl?number=43625

Yikes.  Do I understand correctly that they effectively just mapped
pselect to select?


> What I'm inclined to do is to revert the pselect change but not the other,
> to see if that fixes these two animals.  If it does, we could look into
> blacklisting these particular platforms when choosing pselect.

Seems sensible.

- Andres

  39. Re: Unportable implementation of background worker start

    Tom Lane <tgl@sss.pgh.pa.us> — 2017-04-24T22:20:44Z

    Andres Freund <andres@anarazel.de> writes:
> On 2017-04-24 18:14:41 -0400, Tom Lane wrote:
>> A bit of googling establishes that NetBSD 5.1 has a broken pselect
>> implementation:
>> 
>> http://gnats.netbsd.org/cgi-bin/query-pr-single.pl?number=43625

> Yikes.  Do I understand correctly that they effectively just mapped
> pselect to select?

That's what it sounds like.  Probably not intentionally, but in effect.

>> What I'm inclined to do is to revert the pselect change but not the other,
>> to see if that fixes these two animals.  If it does, we could look into
>> blacklisting these particular platforms when choosing pselect.

> Seems sensible.

Will do.

			regards, tom lane

  40. Re: Unportable implementation of background worker start

    Tom Lane <tgl@sss.pgh.pa.us> — 2017-04-24T23:47:09Z

    I wrote:
> What I'm inclined to do is to revert the pselect change but not the other,
> to see if that fixes these two animals.  If it does, we could look into
> blacklisting these particular platforms when choosing pselect.

It looks like coypu is going to need manual intervention (ie, kill -9
on the leftover postmaster) to get unwedged :-(.  That's particularly
disturbing because it implies that ServerLoop isn't iterating at all;
otherwise, it'd have noticed by now that the buildfarm script deleted
its data directory out from under it.  Even if NetBSD's pselect had
forgotten to unblock signals, you'd figure it'd time out after a
minute ... so it's even more broken than that.

			regards, tom lane

  41. Re: Unportable implementation of background worker start

    Rémi Zara <remi_zara@mac.com> — 2017-04-25T05:53:02Z

    > Le 25 avr. 2017 à 01:47, Tom Lane <tgl@sss.pgh.pa.us> a écrit :
> 
> I wrote:
>> What I'm inclined to do is to revert the pselect change but not the other,
>> to see if that fixes these two animals.  If it does, we could look into
>> blacklisting these particular platforms when choosing pselect.
> 
> It looks like coypu is going to need manual intervention (ie, kill -9
> on the leftover postmaster) to get unwedged :-(.  That's particularly
> disturbing because it implies that ServerLoop isn't iterating at all;
> otherwise, it'd have noticed by now that the buildfarm script deleted
> its data directory out from under it.  Even if NetBSD's pselect had
> forgotten to unblock signals, you'd figure it'd time out after a
> minute ... so it's even more broken than that.
> 

Hi,

coypu was not stuck (no buildfarm related process running), but failed to clean-up shared memory and semaphores.
I’ve done the clean-up.

Regards,

Rémi

  42. Re: Unportable implementation of background worker start

    Tom Lane <tgl@sss.pgh.pa.us> — 2017-04-25T15:57:30Z

    =?utf-8?Q?R=C3=A9mi_Zara?= <remi_zara@mac.com> writes:
>> Le 25 avr. 2017 à 01:47, Tom Lane <tgl@sss.pgh.pa.us> a écrit :
>> It looks like coypu is going to need manual intervention (ie, kill -9
>> on the leftover postmaster) to get unwedged :-(.  That's particularly
>> disturbing because it implies that ServerLoop isn't iterating at all;
>> otherwise, it'd have noticed by now that the buildfarm script deleted
>> its data directory out from under it.

> coypu was not stuck (no buildfarm related process running), but failed to clean-up shared memory and semaphores.
> I’ve done the clean-up.

Huh, that's even more interesting.

Looking at the code, what ServerLoop actually does when it notices that
the postmaster.pid file has been removed is

				kill(MyProcPid, SIGQUIT);

So if our hypothesis is that pselect() failed to unblock signals,
then failure to quit is easily explained: the postmaster never
received/acted on its own signal.  But that should have left you
with a running postmaster holding the shared memory and semaphores.
Seems like if it is gone but it failed to remove those, somebody must've
kill -9'd it ... but who?  I see nothing in the buildfarm script that
would.

			regards, tom lane

  43. Re: Unportable implementation of background worker start

    Tom Lane <tgl@sss.pgh.pa.us> — 2017-04-26T15:42:38Z

    I wrote:
> =?utf-8?Q?R=C3=A9mi_Zara?= <remi_zara@mac.com> writes:
>> coypu was not stuck (no buildfarm related process running), but failed to clean-up shared memory and semaphores.
>> I’ve done the clean-up.

> Huh, that's even more interesting.

I installed NetBSD 5.1.5 on an old Mac G4; I believe this is a reasonable
approximation to coypu's environment.  With the pselect patch installed,
I can replicate the behavior we saw in the buildfarm of connections
immediately failing with "the database system is starting up".
Investigation shows that pselect reports ready sockets correctly (which is
what allows connections to get in at all), and it does stop waiting either
for a signal or for a timeout.  What it forgets to do is to actually
service the signal.  The observed behavior is caused by the fact that
reaper() is never called so the postmaster never realizes that the startup
process has finished.

I experimented with putting

			PG_SETMASK(&UnBlockSig);
			PG_SETMASK(&BlockSig);

immediately after the pselect() call, and found that indeed that lets
signals get serviced, and things work pretty much normally.

However, closer inspection finds that pselect only stops waiting when
a signal arrives *while it's waiting*, not if there was a signal already
pending.  So this is actually even more broken than the so called "non
atomic" behavior we had expected to see --- at least with that, the
pending signal would have gotten serviced promptly, even if ServerLoop
itself didn't iterate.

This is all giving me less than warm fuzzy feelings about the state of
pselect support out in the real world.

So at this point we seem to have three plausible alternatives:

1. Let HEAD stand as it is.  We have a problem with slow response to
bgworker start requests that arrive while ServerLoop is active, but that's
a pretty tight window usually (although I believe I've seen it hit at
least once in testing).

2. Reinstall the pselect patch, blacklisting NetBSD and HPUX and whatever
else we find to be flaky.  Then only the blacklisted platforms have the
problem.

3. Go ahead with converting the postmaster to use WaitEventSet, a la
the draft patch I posted earlier.  I'd be happy to do this if we were
at the start of a devel cycle, but right now seems a bit late --- not
to mention that we really need to fix 9.6 as well.

We could substantially ameliorate the slow-response problem by allowing
maybe_start_bgworker to launch multiple workers per call, which is
something I think we should do regardless.  (I have a patch written to
allow it to launch up to N workers per call, but have held off committing
that till after the dust settles in ServerLoop.)

I'm leaning to doing #1 plus the maybe_start_bgworker change.  There's
certainly room for difference of opinion here, though.  Thoughts?

			regards, tom lane

  44. Re: Unportable implementation of background worker start

    Tom Lane <tgl@sss.pgh.pa.us> — 2017-04-26T21:05:39Z

    Andres Freund <andres@anarazel.de> writes:
> I'd still like to get something like your CLOEXEC patch applied
> independently however.

Here's an updated version of that, which makes use of our previous
conclusion that F_SETFD/FD_CLOEXEC are available everywhere except
Windows, and fixes some sloppy thinking about the EXEC_BACKEND case.

I went ahead and changed the call to epoll_create into epoll_create1.
I'm not too concerned about loss of portability there --- it seems
unlikely that many people are still using ten-year-old glibc, and
even less likely that any of them would be interested in running
current Postgres on their stable-unto-death platform.  We could add
a configure test for epoll_create1 if you feel one's needed, but
I think it'd just be a waste of cycles.

I propose to push this into HEAD and 9.6 too.

			regards, tom lane

  45. Re: Unportable implementation of background worker start

    Andres Freund <andres@anarazel.de> — 2017-04-27T00:37:40Z

    On 2017-04-26 11:42:38 -0400, Tom Lane wrote:
> 1. Let HEAD stand as it is.  We have a problem with slow response to
> bgworker start requests that arrive while ServerLoop is active, but that's
> a pretty tight window usually (although I believe I've seen it hit at
> least once in testing).
> 
> 2. Reinstall the pselect patch, blacklisting NetBSD and HPUX and whatever
> else we find to be flaky.  Then only the blacklisted platforms have the
> problem.

That seems unattractive at this point.  I'm not looking forward to
having to debug more random platforms that implement this badly in a
yet another weird way.


> 3. Go ahead with converting the postmaster to use WaitEventSet, a la
> the draft patch I posted earlier.  I'd be happy to do this if we were
> at the start of a devel cycle, but right now seems a bit late --- not
> to mention that we really need to fix 9.6 as well.

Yea, backpatching this to 9.6 seems like a bigger hammer than
appropriate.  I'm on the fence WRT master, I think there's an argument
to be made that this is going to become a bigger and bigger problem, and
that we'll wish in a year or two that we had fewer releases with
parallelism etc that don't use WaitEventSets.


> I'm leaning to doing #1 plus the maybe_start_bgworker change.  There's
> certainly room for difference of opinion here, though.  Thoughts?

I see you did the bgworker thing - that seems good to me.

Thanks,

Andres

  46. Re: Unportable implementation of background worker start

    Robert Haas <robertmhaas@gmail.com> — 2017-04-27T00:58:31Z

    On Wed, Apr 26, 2017 at 8:37 PM, Andres Freund <andres@anarazel.de> wrote:
>> 3. Go ahead with converting the postmaster to use WaitEventSet, a la
>> the draft patch I posted earlier.  I'd be happy to do this if we were
>> at the start of a devel cycle, but right now seems a bit late --- not
>> to mention that we really need to fix 9.6 as well.
>
> Yea, backpatching this to 9.6 seems like a bigger hammer than
> appropriate.  I'm on the fence WRT master, I think there's an argument
> to be made that this is going to become a bigger and bigger problem, and
> that we'll wish in a year or two that we had fewer releases with
> parallelism etc that don't use WaitEventSets.

I think changing this might be wise.  This problem isn't going away
for real until we do this, right?

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

  47. Re: Unportable implementation of background worker start

    Andres Freund <andres@anarazel.de> — 2017-04-27T01:10:33Z

    On 2017-04-26 17:05:39 -0400, Tom Lane wrote:
> Here's an updated version of that, which makes use of our previous
> conclusion that F_SETFD/FD_CLOEXEC are available everywhere except
> Windows, and fixes some sloppy thinking about the EXEC_BACKEND case.
> 
> I went ahead and changed the call to epoll_create into epoll_create1.
> I'm not too concerned about loss of portability there --- it seems
> unlikely that many people are still using ten-year-old glibc, and
> even less likely that any of them would be interested in running
> current Postgres on their stable-unto-death platform.  We could add
> a configure test for epoll_create1 if you feel one's needed, but
> I think it'd just be a waste of cycles.

Yea, I think we can live with that.  If we find it's a problem, we can
add a configure test later.


> I propose to push this into HEAD and 9.6 too.

Cool.


Change looks good to me.


Greetings,

Andres Freund

  48. Re: Unportable implementation of background worker start

    Tom Lane <tgl@sss.pgh.pa.us> — 2017-04-27T01:38:38Z

    Robert Haas <robertmhaas@gmail.com> writes:
> On Wed, Apr 26, 2017 at 8:37 PM, Andres Freund <andres@anarazel.de> wrote:
>>> 3. Go ahead with converting the postmaster to use WaitEventSet, a la
>>> the draft patch I posted earlier.  I'd be happy to do this if we were
>>> at the start of a devel cycle, but right now seems a bit late --- not
>>> to mention that we really need to fix 9.6 as well.

>> Yea, backpatching this to 9.6 seems like a bigger hammer than
>> appropriate.  I'm on the fence WRT master, I think there's an argument
>> to be made that this is going to become a bigger and bigger problem, and
>> that we'll wish in a year or two that we had fewer releases with
>> parallelism etc that don't use WaitEventSets.

> I think changing this might be wise.  This problem isn't going away
> for real until we do this, right?

Sure, but we have a lot of other problems that aren't going away until
we fix them, either.  This patch smells like new development to me ---
and it's not even very complete, because really what Andres wants to do
(and I concur) is to get rid of the postmaster's habit of doing
interesting things in signal handlers.  I'm definitely not on board with
doing that for v10 at this point.  But if we apply this patch, and then
do that in v11, then v10 will look like neither earlier nor later branches
with respect to the postmaster's event wait mechanisms.  I think that's
a recipe for undue maintenance pain.

I believe that the already-committed patches represent a sufficient-for-now
response to the known performance problems here, and so I'm thinking we
should stop here for v10.  I'm okay with pushing the latch.c changes
I just proposed, because those provide a useful safeguard against
extension modules doing something exciting in the postmaster process.
But I don't think we should go much further than that for v10.

			regards, tom lane

  49. Re: Unportable implementation of background worker start

    Tom Lane <tgl@sss.pgh.pa.us> — 2017-04-27T20:35:29Z

    Andres Freund <andres@anarazel.de> writes:
> On 2017-04-26 17:05:39 -0400, Tom Lane wrote:
>> I went ahead and changed the call to epoll_create into epoll_create1.
>> I'm not too concerned about loss of portability there --- it seems
>> unlikely that many people are still using ten-year-old glibc, and
>> even less likely that any of them would be interested in running
>> current Postgres on their stable-unto-death platform.  We could add
>> a configure test for epoll_create1 if you feel one's needed, but
>> I think it'd just be a waste of cycles.

> Yea, I think we can live with that.  If we find it's a problem, we can
> add a configure test later.

Well, according to the buildfarm, "later" is "now" :-(.

If RHEL5 is too old to have epoll_create1, I think your dates for it
might be a bit off.  Anyway, I'll go do something about that in a
little bit.

It looks like it might be sufficient to do "#ifdef EPOLL_CLOEXEC"
in latch.c, rather than bothering with a full-blown configure check.

			regards, tom lane

  50. Re: Unportable implementation of background worker start

    Andres Freund <andres@anarazel.de> — 2017-04-27T20:45:05Z

    On 2017-04-27 16:35:29 -0400, Tom Lane wrote:
> Andres Freund <andres@anarazel.de> writes:
> > On 2017-04-26 17:05:39 -0400, Tom Lane wrote:
> >> I went ahead and changed the call to epoll_create into epoll_create1.
> >> I'm not too concerned about loss of portability there --- it seems
> >> unlikely that many people are still using ten-year-old glibc, and
> >> even less likely that any of them would be interested in running
> >> current Postgres on their stable-unto-death platform.  We could add
> >> a configure test for epoll_create1 if you feel one's needed, but
> >> I think it'd just be a waste of cycles.
> 
> > Yea, I think we can live with that.  If we find it's a problem, we can
> > add a configure test later.
> 
> Well, according to the buildfarm, "later" is "now" :-(.

Too bad.


> If RHEL5 is too old to have epoll_create1, I think your dates for it
> might be a bit off.  Anyway, I'll go do something about that in a
> little bit.

2008-11-13 is when glibc 2.9 was released. Appears that RHEL5 still
ships with glibc 2.5, released 2006-09-29. Given that RHEL 5 originally
was released 2007-03-05, that's not too surprising?


> It looks like it might be sufficient to do "#ifdef EPOLL_CLOEXEC"
> in latch.c, rather than bothering with a full-blown configure check.

Yea, that sounds worth trying.  Wonder if we need to care about kernels
not supporting it, but glibc having support?  I'd be ok skimping on that
for now.

Greetings,

Andres Freund

  51. Re: Unportable implementation of background worker start

    Tom Lane <tgl@sss.pgh.pa.us> — 2017-04-27T21:06:24Z

    Andres Freund <andres@anarazel.de> writes:
> On 2017-04-27 16:35:29 -0400, Tom Lane wrote:
>> It looks like it might be sufficient to do "#ifdef EPOLL_CLOEXEC"
>> in latch.c, rather than bothering with a full-blown configure check.

> Yea, that sounds worth trying.  Wonder if we need to care about kernels
> not supporting it, but glibc having support?  I'd be ok skimping on that
> for now.

On my RHEL6 box, <sys/epoll.h> is provided by glibc not the kernel:

$ rpm -qf /usr/include/sys/epoll.h
glibc-headers-2.12-1.209.el6_9.1.x86_64

So I think it's probably safe to assume that the header is in sync
with what glibc can do.

As for kernel (much) older than glibc, I'd rather expect glibc to paper
over that, though I've not looked at the source code to be sure.

			regards, tom lane

  52. Re: [HACKERS] Unportable implementation of background worker start

    Andres Freund <andres@anarazel.de> — 2019-02-11T11:07:03Z

    Hi,

On 2017-04-26 11:42:38 -0400, Tom Lane wrote:
> 3. Go ahead with converting the postmaster to use WaitEventSet, a la
> the draft patch I posted earlier.  I'd be happy to do this if we were
> at the start of a devel cycle, but right now seems a bit late --- not
> to mention that we really need to fix 9.6 as well.

Btw, recent-ish versions of
http://man7.org/linux/man-pages/man7/signal-safety.7.html
have
       *  POSIX.1-2003 clarified that if an application calls fork(2) from a
          signal handler and any of the fork handlers registered by
          pthread_atfork(3) calls a function that is not async-signal-safe,
          the behavior is undefined.  A future revision of the standard is
          likely to remove fork(2) from the list of async-signal-safe
          functions.

Greetings,

Andres Freund