pg.ddx.io pgsql-hackers@postgresql.org mailing list archive
help / color / mirror / Atom feed"could not reattach to shared memory" on buildfarm member dory
33+ messages / 8 participants
[nested] [flat]
* "could not reattach to shared memory" on buildfarm member dory
@ 2018-04-23 21:10 Tom Lane <tgl@sss.pgh.pa.us>
0 siblings, 1 reply; 33+ messages in thread
From: Tom Lane @ 2018-04-23 21:10 UTC (permalink / raw)
To: Stephen Frost <sfrost@snowman.net>; +Cc: pgsql-hackers@lists.postgresql.org
So far, dory has failed three times with essentially identical symptoms:
2018-04-23 19:57:10.624 GMT [2240] FATAL: could not reattach to shared memory (key=0000000000000190, addr=00000000018E0000): error code 487
2018-04-23 15:57:10.657 EDT [8836] ERROR: lost connection to parallel worker
2018-04-23 15:57:10.657 EDT [8836] STATEMENT: select count(*) from tenk1 group by twenty;
2018-04-23 15:57:10.660 EDT [3820] LOG: background worker "parallel worker" (PID 2240) exited with exit code 1
Now how can this be? We've successfully reserved and released the address
range we want to use, so it *should* be free at the instant we try to map.
Another thing that seems curious, though it may just be an artifact of
not having many data points yet, is that these failures all occurred
during pg_upgradecheck. You'd think the "check" and "install-check"
steps would be equally vulnerable to the failure.
I guess the good news is that we're seeing this in a reasonably
reproducible fashion, so there's some hope of digging down to find
out the actual cause.
regards, tom lane
^ permalink raw reply [nested|flat] 33+ messages in thread
* Re: "could not reattach to shared memory" on buildfarm member dory
@ 2018-04-23 23:18 Stephen Frost <sfrost@snowman.net>
parent: Tom Lane <tgl@sss.pgh.pa.us>
0 siblings, 2 replies; 33+ messages in thread
From: Stephen Frost @ 2018-04-23 23:18 UTC (permalink / raw)
To: Tom Lane <tgl@sss.pgh.pa.us>; +Cc: pgsql-hackers@lists.postgresql.org
Greetings,
* Tom Lane (tgl@sss.pgh.pa.us) wrote:
> So far, dory has failed three times with essentially identical symptoms:
>
> 2018-04-23 19:57:10.624 GMT [2240] FATAL: could not reattach to shared memory (key=0000000000000190, addr=00000000018E0000): error code 487
> 2018-04-23 15:57:10.657 EDT [8836] ERROR: lost connection to parallel worker
> 2018-04-23 15:57:10.657 EDT [8836] STATEMENT: select count(*) from tenk1 group by twenty;
> 2018-04-23 15:57:10.660 EDT [3820] LOG: background worker "parallel worker" (PID 2240) exited with exit code 1
>
> Now how can this be? We've successfully reserved and released the address
> range we want to use, so it *should* be free at the instant we try to map.
Yeah, that's definitely interesting.
> I guess the good news is that we're seeing this in a reasonably
> reproducible fashion, so there's some hope of digging down to find
> out the actual cause.
I've asked Heath to take a look at the system again and see if there's
any Windows logs or such that might help us understand what's happening.
AV was disabled on the box, so don't think it's that, at least.
Thanks!
Stephen
Attachments:
[application/pgp-signature] signature.asc (818B, ../../20180423231855.GK27724@tamriel.snowman.net/2-signature.asc)
download
^ permalink raw reply [nested|flat] 33+ messages in thread
* Re: "could not reattach to shared memory" on buildfarm member dory
@ 2018-04-23 23:37 Thomas Munro <thomas.munro@enterprisedb.com>
parent: Stephen Frost <sfrost@snowman.net>
1 sibling, 1 reply; 33+ messages in thread
From: Thomas Munro @ 2018-04-23 23:37 UTC (permalink / raw)
To: Stephen Frost <sfrost@snowman.net>; +Cc: Tom Lane <tgl@sss.pgh.pa.us>; PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
On Tue, Apr 24, 2018 at 11:18 AM, Stephen Frost <sfrost@snowman.net> wrote:
> Greetings,
>
> * Tom Lane (tgl@sss.pgh.pa.us) wrote:
>> So far, dory has failed three times with essentially identical symptoms:
>>
>> 2018-04-23 19:57:10.624 GMT [2240] FATAL: could not reattach to shared memory (key=0000000000000190, addr=00000000018E0000): error code 487
>> 2018-04-23 15:57:10.657 EDT [8836] ERROR: lost connection to parallel worker
>> 2018-04-23 15:57:10.657 EDT [8836] STATEMENT: select count(*) from tenk1 group by twenty;
>> 2018-04-23 15:57:10.660 EDT [3820] LOG: background worker "parallel worker" (PID 2240) exited with exit code 1
>>
>> Now how can this be? We've successfully reserved and released the address
>> range we want to use, so it *should* be free at the instant we try to map.
>
> Yeah, that's definitely interesting.
I wondered if another thread with the right timing could map something
between the VirtualFree() and MapViewOfFileEx() calls, but we don't
create the Windows signal handling thread until a bit later. Could
there be any any other threads active in the process?
Maybe try asking what's mapped there with VirtualQueryEx() on failure?
--
Thomas Munro
http://www.enterprisedb.com
^ permalink raw reply [nested|flat] 33+ messages in thread
* Re: "could not reattach to shared memory" on buildfarm member dory
@ 2018-04-24 07:06 Noah Misch <noah@leadboat.com>
parent: Thomas Munro <thomas.munro@enterprisedb.com>
0 siblings, 2 replies; 33+ messages in thread
From: Noah Misch @ 2018-04-24 07:06 UTC (permalink / raw)
To: Thomas Munro <thomas.munro@enterprisedb.com>; +Cc: Stephen Frost <sfrost@snowman.net>; Tom Lane <tgl@sss.pgh.pa.us>; PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
On Tue, Apr 24, 2018 at 11:37:33AM +1200, Thomas Munro wrote:
> On Tue, Apr 24, 2018 at 11:18 AM, Stephen Frost <sfrost@snowman.net> wrote:
> > Greetings,
> >
> > * Tom Lane (tgl@sss.pgh.pa.us) wrote:
> >> So far, dory has failed three times with essentially identical symptoms:
> >>
> >> 2018-04-23 19:57:10.624 GMT [2240] FATAL: could not reattach to shared memory (key=0000000000000190, addr=00000000018E0000): error code 487
> >> 2018-04-23 15:57:10.657 EDT [8836] ERROR: lost connection to parallel worker
> >> 2018-04-23 15:57:10.657 EDT [8836] STATEMENT: select count(*) from tenk1 group by twenty;
> >> 2018-04-23 15:57:10.660 EDT [3820] LOG: background worker "parallel worker" (PID 2240) exited with exit code 1
> >>
> >> Now how can this be? We've successfully reserved and released the address
> >> range we want to use, so it *should* be free at the instant we try to map.
> >
> > Yeah, that's definitely interesting.
>
> I wondered if another thread with the right timing could map something
> between the VirtualFree() and MapViewOfFileEx() calls, but we don't
> create the Windows signal handling thread until a bit later. Could
> there be any any other threads active in the process?
>
> Maybe try asking what's mapped there with VirtualQueryEx() on failure?
+1. An implementation of that:
https://www.postgresql.org/message-id/20170403065106.GA2624300%40tornado.leadboat.com
^ permalink raw reply [nested|flat] 33+ messages in thread
* Re: "could not reattach to shared memory" on buildfarm member dory
@ 2018-04-24 07:18 Magnus Hagander <magnus@hagander.net>
parent: Stephen Frost <sfrost@snowman.net>
1 sibling, 2 replies; 33+ messages in thread
From: Magnus Hagander @ 2018-04-24 07:18 UTC (permalink / raw)
To: Stephen Frost <sfrost@snowman.net>; +Cc: Tom Lane <tgl@sss.pgh.pa.us>; PostgreSQL Developers <pgsql-hackers@lists.postgresql.org>
On Tue, Apr 24, 2018 at 1:18 AM, Stephen Frost <sfrost@snowman.net> wrote:
> Greetings,
>
> * Tom Lane (tgl@sss.pgh.pa.us) wrote:
> > So far, dory has failed three times with essentially identical symptoms:
> >
> > 2018-04-23 19:57:10.624 GMT [2240] FATAL: could not reattach to shared
> memory (key=0000000000000190, addr=00000000018E0000): error code 487
> > 2018-04-23 15:57:10.657 EDT [8836] ERROR: lost connection to parallel
> worker
> > 2018-04-23 15:57:10.657 EDT [8836] STATEMENT: select count(*) from
> tenk1 group by twenty;
> > 2018-04-23 15:57:10.660 EDT [3820] LOG: background worker "parallel
> worker" (PID 2240) exited with exit code 1
> >
> > Now how can this be? We've successfully reserved and released the
> address
> > range we want to use, so it *should* be free at the instant we try to
> map.
>
> Yeah, that's definitely interesting.
>
> > I guess the good news is that we're seeing this in a reasonably
> > reproducible fashion, so there's some hope of digging down to find
> > out the actual cause.
>
> I've asked Heath to take a look at the system again and see if there's
> any Windows logs or such that might help us understand what's happening.
> AV was disabled on the box, so don't think it's that, at least.
>
Disabled or uninstalled?
Back when I was combating windows AV on a daily basis, this normally did
not have the same effect. Just disabling the AV didn't actually remove the
parts that caused issues, it just hid them. Actual uninstall is what was
required.
--
Magnus Hagander
Me: https://www.hagander.net/ <http://www.hagander.net/;
Work: https://www.redpill-linpro.com/ <http://www.redpill-linpro.com/;
^ permalink raw reply [nested|flat] 33+ messages in thread
* Re: "could not reattach to shared memory" on buildfarm member dory
@ 2018-04-26 10:48 Craig Ringer <craig@2ndquadrant.com>
parent: Magnus Hagander <magnus@hagander.net>
1 sibling, 0 replies; 33+ messages in thread
From: Craig Ringer @ 2018-04-26 10:48 UTC (permalink / raw)
To: Magnus Hagander <magnus@hagander.net>; +Cc: Stephen Frost <sfrost@snowman.net>; Tom Lane <tgl@sss.pgh.pa.us>; PostgreSQL Developers <pgsql-hackers@lists.postgresql.org>
On 24 April 2018 at 15:18, Magnus Hagander <magnus@hagander.net> wrote:
> Back when I was combating windows AV on a daily basis, this normally did not
> have the same effect. Just disabling the AV didn't actually remove the parts
> that caused issues, it just hid them. Actual uninstall is what was required.
Yep. Specifically, they tended to inject kernel hooks and/or load hook
DLLs that did funky and often flakey things. Often with poor awareness
of things like multiple processes opening one file for write at the
same time.
I think I heard that MS has cleaned up the situation with AV
considerably by offering more kernel infrastructure for it, and
restricting what you can do in terms of kernel extensions etc. But I
don't know how much.
In any case, do you think dropping a minidump at the point of failure
might be informative? It should contain the full memory mapping
information. For this purpose we could just create a crashdumps/
directory then abort() when we detect the error, and have the
buildfarm stop processing until someone can grab the tempdir with the
dumps, binaries, .pdb files, etc.
src/backend/port/win32/crashdump.c doesn't expose a helper function to
do all the dbghelp.dll messing around and create a crashdump, it only
allows that to be done via a crash handler. But it might make sense to
break out the actual "write a crash dump" part to a separately
callable function. I've looked at doing this before, but always got
stuck with the apparent lack of support in gdb or lldb to be used as a
library for self-dumping. You can always shell out to gcore I guess...
but ew. Or we can fork() and abort() the forked child like
https://github.com/RuntimeTools/gencore does, but again, ew.
I was thinking that maybe the buildfarm could just create crashdumps/
automatically, but then we'd need to have support infrastructure for
recording the Pg binaries and .pdb files along with the dumps,
rotating them so we don't run out of space, etc etc.
--
Craig Ringer http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services
^ permalink raw reply [nested|flat] 33+ messages in thread
* Re: "could not reattach to shared memory" on buildfarm member dory
@ 2018-04-27 12:26 Stephen Frost <sfrost@snowman.net>
parent: Magnus Hagander <magnus@hagander.net>
1 sibling, 0 replies; 33+ messages in thread
From: Stephen Frost @ 2018-04-27 12:26 UTC (permalink / raw)
To: Magnus Hagander <magnus@hagander.net>; +Cc: Tom Lane <tgl@sss.pgh.pa.us>; PostgreSQL Developers <pgsql-hackers@lists.postgresql.org>
Greetings,
* Magnus Hagander (magnus@hagander.net) wrote:
> On Tue, Apr 24, 2018 at 1:18 AM, Stephen Frost <sfrost@snowman.net> wrote:
> > I've asked Heath to take a look at the system again and see if there's
> > any Windows logs or such that might help us understand what's happening.
> > AV was disabled on the box, so don't think it's that, at least.
>
> Disabled or uninstalled?
The only AV installed on the system is the "Windows Defender" thing, so
it's not some additional AV system.
Thanks!
Stephen
Attachments:
[application/pgp-signature] signature.asc (818B, ../../20180427122630.GL27724@tamriel.snowman.net/2-signature.asc)
download
^ permalink raw reply [nested|flat] 33+ messages in thread
* Re: "could not reattach to shared memory" on buildfarm member dory
@ 2018-04-28 02:06 Tom Lane <tgl@sss.pgh.pa.us>
parent: Noah Misch <noah@leadboat.com>
1 sibling, 1 reply; 33+ messages in thread
From: Tom Lane @ 2018-04-28 02:06 UTC (permalink / raw)
To: Noah Misch <noah@leadboat.com>; +Cc: Thomas Munro <thomas.munro@enterprisedb.com>; Stephen Frost <sfrost@snowman.net>; PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
Noah Misch <noah@leadboat.com> writes:
> On Tue, Apr 24, 2018 at 11:37:33AM +1200, Thomas Munro wrote:
>> Maybe try asking what's mapped there with VirtualQueryEx() on failure?
> +1. An implementation of that:
> https://www.postgresql.org/message-id/20170403065106.GA2624300%40tornado.leadboat.com
Not seeing any other work happening here, I pushed a little bit of
quick-hack investigation code. This is based on noting that
VirtualAllocEx is documented as rounding the allocation up to a page
boundary (4K), but there's nothing specific about whether or how much
CreateFileMapping or MapViewOfFileEx might round up. The observed
failures could be explained if those guys might eat more virtual
address space for the same request size as VirtualAllocEx does.
This is a stretch, for sure, but given the lack of any other theories
we might as well check it.
regards, tom lane
^ permalink raw reply [nested|flat] 33+ messages in thread
* Re: "could not reattach to shared memory" on buildfarm member dory
@ 2018-04-28 13:59 Stephen Frost <sfrost@snowman.net>
parent: Tom Lane <tgl@sss.pgh.pa.us>
0 siblings, 0 replies; 33+ messages in thread
From: Stephen Frost @ 2018-04-28 13:59 UTC (permalink / raw)
To: Tom Lane <tgl@sss.pgh.pa.us>; +Cc: Noah Misch <noah@leadboat.com>; Thomas Munro <thomas.munro@enterprisedb.com>; PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
Greetings,
* Tom Lane (tgl@sss.pgh.pa.us) wrote:
> Noah Misch <noah@leadboat.com> writes:
> > On Tue, Apr 24, 2018 at 11:37:33AM +1200, Thomas Munro wrote:
> >> Maybe try asking what's mapped there with VirtualQueryEx() on failure?
>
> > +1. An implementation of that:
> > https://www.postgresql.org/message-id/20170403065106.GA2624300%40tornado.leadboat.com
>
> Not seeing any other work happening here, I pushed a little bit of
> quick-hack investigation code. This is based on noting that
> VirtualAllocEx is documented as rounding the allocation up to a page
> boundary (4K), but there's nothing specific about whether or how much
> CreateFileMapping or MapViewOfFileEx might round up. The observed
> failures could be explained if those guys might eat more virtual
> address space for the same request size as VirtualAllocEx does.
> This is a stretch, for sure, but given the lack of any other theories
> we might as well check it.
Sounds good to me. Just as an FYI, there are a couple folks taking a
look at the system and trying to figure out what's going on. We've seen
an Event ID 1530 error in the Windows Event log associated with
vctip.exe which Visual Studio was running with the build, but only
sometimes. When vctip.exe is being run and then finishes, it goes and
cleans things up which seems to be what's triggering the 1530 and that
appears to correllate with the failures, but hard to say if that's
really a smoking gun or is just coincidence.
Thanks!
Stephen
Attachments:
[application/pgp-signature] signature.asc (818B, ../../20180428135923.GO27724@tamriel.snowman.net/2-signature.asc)
download
^ permalink raw reply [nested|flat] 33+ messages in thread
* Re: "could not reattach to shared memory" on buildfarm member dory
@ 2018-04-30 17:07 Tom Lane <tgl@sss.pgh.pa.us>
parent: Noah Misch <noah@leadboat.com>
1 sibling, 2 replies; 33+ messages in thread
From: Tom Lane @ 2018-04-30 17:07 UTC (permalink / raw)
To: Noah Misch <noah@leadboat.com>; +Cc: Thomas Munro <thomas.munro@enterprisedb.com>; Stephen Frost <sfrost@snowman.net>; PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
[ Thanks to Stephen for cranking up a continuous build loop on dory ]
Noah Misch <noah@leadboat.com> writes:
> On Tue, Apr 24, 2018 at 11:37:33AM +1200, Thomas Munro wrote:
>> Maybe try asking what's mapped there with VirtualQueryEx() on failure?
> +1. An implementation of that:
> https://www.postgresql.org/message-id/20170403065106.GA2624300%40tornado.leadboat.com
So I tried putting in that code, and it turns the problem from something
that maybe happens in every third buildfarm run or so, to something that
happens at least a dozen times in a single "make check" step. This seems
to mean that either EnumProcessModules or GetModuleFileNameEx is itself
allocating memory, and sometimes that allocation comes out of the space
VirtualFree just freed :-(.
So we can't use those functions. We have however proven that no new
module gets loaded during VirtualFree or MapViewOfFileEx, so there
doesn't seem to be anything more to be learned from them anyway.
What it looks like to me is that MapViewOfFileEx allocates some memory and
sometimes that comes out of the wrong place. This is, um, unfortunate.
It also appears that VirtualFree might sometimes allocate some memory,
and that'd be even more unfortunate, but it's hard to be certain; the
blame might well fail on VirtualQuery instead. (Ain't Heisenbugs fun?)
The solution I was thinking about last night was to have
PGSharedMemoryReAttach call MapViewOfFileEx to map the shared memory
segment at an unspecified address, then unmap it, then call VirtualFree,
and finally call MapViewOfFileEx with the real target address. The idea
here is to get these various DLLs to set up any memory allocation pools
they're going to set up before we risk doing VirtualFree. I am not,
at this point, convinced this will fix it :-( ... but I'm not sure what
else to try.
In any case, it's still pretty unclear why dory is showing this problem
and other buildfarm members are not. whelk for instance seems to be
loading all the same DLLs and more besides.
regards, tom lane
^ permalink raw reply [nested|flat] 33+ messages in thread
* Re: "could not reattach to shared memory" on buildfarm member dory
@ 2018-04-30 18:34 Stephen Frost <sfrost@snowman.net>
parent: Tom Lane <tgl@sss.pgh.pa.us>
1 sibling, 1 reply; 33+ messages in thread
From: Stephen Frost @ 2018-04-30 18:34 UTC (permalink / raw)
To: Tom Lane <tgl@sss.pgh.pa.us>; +Cc: Noah Misch <noah@leadboat.com>; Thomas Munro <thomas.munro@enterprisedb.com>; PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>; Heath Lord <heath.lord@crunchydata.com>
Greetings,
* Tom Lane (tgl@sss.pgh.pa.us) wrote:
> [ Thanks to Stephen for cranking up a continuous build loop on dory ]
That was actually Heath, who is also trying to work this issue and has
an idea about something else which might help (and some more information
about what's happening in the event log). Adding him to the thread so
he can (easily) reply with what he's found.
Heath..?
Thanks!
Stephen
Attachments:
[application/pgp-signature] signature.asc (818B, ../../20180430183427.GV27724@tamriel.snowman.net/2-signature.asc)
download
^ permalink raw reply [nested|flat] 33+ messages in thread
* Re: "could not reattach to shared memory" on buildfarm member dory
@ 2018-04-30 19:25 Heath Lord <heath.lord@crunchydata.com>
parent: Stephen Frost <sfrost@snowman.net>
0 siblings, 2 replies; 33+ messages in thread
From: Heath Lord @ 2018-04-30 19:25 UTC (permalink / raw)
To: Stephen Frost <sfrost@snowman.net>; +Cc: Tom Lane <tgl@sss.pgh.pa.us>; Noah Misch <noah@leadboat.com>; Thomas Munro <thomas.munro@enterprisedb.com>; PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
On Mon, Apr 30, 2018 at 2:34 PM, Stephen Frost <sfrost@snowman.net> wrote:
> Greetings,
>
> * Tom Lane (tgl@sss.pgh.pa.us) wrote:
> > [ Thanks to Stephen for cranking up a continuous build loop on dory ]
>
> That was actually Heath, who is also trying to work this issue and has
> an idea about something else which might help (and some more information
> about what's happening in the event log). Adding him to the thread so
> he can (easily) reply with what he's found.
>
> Heath..?
>
> Thanks!
>
> Stephen
>
So what I noticed after adding the '--force' flag was that in the Event
Viewer logs there was an Error in the System log stating that "The
application-specific permission settings do not grant Local Activation
permission for the COM Server application" for the Runtime Broker. So
around 2:00 pm today I went and changed the ownership of the registry
values to Administrators so I could add the user we are running the builds
under to the list of members that have access to it. However right after I
made that change the build was actually broken for me so I am just now
turning the builds back on to run constantly to verify if this has
any effect on the issue of not being able to reattach to the shared
memory. I am hoping that this makes things more stable, however I am not
confident that these are related.
The change that I attempted prior to this, which was done last Wednesday,
was to remove the vctip.exe program from being used as this was causing
issues for us as well. This was causing an Event ID 1530 error that stated
that "The application this is listed in the event details is leaving the
registry handle open" and Windows was helpfully closing any registry values
for that user profile, and I thought that possibly when doing so it was
cleaning up the memory that was allocated prior. However this did not
change anything for us after making that change.
Any ideas or changes that we could do to help debug or verify would be
helpful. We have considered changing it to run everything as SYSTEM but if
possible we would like to avoid this for security reasons. Thank you in
advance and I appreciate all the help.
-Heath
^ permalink raw reply [nested|flat] 33+ messages in thread
* Re: "could not reattach to shared memory" on buildfarm member dory
@ 2018-04-30 19:29 Stephen Frost <sfrost@snowman.net>
parent: Heath Lord <heath.lord@crunchydata.com>
1 sibling, 0 replies; 33+ messages in thread
From: Stephen Frost @ 2018-04-30 19:29 UTC (permalink / raw)
To: Heath Lord <heath.lord@crunchydata.com>; +Cc: Tom Lane <tgl@sss.pgh.pa.us>; Noah Misch <noah@leadboat.com>; Thomas Munro <thomas.munro@enterprisedb.com>; PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
Greetings,
* Heath Lord (heath.lord@crunchydata.com) wrote:
> Any ideas or changes that we could do to help debug or verify would be
> helpful. We have considered changing it to run everything as SYSTEM but if
> possible we would like to avoid this for security reasons. Thank you in
> advance and I appreciate all the help.
Just to be clear- there is no longer anything showing up in the event
viewer associated with running the builds. There may still be an issue
with the system setup or such, but it at least seems less likely for
that to be the issue, so I'm thinking that Tom is more likely correct
that PG is doing something not quite right here.
Thanks!
Stephen
Attachments:
[application/pgp-signature] signature.asc (818B, ../../20180430192953.GW27724@tamriel.snowman.net/2-signature.asc)
download
^ permalink raw reply [nested|flat] 33+ messages in thread
* Re: "could not reattach to shared memory" on buildfarm member dory
@ 2018-04-30 19:45 Tom Lane <tgl@sss.pgh.pa.us>
parent: Heath Lord <heath.lord@crunchydata.com>
1 sibling, 0 replies; 33+ messages in thread
From: Tom Lane @ 2018-04-30 19:45 UTC (permalink / raw)
To: Heath Lord <heath.lord@crunchydata.com>; +Cc: Stephen Frost <sfrost@snowman.net>; Noah Misch <noah@leadboat.com>; Thomas Munro <thomas.munro@enterprisedb.com>; PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
Heath Lord <heath.lord@crunchydata.com> writes:
> So what I noticed after adding the '--force' flag was that in the Event
> Viewer logs there was an Error in the System log stating that "The
> application-specific permission settings do not grant Local Activation
> permission for the COM Server application" for the Runtime Broker. So
> around 2:00 pm today I went and changed the ownership of the registry
> values to Administrators so I could add the user we are running the builds
> under to the list of members that have access to it. However right after I
> made that change the build was actually broken for me so I am just now
> turning the builds back on to run constantly to verify if this has
> any effect on the issue of not being able to reattach to the shared
> memory. I am hoping that this makes things more stable, however I am not
> confident that these are related.
The build was broken on Windows between about 12:30 and 14:30 EDT,
thanks to an unrelated change. Now that that's sorted, dory is
still failing :-(.
Moreover, even though I took out the module dump logic, the failure rate
is still a good bit higher than it was before, which seems to confirm my
fear that this is a Heisenbug: either VirtualQuery itself, or the act of
elog'ing a bunch of output, is causing memory allocations to take place
that otherwise would not have.
I'm hoping that the elog output is to blame for that, and am going to
go try to rejigger the code so that we capture the memory maps into space
that was allocated before VirtualFree.
> Any ideas or changes that we could do to help debug or verify would be
> helpful. We have considered changing it to run everything as SYSTEM but if
> possible we would like to avoid this for security reasons.
Yeah, that's no solution. Even if it were OK for dory, end users wouldn't
necessarily want to run PG that way.
regards, tom lane
^ permalink raw reply [nested|flat] 33+ messages in thread
* Re: "could not reattach to shared memory" on buildfarm member dory
@ 2018-05-01 00:01 Tom Lane <tgl@sss.pgh.pa.us>
parent: Tom Lane <tgl@sss.pgh.pa.us>
1 sibling, 2 replies; 33+ messages in thread
From: Tom Lane @ 2018-05-01 00:01 UTC (permalink / raw)
To: Noah Misch <noah@leadboat.com>; +Cc: Heath Lord <heath.lord@crunchydata.com>; Thomas Munro <thomas.munro@enterprisedb.com>; Stephen Frost <sfrost@snowman.net>; PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
I wrote:
> The solution I was thinking about last night was to have
> PGSharedMemoryReAttach call MapViewOfFileEx to map the shared memory
> segment at an unspecified address, then unmap it, then call VirtualFree,
> and finally call MapViewOfFileEx with the real target address. The idea
> here is to get these various DLLs to set up any memory allocation pools
> they're going to set up before we risk doing VirtualFree. I am not,
> at this point, convinced this will fix it :-( ... but I'm not sure what
> else to try.
So the answer is that that doesn't help at all.
It's clear from dory's results that something is causing a 4MB chunk
of memory to get reserved in the process's address space, sometimes.
It might happen during the main MapViewOfFileEx call, or during the
preceding VirtualFree, or with my map/unmap dance in place, it might
happen during that. Frequently it doesn't happen at all, at least not
before the point where we've successfully done MapViewOfFileEx. But
if it does happen, and the chunk happens to get put in a spot that
overlaps where we want to put the shmem block, kaboom.
What seems like a plausible theory at this point is that the apparent
asynchronicity is due to the allocation being triggered by a different
thread, and the fact that our added monitoring code seems to make the
failure more likely can be explained by that code changing the timing.
But what thread could it be? It doesn't really look to me like either
the signal thread or the timer thread could eat 4MB. syslogger.c
also spawns a thread, on Windows, but AFAICS that's not being used in
this test configuration. Maybe the reason dory is showing the problem
is something or other is spawning a thread we don't even know about?
I'm going to go put a 1-sec sleep into the beginning of
PGSharedMemoryReAttach and see if that changes anything. If I'm right
that this is being triggered by another thread, that should allow the
other thread to do its thing (at least most of the time) so that the
failure rate ought to go way down.
Even if that does happen, I'm at a loss for a reasonable way to fix it
for real. Is there a way to seize control of a Windows process so that
there are no other running threads? Any other ideas?
regards, tom lane
^ permalink raw reply [nested|flat] 33+ messages in thread
* Re: "could not reattach to shared memory" on buildfarm member dory
@ 2018-05-01 00:24 Andres Freund <andres@anarazel.de>
parent: Tom Lane <tgl@sss.pgh.pa.us>
1 sibling, 1 reply; 33+ messages in thread
From: Andres Freund @ 2018-05-01 00:24 UTC (permalink / raw)
To: Tom Lane <tgl@sss.pgh.pa.us>; +Cc: Noah Misch <noah@leadboat.com>; Heath Lord <heath.lord@crunchydata.com>; Thomas Munro <thomas.munro@enterprisedb.com>; Stephen Frost <sfrost@snowman.net>; PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
Him
On 2018-04-30 20:01:40 -0400, Tom Lane wrote:
> What seems like a plausible theory at this point is that the apparent
> asynchronicity is due to the allocation being triggered by a different
> thread, and the fact that our added monitoring code seems to make the
> failure more likely can be explained by that code changing the timing.
> But what thread could it be? It doesn't really look to me like either
> the signal thread or the timer thread could eat 4MB.
It seems plausible that the underlying allocator allocates larger chunks
to serve small allocations. But we don't seem to have started any threads
at PGSharedMemoryReAttach() time? So it'd have to be something else that
starts threads.
Heath, could you use process explorer or such to check which processes
are running inside a working backend process?
Greetings,
Andres Freund
^ permalink raw reply [nested|flat] 33+ messages in thread
* Re: "could not reattach to shared memory" on buildfarm member dory
@ 2018-05-01 02:59 Noah Misch <noah@leadboat.com>
parent: Tom Lane <tgl@sss.pgh.pa.us>
1 sibling, 2 replies; 33+ messages in thread
From: Noah Misch @ 2018-05-01 02:59 UTC (permalink / raw)
To: Tom Lane <tgl@sss.pgh.pa.us>; +Cc: Heath Lord <heath.lord@crunchydata.com>; Thomas Munro <thomas.munro@enterprisedb.com>; Stephen Frost <sfrost@snowman.net>; PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
On Mon, Apr 30, 2018 at 08:01:40PM -0400, Tom Lane wrote:
> It's clear from dory's results that something is causing a 4MB chunk
> of memory to get reserved in the process's address space, sometimes.
> It might happen during the main MapViewOfFileEx call, or during the
> preceding VirtualFree, or with my map/unmap dance in place, it might
> happen during that. Frequently it doesn't happen at all, at least not
> before the point where we've successfully done MapViewOfFileEx. But
> if it does happen, and the chunk happens to get put in a spot that
> overlaps where we want to put the shmem block, kaboom.
>
> What seems like a plausible theory at this point is that the apparent
> asynchronicity is due to the allocation being triggered by a different
> thread, and the fact that our added monitoring code seems to make the
> failure more likely can be explained by that code changing the timing.
> But what thread could it be? It doesn't really look to me like either
> the signal thread or the timer thread could eat 4MB. syslogger.c
> also spawns a thread, on Windows, but AFAICS that's not being used in
> this test configuration. Maybe the reason dory is showing the problem
> is something or other is spawning a thread we don't even know about?
Likely some privileged daemon is creating a thread in every new process. (On
Windows, it's not unusual for one process to create a thread in another
process.) We don't have good control over that.
> I'm at a loss for a reasonable way to fix it
> for real. Is there a way to seize control of a Windows process so that
> there are no other running threads?
I think not.
> Any other ideas?
PostgreSQL could retry the whole process creation, analogous to
internal_forkexec() retries. Have the failed process exit after recording the
fact that it couldn't attach. Make the postmaster notice and spawn a
replacement. Give up after 100 failed attempts.
^ permalink raw reply [nested|flat] 33+ messages in thread
* Re: "could not reattach to shared memory" on buildfarm member dory
@ 2018-05-01 03:33 Tom Lane <tgl@sss.pgh.pa.us>
parent: Noah Misch <noah@leadboat.com>
1 sibling, 0 replies; 33+ messages in thread
From: Tom Lane @ 2018-05-01 03:33 UTC (permalink / raw)
To: Noah Misch <noah@leadboat.com>; +Cc: Heath Lord <heath.lord@crunchydata.com>; Thomas Munro <thomas.munro@enterprisedb.com>; Stephen Frost <sfrost@snowman.net>; PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
Noah Misch <noah@leadboat.com> writes:
> On Mon, Apr 30, 2018 at 08:01:40PM -0400, Tom Lane wrote:
>> What seems like a plausible theory at this point is that the apparent
>> asynchronicity is due to the allocation being triggered by a different
>> thread, and the fact that our added monitoring code seems to make the
>> failure more likely can be explained by that code changing the timing.
>> But what thread could it be? It doesn't really look to me like either
>> the signal thread or the timer thread could eat 4MB. syslogger.c
>> also spawns a thread, on Windows, but AFAICS that's not being used in
>> this test configuration.
The 1-second wait doesn't seem to have changed things much, which puts
a hole in the idea that this is triggered by a thread spawned earlier
in backend process startup.
>> Maybe the reason dory is showing the problem
>> is something or other is spawning a thread we don't even know about?
> Likely some privileged daemon is creating a thread in every new process.
Yeah, I'm afraid that's the most likely theory at this point; it offers
an explanation why we're seeing this on dory and not other machines.
Although if the daemon were responding to process startup, wouldn't the
extra wait have given it time to do so? There's still something that
doesn't add up here.
>> Any other ideas?
> PostgreSQL could retry the whole process creation, analogous to
> internal_forkexec() retries.
In the absence of any clearer theory about what's causing this,
that may be our only recourse. Sure is ugly though.
regards, tom lane
^ permalink raw reply [nested|flat] 33+ messages in thread
* Re: "could not reattach to shared memory" on buildfarm member dory
@ 2018-05-01 03:52 Thomas Munro <thomas.munro@enterprisedb.com>
parent: Noah Misch <noah@leadboat.com>
1 sibling, 0 replies; 33+ messages in thread
From: Thomas Munro @ 2018-05-01 03:52 UTC (permalink / raw)
To: Noah Misch <noah@leadboat.com>; +Cc: Tom Lane <tgl@sss.pgh.pa.us>; Heath Lord <heath.lord@crunchydata.com>; Stephen Frost <sfrost@snowman.net>; PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
On Tue, May 1, 2018 at 2:59 PM, Noah Misch <noah@leadboat.com> wrote:
> Likely some privileged daemon is creating a thread in every new process. (On
> Windows, it's not unusual for one process to create a thread in another
> process.) We don't have good control over that.
Huh. I was already amazed (as a non-Windows user) by the DSM code
that duplicates file handles into the postmaster process without its
cooperation, but starting threads is even more amazing.
Apparently debuggers do that. Could this be running in some kind of
debugger-managed environment or build, perhaps as a result of some
core dump capturing mode or something?
https://msdn.microsoft.com/en-us/library/windows/desktop/dd405484(v=vs.85).aspx
Apparently another way to mess with another process's memory map is
via "Asynchronous Procedure Calls":
http://blogs.microsoft.co.il/pavely/2017/03/14/injecting-a-dll-without-a-remote-thread/
It looks like that mechanism could allow something either in our own
process (perhaps some timer-related thing that we might have set up
ourselves or might be set up by the system?) or another process to
queue actions for our own thread to run at certain points.
https://msdn.microsoft.com/en-us/library/windows/desktop/ms681951(v=vs.85).aspx
--
Thomas Munro
http://www.enterprisedb.com
^ permalink raw reply [nested|flat] 33+ messages in thread
* Re: "could not reattach to shared memory" on buildfarm member dory
@ 2018-05-01 04:13 Tom Lane <tgl@sss.pgh.pa.us>
parent: Andres Freund <andres@anarazel.de>
0 siblings, 1 reply; 33+ messages in thread
From: Tom Lane @ 2018-05-01 04:13 UTC (permalink / raw)
To: Andres Freund <andres@anarazel.de>; +Cc: Noah Misch <noah@leadboat.com>; Heath Lord <heath.lord@crunchydata.com>; Thomas Munro <thomas.munro@enterprisedb.com>; Stephen Frost <sfrost@snowman.net>; PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
Andres Freund <andres@anarazel.de> writes:
> Heath, could you use process explorer or such to check which processes
> are running inside a working backend process?
It seems to be possible to enumerate the threads that are present inside a
Windows process, although it's not clear to me how much identifying info
is available. Perhaps it'd be worth putting in some "dump threads"
debugging code like the "dump modules" code we had in there for a bit?
regards, tom lane
^ permalink raw reply [nested|flat] 33+ messages in thread
* Re: "could not reattach to shared memory" on buildfarm member dory
@ 2018-05-01 15:31 Tom Lane <tgl@sss.pgh.pa.us>
parent: Tom Lane <tgl@sss.pgh.pa.us>
0 siblings, 1 reply; 33+ messages in thread
From: Tom Lane @ 2018-05-01 15:31 UTC (permalink / raw)
To: Heath Lord <heath.lord@crunchydata.com>; +Cc: Andres Freund <andres@anarazel.de>; Noah Misch <noah@leadboat.com>; Thomas Munro <thomas.munro@enterprisedb.com>; Stephen Frost <sfrost@snowman.net>; PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
Well, at this point the only thing that's entirely clear is that none
of the ideas I had work. I think we are going to be forced to pursue
Noah's idea of doing an end-to-end retry. Somebody else will need to
take point on that; I lack a Windows environment and have already done
a lot more blind patch-pushing than I like in this effort.
I'll revert the debugging code I added to win32_shmem.c, unless
someone sees a reason to leave it there awhile longer.
regards, tom lane
^ permalink raw reply [nested|flat] 33+ messages in thread
* Re: "could not reattach to shared memory" on buildfarm member dory
@ 2018-08-19 02:00 Noah Misch <noah@leadboat.com>
parent: Tom Lane <tgl@sss.pgh.pa.us>
0 siblings, 1 reply; 33+ messages in thread
From: Noah Misch @ 2018-08-19 02:00 UTC (permalink / raw)
To: Tom Lane <tgl@sss.pgh.pa.us>; +Cc: Heath Lord <heath.lord@crunchydata.com>; Andres Freund <andres@anarazel.de>; Thomas Munro <thomas.munro@enterprisedb.com>; Stephen Frost <sfrost@snowman.net>; PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
On Tue, May 01, 2018 at 11:31:50AM -0400, Tom Lane wrote:
> Well, at this point the only thing that's entirely clear is that none
> of the ideas I had work. I think we are going to be forced to pursue
> Noah's idea of doing an end-to-end retry. Somebody else will need to
> take point on that; I lack a Windows environment and have already done
> a lot more blind patch-pushing than I like in this effort.
Having tried this, I find a choice between performance and complexity. Both
of my designs use proc_exit(4) to indicate failure to reattach. The simpler,
slower design has WIN32 internal_forkexec() block until the child reports (via
SetEvent()) that it reattached to shared memory. This caused a fivefold
reduction in process creation performance[1]. The less-simple, faster design
stashes the Port structure and retry count in the BackendList entry, which
reaper() uses to retry the fork upon seeing status 4. Notably, this requires
new code for regular backends, for bgworkers, and for others. It's currently
showing a 30% performance _increase_ on the same benchmark; I can't explain
that increase and doubt it will last, but I think it's plausible for the
less-simple design to be performance-neutral.
I see these options:
1. Use the simpler design with a GUC, disabled by default, to control whether
the new code is active. Mention the GUC in a new errhint() for the "could
not reattach to shared memory" error.
2. Like (1), but enable the GUC by default.
3. Like (1), but follow up with a patch to enable the GUC by default in v12
only.
4. In addition to (1), enable retries if the GUC is set _or_ this postmaster
has seen at least one child fail to reattach.
5. Use the less-simple design, with retries enabled unconditionally.
I think I prefer (3), with (1) being a close second. My hesitation on (3) is
that parallel query has made startup time count even if you use a connection
pool, and all the Windows users not needing these retries will see parallel
query become that much slower. I dislike (5) for its impact on
platform-independent postmaster code. Other opinions?
I'm attaching a mostly-finished patch for the slower design. I tested
correctness with -DREATTACH_FORCE_FAIL_PERCENT=99. I'm also attaching a
proof-of-concept patch for the faster design. In this proof of concept, the
postmaster does not close its copy of a backend socket until the backend
exits. Also, bgworkers can change from BGWH_STARTED back to
BGWH_NOT_YET_STARTED; core code tolerates this, but external code may not.
Those would justify paying some performance to fix. The proof of concept
handles bgworkers and regular backends, but it does not handle the startup
process, checkpointer, etc. That doesn't affect benchmarking, of course.
nm
[1] This (2 forks per transaction) dropped from 139tps to 27tps:
echo 'select 1' >script
env PGOPTIONS="--default_transaction_isolation=repeatable\\ read --force_parallel_mode=on" pgbench -T15 -j30 -c30 --connect -n -fscript
diff --git a/src/backend/port/win32_shmem.c b/src/backend/port/win32_shmem.c
index f8ca52e..e96517f 100644
--- a/src/backend/port/win32_shmem.c
+++ b/src/backend/port/win32_shmem.c
@@ -383,6 +383,20 @@ PGSharedMemoryReAttach(void)
Assert(UsedShmemSegAddr != NULL);
Assert(IsUnderPostmaster);
+#ifdef REATTACH_FORCE_FAIL_PERCENT
+
+ /*
+ * For testing, emulate a system where my MapViewOfFileEx() fails some
+ * percentage of the time.
+ */
+ srandom((unsigned int) (MyProcPid ^ MyStartTime));
+ if (random() % 100 < (REATTACH_FORCE_FAIL_PERCENT))
+ {
+ elog(LOG, "emulating failure to reattach to shared memory");
+ proc_exit(4);
+ }
+#endif
+
/*
* Release memory region reservation that was made by the postmaster
*/
@@ -392,8 +406,11 @@ PGSharedMemoryReAttach(void)
hdr = (PGShmemHeader *) MapViewOfFileEx(UsedShmemSegID, FILE_MAP_READ | FILE_MAP_WRITE, 0, 0, 0, UsedShmemSegAddr);
if (!hdr)
- elog(FATAL, "could not reattach to shared memory (key=%p, addr=%p): error code %lu",
+ {
+ elog(LOG, "could not reattach to shared memory (key=%p, addr=%p): error code %lu",
UsedShmemSegID, UsedShmemSegAddr, GetLastError());
+ proc_exit(4); /* Ask internal_forkexec() to retry. */
+ }
if (hdr != origUsedShmemSegAddr)
elog(FATAL, "reattaching to shared memory returned unexpected address (got %p, expected %p)",
hdr, origUsedShmemSegAddr);
diff --git a/src/backend/postmaster/postmaster.c b/src/backend/postmaster/postmaster.c
index a4b53b3..7432223 100644
--- a/src/backend/postmaster/postmaster.c
+++ b/src/backend/postmaster/postmaster.c
@@ -454,6 +454,12 @@ static void InitPostmasterDeathWatchHandle(void);
static pid_t waitpid(pid_t pid, int *exitstatus, int options);
static void WINAPI pgwin32_deadchild_callback(PVOID lpParameter, BOOLEAN TimerOrWaitFired);
+/*
+ * During internal_forkexec(), the child signals this auto-reset event to
+ * indicate that it is no longer at risk of needing a fork retry.
+ */
+static HANDLE win32ReAttach;
+
static HANDLE win32ChildQueue;
typedef struct
@@ -520,6 +526,7 @@ typedef struct
int max_safe_fds;
int MaxBackends;
#ifdef WIN32
+ HANDLE win32ReAttach;
HANDLE PostmasterHandle;
HANDLE initial_signal_pipe;
HANDLE syslogPipe[2];
@@ -556,6 +563,7 @@ static void ShmemBackendArrayRemove(Backend *bn);
#define EXIT_STATUS_0(st) ((st) == 0)
#define EXIT_STATUS_1(st) (WIFEXITED(st) && WEXITSTATUS(st) == 1)
#define EXIT_STATUS_3(st) (WIFEXITED(st) && WEXITSTATUS(st) == 3)
+/* status 4 means PGSharedMemoryReAttach() failure; see internal_forkexec() */
#ifndef WIN32
/*
@@ -1197,6 +1205,20 @@ PostmasterMain(int argc, char *argv[])
#ifdef WIN32
+ {
+ SECURITY_ATTRIBUTES sa;
+
+ sa.nLength = sizeof(sa);
+ sa.lpSecurityDescriptor = NULL;
+ sa.bInheritHandle = TRUE;
+
+ win32ReAttach = CreateEvent(&sa, FALSE, FALSE, NULL);
+ if (win32ReAttach == NULL)
+ ereport(FATAL,
+ (errmsg("could not create event to facilitate fork emulation: error code %lu",
+ GetLastError())));
+ }
+
/*
* Initialize I/O completion port used to deliver list of dead children.
*/
@@ -4524,6 +4546,8 @@ internal_forkexec(int argc, char *argv[], Port *port)
BackendParameters *param;
SECURITY_ATTRIBUTES sa;
char paramHandleStr[32];
+ HANDLE wait_handles[2];
+ bool done;
win32_deadchild_waitinfo *childinfo;
/* Make sure caller set up argv properly */
@@ -4650,8 +4674,7 @@ retry:
/*
* Now that the backend variables are written out, we start the child
- * thread so it can start initializing while we set up the rest of the
- * parent state.
+ * thread so it can start initializing.
*/
if (ResumeThread(pi.hThread) == -1)
{
@@ -4673,6 +4696,64 @@ retry:
}
/*
+ * Block until child dies or uses SetEvent(win32ReAttach) to indicate that
+ * it reattached to shared memory.
+ */
+ wait_handles[0] = win32ReAttach;
+ wait_handles[1] = pi.hProcess;
+ done = false;
+ while (!done)
+ {
+ DWORD rc,
+ exitcode;
+
+ rc = WaitForMultipleObjectsEx(2, wait_handles, FALSE, INFINITE, TRUE);
+ switch (rc)
+ {
+ case WAIT_OBJECT_0: /* child reattached */
+ done = true;
+ break;
+ case WAIT_OBJECT_0 + 1: /* child exited */
+ done = true;
+ if (GetExitCodeProcess(pi.hProcess, &exitcode) &&
+ exitcode == 4)
+ {
+ /* child already made a log entry */
+ CloseHandle(pi.hProcess);
+ CloseHandle(pi.hThread);
+ if (++retry_count < 100)
+ goto retry;
+ ereport(LOG,
+ (errmsg("giving up after too many tries to reattach to shared memory"),
+ errhint("This might be caused by ASLR or antivirus software.")));
+ return -1;
+ }
+ /* else, let pgwin32_deadchild_callback() handle the exit */
+ break;
+ case WAIT_IO_COMPLETION:
+
+ /*
+ * The system interrupted the wait to execute an I/O
+ * completion routine or asynchronous procedure call in this
+ * thread. PostgreSQL does not provoke either of these, but
+ * atypical loaded DLLs or even other processes might do so.
+ * Now, resume waiting.
+ */
+ break;
+ case WAIT_FAILED:
+ ereport(FATAL,
+ (errmsg("could not wait for new child to start: error code %lu",
+ GetLastError())));
+ break;
+ default:
+ elog(FATAL,
+ "unexpected return code from WaitForMultipleObjectsEx(): %lu",
+ rc);
+ break;
+ }
+ }
+
+ /*
* Queue a waiter for to signal when this child dies. The wait will be
* handled automatically by an operating system thread pool.
*
@@ -4787,6 +4868,14 @@ SubPostmasterMain(int argc, char *argv[])
else
PGSharedMemoryNoReAttach();
+#ifdef WIN32
+ if (!SetEvent(win32ReAttach))
+ elog(FATAL,
+ "could not indicate, to postmaster, reattach to shared memory: error code %lu",
+ GetLastError());
+ /* postmaster is now free to return from internal_forkexec() */
+#endif
+
/* autovacuum needs this set before calling InitProcess */
if (strcmp(argv[1], "--forkavlauncher") == 0)
AutovacuumLauncherIAm();
@@ -6030,6 +6119,7 @@ save_backend_variables(BackendParameters *param, Port *port,
param->MaxBackends = MaxBackends;
#ifdef WIN32
+ param->win32ReAttach = win32ReAttach;
param->PostmasterHandle = PostmasterHandle;
if (!write_duplicated_handle(¶m->initial_signal_pipe,
pgwin32_create_signal_listener(childPid),
@@ -6262,6 +6352,7 @@ restore_backend_variables(BackendParameters *param, Port *port)
MaxBackends = param->MaxBackends;
#ifdef WIN32
+ win32ReAttach = param->win32ReAttach;
PostmasterHandle = param->PostmasterHandle;
pgwin32_initial_signal_pipe = param->initial_signal_pipe;
#else
diff --git a/src/backend/port/win32_shmem.c b/src/backend/port/win32_shmem.c
index f8ca52e..e84e2fc 100644
--- a/src/backend/port/win32_shmem.c
+++ b/src/backend/port/win32_shmem.c
@@ -382,6 +382,7 @@ PGSharedMemoryReAttach(void)
Assert(UsedShmemSegAddr != NULL);
Assert(IsUnderPostmaster);
+ /* FIXME change FATAL to proc_exit(1) */
/*
* Release memory region reservation that was made by the postmaster
diff --git a/src/backend/postmaster/postmaster.c b/src/backend/postmaster/postmaster.c
index a4b53b3..eadf7f1 100644
--- a/src/backend/postmaster/postmaster.c
+++ b/src/backend/postmaster/postmaster.c
@@ -180,6 +180,14 @@ typedef struct bkend
bool dead_end; /* is it going to send an error and quit? */
bool bgworker_notify; /* gets bgworker start/stop notifications */
dlist_node elem; /* list link in BackendList */
+
+#ifdef EXEC_BACKEND
+#define RETRYABLE_FORK
+#endif
+#ifdef RETRYABLE_FORK
+ Port *port; /* for still-opening backends */
+ int retry_count;
+#endif
} Backend;
static dlist_head BackendList = DLIST_STATIC_INIT(BackendList);
@@ -412,6 +420,7 @@ static void BackendInitialize(Port *port);
static void BackendRun(Port *port) pg_attribute_noreturn();
static void ExitPostmaster(int status) pg_attribute_noreturn();
static int ServerLoop(void);
+static pid_t BackendFork(Backend *bn);
static int BackendStartup(Port *port);
static int ProcessStartupPacket(Port *port, bool SSLdone);
static void SendNegotiateProtocolVersion(List *unrecognized_protocol_options);
@@ -427,6 +436,7 @@ static void TerminateChildren(int signal);
#define SignalChildren(sig) SignalSomeChildren(sig, BACKEND_TYPE_ALL)
static int CountChildren(int target);
+static pid_t do_fork_bgworker(RegisteredBgWorker *rw);
static bool assign_backendlist_entry(RegisteredBgWorker *rw);
static void maybe_start_bgworkers(void);
static bool CreateOptsFile(int argc, char *argv[], char *fullprogname);
@@ -543,6 +553,7 @@ static bool save_backend_variables(BackendParameters *param, Port *port,
#endif
static void ShmemBackendArrayAdd(Backend *bn);
+static void ShmemBackendArrayUpdate(Backend *bn, pid_t oldpid);
static void ShmemBackendArrayRemove(Backend *bn);
#endif /* EXEC_BACKEND */
@@ -556,6 +567,7 @@ static void ShmemBackendArrayRemove(Backend *bn);
#define EXIT_STATUS_0(st) ((st) == 0)
#define EXIT_STATUS_1(st) (WIFEXITED(st) && WEXITSTATUS(st) == 1)
#define EXIT_STATUS_3(st) (WIFEXITED(st) && WEXITSTATUS(st) == 3)
+#define EXIT_STATUS_4(st) (WIFEXITED(st) && WEXITSTATUS(st) == 4)
#ifndef WIN32
/*
@@ -1703,14 +1715,8 @@ ServerLoop(void)
port = ConnCreate(ListenSocket[i]);
if (port)
{
+ /* FIXME refactor more to happen in BackendStartup() */
BackendStartup(port);
-
- /*
- * We no longer need the open socket or port structure
- * in this process
- */
- StreamClose(port->sock);
- ConnFree(port);
}
}
}
@@ -2311,7 +2317,11 @@ processCancelRequest(Port *port, void *pkt)
backendPID)));
return;
}
+#ifndef EXEC_BACKEND
+ }
+#else
}
+#endif
/* No matching backend */
ereport(LOG,
@@ -2393,8 +2403,6 @@ ConnCreate(int serverFd)
if (StreamConnection(serverFd, port) != STATUS_OK)
{
- if (port->sock != PGINVALID_SOCKET)
- StreamClose(port->sock);
ConnFree(port);
return NULL;
}
@@ -2425,6 +2433,8 @@ ConnCreate(int serverFd)
static void
ConnFree(Port *conn)
{
+ if (conn->sock != PGINVALID_SOCKET)
+ StreamClose(conn->sock);
#ifdef USE_SSL
secure_close(conn);
#endif
@@ -2814,6 +2824,8 @@ reaper(SIGNAL_ARGS)
continue;
}
+ /* FIXME restart startup process after status 4 */
+
/*
* Unexpected exit of startup process (including FATAL exit)
* during PM_STARTUP is treated as catastrophic. There are no
@@ -2897,14 +2909,15 @@ reaper(SIGNAL_ARGS)
}
/*
- * Was it the bgwriter? Normal exit can be ignored; we'll start a new
- * one at the next iteration of the postmaster's main loop, if
- * necessary. Any other exit condition is treated as a crash.
+ * Was it the bgwriter? Normal exit and retryable startup failure can
+ * be ignored; we'll start a new one at the next iteration of the
+ * postmaster's main loop, if necessary. Any other exit condition is
+ * treated as a crash.
*/
if (pid == BgWriterPID)
{
BgWriterPID = 0;
- if (!EXIT_STATUS_0(exitstatus))
+ if (!EXIT_STATUS_0(exitstatus) && !EXIT_STATUS_4(exitstatus))
HandleChildCrash(pid, exitstatus,
_("background writer process"));
continue;
@@ -2953,6 +2966,7 @@ reaper(SIGNAL_ARGS)
if (PgStatPID != 0)
signal_child(PgStatPID, SIGQUIT);
}
+ /* FIXME retry checkpointer */
else
{
/*
@@ -2967,44 +2981,47 @@ reaper(SIGNAL_ARGS)
}
/*
- * Was it the wal writer? Normal exit can be ignored; we'll start a
- * new one at the next iteration of the postmaster's main loop, if
- * necessary. Any other exit condition is treated as a crash.
+ * Was it the wal writer? Normal exit and retryable startup failure
+ * can be ignored; we'll start a new one at the next iteration of the
+ * postmaster's main loop, if necessary. Any other exit condition is
+ * treated as a crash.
*/
if (pid == WalWriterPID)
{
WalWriterPID = 0;
- if (!EXIT_STATUS_0(exitstatus))
+ if (!EXIT_STATUS_0(exitstatus) && !EXIT_STATUS_4(exitstatus))
HandleChildCrash(pid, exitstatus,
_("WAL writer process"));
continue;
}
/*
- * Was it the wal receiver? If exit status is zero (normal) or one
- * (FATAL exit), we assume everything is all right just like normal
- * backends. (If we need a new wal receiver, we'll start one at the
- * next iteration of the postmaster's main loop.)
+ * Was it the wal receiver? If exit status is zero (normal), one
+ * (FATAL exit) or four (retryable startup failure), we assume
+ * everything is all right just like normal backends. (If we need a
+ * new wal receiver, we'll start one at the next iteration of the
+ * postmaster's main loop.)
*/
if (pid == WalReceiverPID)
{
WalReceiverPID = 0;
- if (!EXIT_STATUS_0(exitstatus) && !EXIT_STATUS_1(exitstatus))
+ if (!EXIT_STATUS_0(exitstatus) && !EXIT_STATUS_1(exitstatus) &&
+ !EXIT_STATUS_4(exitstatus))
HandleChildCrash(pid, exitstatus,
_("WAL receiver process"));
continue;
}
/*
- * Was it the autovacuum launcher? Normal exit can be ignored; we'll
- * start a new one at the next iteration of the postmaster's main
- * loop, if necessary. Any other exit condition is treated as a
- * crash.
+ * Was it the autovacuum launcher? Normal exit and retryable startup
+ * failure can be ignored; we'll start a new one at the next iteration
+ * of the postmaster's main loop, if necessary. Any other exit
+ * condition is treated as a crash.
*/
if (pid == AutoVacPID)
{
AutoVacPID = 0;
- if (!EXIT_STATUS_0(exitstatus))
+ if (!EXIT_STATUS_0(exitstatus) && !EXIT_STATUS_4(exitstatus))
HandleChildCrash(pid, exitstatus,
_("autovacuum launcher process"));
continue;
@@ -3116,33 +3133,60 @@ CleanupBackgroundWorker(int pid,
snprintf(namebuf, MAXPGPATH, _("background worker \"%s\""),
rw->rw_worker.bgw_type);
+ LogChildExit(DEBUG2, namebuf, pid, exitstatus);
- if (!EXIT_STATUS_0(exitstatus))
- {
- /* Record timestamp, so we know when to restart the worker. */
- rw->rw_crashed_at = GetCurrentTimestamp();
- }
- else
+ if (EXIT_STATUS_0(exitstatus))
{
/* Zero exit status means terminate */
rw->rw_crashed_at = 0;
rw->rw_terminate = true;
}
+ else if (!EXIT_STATUS_4(exitstatus))
+ {
+ /* Record timestamp, so we know when to restart the worker. */
+ rw->rw_crashed_at = GetCurrentTimestamp();
+ }
/*
* Additionally, for shared-memory-connected workers, just like a
- * backend, any exit status other than 0 or 1 is considered a crash
+ * backend, any exit status other than 0, 1 or 4 is considered a crash
* and causes a system-wide restart.
*/
if ((rw->rw_worker.bgw_flags & BGWORKER_SHMEM_ACCESS) != 0)
{
- if (!EXIT_STATUS_0(exitstatus) && !EXIT_STATUS_1(exitstatus))
+ if (!EXIT_STATUS_0(exitstatus) && !EXIT_STATUS_1(exitstatus) &&
+ !EXIT_STATUS_4(exitstatus))
{
HandleChildCrash(pid, exitstatus, namebuf);
return true;
}
}
+#ifdef RETRYABLE_FORK
+ if (EXIT_STATUS_4(exitstatus))
+ {
+ Backend *bp = rw->rw_backend;
+
+ if (++bp->retry_count >= 100) /* FIXME use a constant */
+ {
+ ereport(LOG,
+ (errmsg("giving up after too many tries to reserve shared memory"),
+ errhint("This might be caused by ASLR or antivirus software.")));
+ }
+ else
+ {
+ bp->pid = do_fork_bgworker(rw);
+ if (bp->pid > 0)
+ {
+ ShmemBackendArrayUpdate(bp, pid);
+ return true;
+ }
+ }
+ /* mark entry as crashed, so we'll try again later */
+ rw->rw_crashed_at = GetCurrentTimestamp();
+ }
+#endif
+
/*
* We must release the postmaster child slot whether this worker is
* connected to shared memory or not, but we only treat it as a crash
@@ -3187,7 +3231,8 @@ CleanupBackgroundWorker(int pid,
/*
* CleanupBackend -- cleanup after terminated backend.
*
- * Remove all local state associated with backend.
+ * If the backend failed during early startup, retry forking it. Otherwise,
+ * remove all local state associated with backend.
*
* If you change this, see also CleanupBackgroundWorker.
*/
@@ -3222,7 +3267,8 @@ CleanupBackend(int pid,
}
#endif
- if (!EXIT_STATUS_0(exitstatus) && !EXIT_STATUS_1(exitstatus))
+ if (!EXIT_STATUS_0(exitstatus) && !EXIT_STATUS_1(exitstatus) &&
+ !EXIT_STATUS_4(exitstatus))
{
HandleChildCrash(pid, exitstatus, _("server process"));
return;
@@ -3234,6 +3280,40 @@ CleanupBackend(int pid,
if (bp->pid == pid)
{
+#ifdef RETRYABLE_FORK
+ if (EXIT_STATUS_4(exitstatus))
+ {
+ Assert(bp->port);
+
+ if (++bp->retry_count >= 100)
+ {
+ ereport(LOG,
+ (errmsg("giving up after too many tries to reserve shared memory"),
+ errhint("This might be caused by ASLR or antivirus software.")));
+ report_fork_failure_to_client(bp->port, EAGAIN);
+ }
+ else
+ {
+ bp->pid = BackendFork(bp);
+ if (bp->pid > 0)
+ {
+ if (!bp->dead_end)
+ ShmemBackendArrayUpdate(bp, pid);
+ return;
+ }
+ /* Else, fork failed. Fall through to remove local state. */
+ }
+ }
+ /*
+ * For a sufficiently long-running backend, FIXME will have
+ * already called ConnFree().
+ */
+ if (bp->port)
+ {
+ ConnFree(bp->port);
+ bp->port = NULL;
+ }
+#endif
if (!bp->dead_end)
{
if (!ReleasePostmasterChildSlot(bp->child_slot))
@@ -3607,6 +3687,14 @@ LogChildExit(int lev, const char *procname, int pid, int exitstatus)
static void
PostmasterStateMachine(void)
{
+ if (pmState == PM_STARTUP && StartupStatus == STARTUP_NOT_RUNNING)
+ {
+ /* We need a startup process, either the */
+ StartupPID = StartupDataBase();
+ Assert(StartupPID != 0);
+ StartupStatus = STARTUP_RUNNING;
+ }
+
if (pmState == PM_WAIT_BACKUP)
{
/*
@@ -3954,9 +4042,60 @@ TerminateChildren(int signal)
signal_child(PgStatPID, signal);
}
+/* FIXME comment */
+static pid_t
+BackendFork(Backend *bn)
+{
+ pid_t pid;
+
+ MyCancelKey = bn->cancel_key;
+ MyPMChildSlot = bn->child_slot;
+
+#ifdef EXEC_BACKEND
+ pid = backend_forkexec(bn->port);
+#else /* !EXEC_BACKEND */
+ pid = fork_process();
+ if (pid == 0) /* child */
+ {
+ /* Detangle from postmaster */
+ InitPostmasterChild();
+
+ /* Close the postmaster's sockets */
+ ClosePostmasterPorts(false);
+
+ /* Perform additional initialization and collect startup packet */
+ BackendInitialize(bn->port);
+
+ /* And run the backend */
+ BackendRun(bn->port);
+ }
+#endif /* EXEC_BACKEND */
+
+ if (pid < 0)
+ {
+ /* in parent, fork failed */
+ int save_errno = errno;
+
+ errno = save_errno;
+ ereport(LOG,
+ (errmsg("could not fork new process for connection: %m")));
+ report_fork_failure_to_client(bn->port, save_errno);
+ }
+ else
+ {
+ /* in parent, successful fork */
+ ereport(DEBUG2,
+ (errmsg_internal("forked new backend, pid=%d socket=%d",
+ (int) pid, (int) bn->port->sock)));
+ }
+
+ return pid;
+}
+
/*
* BackendStartup -- start backend process
*
+ * FIXME
* returns: STATUS_ERROR if the fork failed, STATUS_OK otherwise.
*
* Note: if you change this code, also consider StartAutovacuumWorker.
@@ -3965,7 +4104,6 @@ static int
BackendStartup(Port *port)
{
Backend *bn; /* for backend cleanup */
- pid_t pid;
/*
* Create backend data structure. Better before the fork() so we can
@@ -3977,24 +4115,13 @@ BackendStartup(Port *port)
ereport(LOG,
(errcode(ERRCODE_OUT_OF_MEMORY),
errmsg("out of memory")));
- return STATUS_ERROR;
- }
-
- /*
- * Compute the cancel key that will be assigned to this backend. The
- * backend will have its own copy in the forked-off process' value of
- * MyCancelKey, so that it can transmit the key to the frontend.
- */
- if (!RandomCancelKey(&MyCancelKey))
- {
- free(bn);
- ereport(LOG,
- (errcode(ERRCODE_INTERNAL_ERROR),
- errmsg("could not generate random cancel key")));
- return STATUS_ERROR;
+ goto err;
}
- bn->cancel_key = MyCancelKey;
+#ifdef RETRYABLE_FORK
+ bn->port = port;
+ bn->retry_count = 0;
+#endif
/* Pass down canAcceptConnections state */
port->canAcceptConnections = canAcceptConnections();
@@ -4005,60 +4132,38 @@ BackendStartup(Port *port)
* Unless it's a dead_end child, assign it a child slot number
*/
if (!bn->dead_end)
- bn->child_slot = MyPMChildSlot = AssignPostmasterChildSlot();
+ bn->child_slot = AssignPostmasterChildSlot();
else
bn->child_slot = 0;
/* Hasn't asked to be notified about any bgworkers yet */
bn->bgworker_notify = false;
-#ifdef EXEC_BACKEND
- pid = backend_forkexec(port);
-#else /* !EXEC_BACKEND */
- pid = fork_process();
- if (pid == 0) /* child */
+ /* Compute the cancel key that will be assigned to this backend. */
+ if (!RandomCancelKey(&bn->cancel_key))
{
- free(bn);
-
- /* Detangle from postmaster */
- InitPostmasterChild();
-
- /* Close the postmaster's sockets */
- ClosePostmasterPorts(false);
-
- /* Perform additional initialization and collect startup packet */
- BackendInitialize(port);
-
- /* And run the backend */
- BackendRun(port);
- }
-#endif /* EXEC_BACKEND */
-
- if (pid < 0)
- {
- /* in parent, fork failed */
- int save_errno = errno;
-
- if (!bn->dead_end)
- (void) ReleasePostmasterChildSlot(bn->child_slot);
- free(bn);
- errno = save_errno;
ereport(LOG,
- (errmsg("could not fork new process for connection: %m")));
- report_fork_failure_to_client(port, save_errno);
- return STATUS_ERROR;
+ (errcode(ERRCODE_INTERNAL_ERROR),
+ errmsg("could not generate random cancel key")));
+ goto err;
}
- /* in parent, successful fork */
- ereport(DEBUG2,
- (errmsg_internal("forked new backend, pid=%d socket=%d",
- (int) pid, (int) port->sock)));
+ bn->pid = BackendFork(bn);
+ if (bn->pid < 0)
+ goto err;
+
+#ifndef RETRYABLE_FORK
+ /*
+ * We no longer need the open socket or port structure
+ * in this process
+ */
+ ConnFree(port);
+#endif
/*
* Everything's been successful, it's safe to add this backend to our list
* of backends.
*/
- bn->pid = pid;
bn->bkend_type = BACKEND_TYPE_NORMAL; /* Can change later to WALSND */
dlist_push_head(&BackendList, &bn->elem);
@@ -4068,6 +4173,16 @@ BackendStartup(Port *port)
#endif
return STATUS_OK;
+
+err:
+ ConnFree(port);
+ if (bn)
+ {
+ if (bn->child_slot)
+ (void) ReleasePostmasterChildSlot(bn->child_slot);
+ free(bn);
+ }
+ return STATUS_ERROR;
}
/*
@@ -4708,6 +4823,15 @@ retry:
#endif /* WIN32 */
+static void
+MarkSharedStateComplete(void)
+{
+#ifdef FIXME_WIN32
+ SendPostmasterSignal(PMSIGNAL_BACKEND_SHMAT);
+#endif
+}
+
+
/*
* SubPostmasterMain -- Get the fork/exec'd process into a state equivalent
* to what it would be if we'd simply forked on Unix, and then
@@ -4783,7 +4907,24 @@ SubPostmasterMain(int argc, char *argv[])
strcmp(argv[1], "--forkavworker") == 0 ||
strcmp(argv[1], "--forkboot") == 0 ||
strncmp(argv[1], "--forkbgworker=", 15) == 0)
+ {
+ /* test retryable starts */
+ if (0 && (strcmp(argv[1], "--forkbackend") == 0 ||
+ strcmp(argv[1], "--forkavlauncher") == 0 ||
+ strcmp(argv[1], "--forkavworker") == 0 ||
+ /* strcmp(argv[1], "--forkboot") == 0 || */
+ strncmp(argv[1], "--forkbgworker=", 15) == 0))
+ {
+ MyProcPid = getpid(); /* FIXME not needed? */
+ srandom((unsigned int ) (MyProcPid ^ MyStartTime));
+ if (random() % 10 != 0)
+ {
+ elog(LOG, "failing startup for testing purposes");
+ proc_exit(4);
+ }
+ }
PGSharedMemoryReAttach();
+ }
else
PGSharedMemoryNoReAttach();
@@ -4878,6 +5019,7 @@ SubPostmasterMain(int argc, char *argv[])
/* Attach process to shared data structures */
CreateSharedMemoryAndSemaphores(false, 0);
+ MarkSharedStateComplete();
/* And run the backend */
BackendRun(&port); /* does not return */
@@ -4892,6 +5034,7 @@ SubPostmasterMain(int argc, char *argv[])
/* Attach process to shared data structures */
CreateSharedMemoryAndSemaphores(false, 0);
+ MarkSharedStateComplete();
AuxiliaryProcessMain(argc - 2, argv + 2); /* does not return */
}
@@ -4905,6 +5048,7 @@ SubPostmasterMain(int argc, char *argv[])
/* Attach process to shared data structures */
CreateSharedMemoryAndSemaphores(false, 0);
+ MarkSharedStateComplete();
AutoVacLauncherMain(argc - 2, argv + 2); /* does not return */
}
@@ -4918,6 +5062,7 @@ SubPostmasterMain(int argc, char *argv[])
/* Attach process to shared data structures */
CreateSharedMemoryAndSemaphores(false, 0);
+ MarkSharedStateComplete();
AutoVacWorkerMain(argc - 2, argv + 2); /* does not return */
}
@@ -4936,6 +5081,7 @@ SubPostmasterMain(int argc, char *argv[])
/* Attach process to shared data structures */
CreateSharedMemoryAndSemaphores(false, 0);
+ MarkSharedStateComplete();
/* Fetch MyBgworkerEntry from shared memory */
shmem_slot = atoi(argv[1] + 15);
@@ -4952,12 +5098,14 @@ SubPostmasterMain(int argc, char *argv[])
if (strcmp(argv[1], "--forkcol") == 0)
{
/* Do not want to attach to shared memory */
+ MarkSharedStateComplete();
PgstatCollectorMain(argc, argv); /* does not return */
}
if (strcmp(argv[1], "--forklog") == 0)
{
/* Do not want to attach to shared memory */
+ MarkSharedStateComplete();
SysLoggerMain(argc, argv); /* does not return */
}
@@ -5620,40 +5768,14 @@ bgworker_forkexec(int shmem_slot)
}
#endif
-/*
- * Start a new bgworker.
- * Starting time conditions must have been checked already.
- *
- * Returns true on success, false on failure.
- * In either case, update the RegisteredBgWorker's state appropriately.
- *
- * This code is heavily based on autovacuum.c, q.v.
- */
-static bool
-do_start_bgworker(RegisteredBgWorker *rw)
+/* FIXME comment */
+static pid_t
+do_fork_bgworker(RegisteredBgWorker *rw)
{
pid_t worker_pid;
- Assert(rw->rw_pid == 0);
-
- /*
- * Allocate and assign the Backend element. Note we must do this before
- * forking, so that we can handle out of memory properly.
- *
- * Treat failure as though the worker had crashed. That way, the
- * postmaster will wait a bit before attempting to start it again; if it
- * tried again right away, most likely it'd find itself repeating the
- * out-of-memory or fork failure condition.
- */
- if (!assign_backendlist_entry(rw))
- {
- rw->rw_crashed_at = GetCurrentTimestamp();
- return false;
- }
-
- ereport(DEBUG1,
- (errmsg("starting background worker process \"%s\"",
- rw->rw_worker.bgw_name)));
+ MyCancelKey = rw->rw_backend->cancel_key;
+ MyPMChildSlot = rw->rw_backend->child_slot;
#ifdef EXEC_BACKEND
switch ((worker_pid = bgworker_forkexec(rw->rw_shmem_slot)))
@@ -5665,13 +5787,6 @@ do_start_bgworker(RegisteredBgWorker *rw)
/* in postmaster, fork failed ... */
ereport(LOG,
(errmsg("could not fork worker process: %m")));
- /* undo what assign_backendlist_entry did */
- ReleasePostmasterChildSlot(rw->rw_child_slot);
- rw->rw_child_slot = 0;
- free(rw->rw_backend);
- rw->rw_backend = NULL;
- /* mark entry as crashed, so we'll try again later */
- rw->rw_crashed_at = GetCurrentTimestamp();
break;
#ifndef EXEC_BACKEND
@@ -5702,15 +5817,73 @@ do_start_bgworker(RegisteredBgWorker *rw)
#endif
default:
/* in postmaster, fork successful ... */
- rw->rw_pid = worker_pid;
+ rw->rw_pid = worker_pid; /* FIXME move later */
rw->rw_backend->pid = rw->rw_pid;
- ReportBackgroundWorkerPID(rw);
- /* add new worker to lists of backends */
- dlist_push_head(&BackendList, &rw->rw_backend->elem);
+ ReportBackgroundWorkerPID(rw); /* FIXME move later */
+
+ ereport(DEBUG2,
+ (errmsg_internal("FIXME forked new bgworker, pid=%d",
+ (int) worker_pid)));
+ }
+
+ return worker_pid;
+}
+
+/*
+ * Start a new bgworker.
+ * Starting time conditions must have been checked already.
+ *
+ * Returns true on success, false on failure.
+ * In either case, update the RegisteredBgWorker's state appropriately.
+ * FIXME responsible for BackendList lifecycle
+ *
+ * This code is heavily based on autovacuum.c, q.v.
+ */
+static bool
+do_start_bgworker(RegisteredBgWorker *rw)
+{
+ pid_t worker_pid;
+
+ Assert(rw->rw_pid == 0);
+
+ /*
+ * Allocate and assign the Backend element. Note we must do this before
+ * forking, so that we can handle out of memory properly.
+ *
+ * Treat failure as though the worker had crashed. That way, the
+ * postmaster will wait a bit before attempting to start it again; if it
+ * tried again right away, most likely it'd find itself repeating the
+ * out-of-memory or fork failure condition.
+ */
+ if (!assign_backendlist_entry(rw))
+ {
+ rw->rw_crashed_at = GetCurrentTimestamp();
+ return false;
+ }
+
+ ereport(DEBUG1,
+ (errmsg("starting background worker process \"%s\"",
+ rw->rw_worker.bgw_name)));
+
+ worker_pid = do_fork_bgworker(rw);
+ if (worker_pid == -1)
+ {
+ /* undo what assign_backendlist_entry did */
+ ReleasePostmasterChildSlot(rw->rw_child_slot);
+ rw->rw_child_slot = 0;
+ free(rw->rw_backend);
+ rw->rw_backend = NULL;
+ /* mark entry as crashed, so we'll try again later */
+ rw->rw_crashed_at = GetCurrentTimestamp();
+ }
+ else
+ {
+ /* add new worker to lists of backends */
+ dlist_push_head(&BackendList, &rw->rw_backend->elem);
#ifdef EXEC_BACKEND
- ShmemBackendArrayAdd(rw->rw_backend);
+ ShmemBackendArrayAdd(rw->rw_backend);
#endif
- return true;
+ return true;
}
return false;
@@ -5797,6 +5970,10 @@ assign_backendlist_entry(RegisteredBgWorker *rw)
bn->bkend_type = BACKEND_TYPE_BGWORKER;
bn->dead_end = false;
bn->bgworker_notify = false;
+#ifdef RETRYABLE_FORK
+ bn->port = NULL;
+ bn->retry_count = 0;
+#endif
rw->rw_backend = bn;
rw->rw_child_slot = bn->child_slot;
@@ -6306,6 +6483,15 @@ ShmemBackendArrayAdd(Backend *bn)
}
static void
+ShmemBackendArrayUpdate(Backend *bn, pid_t oldpid)
+{
+ int i = bn->child_slot - 1;
+
+ Assert(ShmemBackendArray[i].pid == oldpid);
+ ShmemBackendArray[i] = *bn;
+}
+
+static void
ShmemBackendArrayRemove(Backend *bn)
{
int i = bn->child_slot - 1;
diff --git a/src/include/storage/pmsignal.h b/src/include/storage/pmsignal.h
index 0747341..1a6a46c 100644
--- a/src/include/storage/pmsignal.h
+++ b/src/include/storage/pmsignal.h
@@ -35,6 +35,7 @@ typedef enum
PMSIGNAL_RECOVERY_STARTED, /* recovery has started */
PMSIGNAL_BEGIN_HOT_STANDBY, /* begin Hot Standby */
PMSIGNAL_WAKEN_ARCHIVER, /* send a NOTIFY signal to xlog archiver */
+ PMSIGNAL_BACKEND_SHMAT, /* newest backend no longer seeks shmem attachment */
PMSIGNAL_ROTATE_LOGFILE, /* send SIGUSR1 to syslogger to rotate logfile */
PMSIGNAL_START_AUTOVAC_LAUNCHER, /* start an autovacuum launcher */
PMSIGNAL_START_AUTOVAC_WORKER, /* start an autovacuum worker */
Attachments:
[text/plain] w32attach-retry-block-v1.patch (6.5K, ../../20180819020007.GD3795674@rfd.leadboat.com/2-w32attach-retry-block-v1.patch)
download | inline diff:
diff --git a/src/backend/port/win32_shmem.c b/src/backend/port/win32_shmem.c
index f8ca52e..e96517f 100644
--- a/src/backend/port/win32_shmem.c
+++ b/src/backend/port/win32_shmem.c
@@ -383,6 +383,20 @@ PGSharedMemoryReAttach(void)
Assert(UsedShmemSegAddr != NULL);
Assert(IsUnderPostmaster);
+#ifdef REATTACH_FORCE_FAIL_PERCENT
+
+ /*
+ * For testing, emulate a system where my MapViewOfFileEx() fails some
+ * percentage of the time.
+ */
+ srandom((unsigned int) (MyProcPid ^ MyStartTime));
+ if (random() % 100 < (REATTACH_FORCE_FAIL_PERCENT))
+ {
+ elog(LOG, "emulating failure to reattach to shared memory");
+ proc_exit(4);
+ }
+#endif
+
/*
* Release memory region reservation that was made by the postmaster
*/
@@ -392,8 +406,11 @@ PGSharedMemoryReAttach(void)
hdr = (PGShmemHeader *) MapViewOfFileEx(UsedShmemSegID, FILE_MAP_READ | FILE_MAP_WRITE, 0, 0, 0, UsedShmemSegAddr);
if (!hdr)
- elog(FATAL, "could not reattach to shared memory (key=%p, addr=%p): error code %lu",
+ {
+ elog(LOG, "could not reattach to shared memory (key=%p, addr=%p): error code %lu",
UsedShmemSegID, UsedShmemSegAddr, GetLastError());
+ proc_exit(4); /* Ask internal_forkexec() to retry. */
+ }
if (hdr != origUsedShmemSegAddr)
elog(FATAL, "reattaching to shared memory returned unexpected address (got %p, expected %p)",
hdr, origUsedShmemSegAddr);
diff --git a/src/backend/postmaster/postmaster.c b/src/backend/postmaster/postmaster.c
index a4b53b3..7432223 100644
--- a/src/backend/postmaster/postmaster.c
+++ b/src/backend/postmaster/postmaster.c
@@ -454,6 +454,12 @@ static void InitPostmasterDeathWatchHandle(void);
static pid_t waitpid(pid_t pid, int *exitstatus, int options);
static void WINAPI pgwin32_deadchild_callback(PVOID lpParameter, BOOLEAN TimerOrWaitFired);
+/*
+ * During internal_forkexec(), the child signals this auto-reset event to
+ * indicate that it is no longer at risk of needing a fork retry.
+ */
+static HANDLE win32ReAttach;
+
static HANDLE win32ChildQueue;
typedef struct
@@ -520,6 +526,7 @@ typedef struct
int max_safe_fds;
int MaxBackends;
#ifdef WIN32
+ HANDLE win32ReAttach;
HANDLE PostmasterHandle;
HANDLE initial_signal_pipe;
HANDLE syslogPipe[2];
@@ -556,6 +563,7 @@ static void ShmemBackendArrayRemove(Backend *bn);
#define EXIT_STATUS_0(st) ((st) == 0)
#define EXIT_STATUS_1(st) (WIFEXITED(st) && WEXITSTATUS(st) == 1)
#define EXIT_STATUS_3(st) (WIFEXITED(st) && WEXITSTATUS(st) == 3)
+/* status 4 means PGSharedMemoryReAttach() failure; see internal_forkexec() */
#ifndef WIN32
/*
@@ -1197,6 +1205,20 @@ PostmasterMain(int argc, char *argv[])
#ifdef WIN32
+ {
+ SECURITY_ATTRIBUTES sa;
+
+ sa.nLength = sizeof(sa);
+ sa.lpSecurityDescriptor = NULL;
+ sa.bInheritHandle = TRUE;
+
+ win32ReAttach = CreateEvent(&sa, FALSE, FALSE, NULL);
+ if (win32ReAttach == NULL)
+ ereport(FATAL,
+ (errmsg("could not create event to facilitate fork emulation: error code %lu",
+ GetLastError())));
+ }
+
/*
* Initialize I/O completion port used to deliver list of dead children.
*/
@@ -4524,6 +4546,8 @@ internal_forkexec(int argc, char *argv[], Port *port)
BackendParameters *param;
SECURITY_ATTRIBUTES sa;
char paramHandleStr[32];
+ HANDLE wait_handles[2];
+ bool done;
win32_deadchild_waitinfo *childinfo;
/* Make sure caller set up argv properly */
@@ -4650,8 +4674,7 @@ retry:
/*
* Now that the backend variables are written out, we start the child
- * thread so it can start initializing while we set up the rest of the
- * parent state.
+ * thread so it can start initializing.
*/
if (ResumeThread(pi.hThread) == -1)
{
@@ -4673,6 +4696,64 @@ retry:
}
/*
+ * Block until child dies or uses SetEvent(win32ReAttach) to indicate that
+ * it reattached to shared memory.
+ */
+ wait_handles[0] = win32ReAttach;
+ wait_handles[1] = pi.hProcess;
+ done = false;
+ while (!done)
+ {
+ DWORD rc,
+ exitcode;
+
+ rc = WaitForMultipleObjectsEx(2, wait_handles, FALSE, INFINITE, TRUE);
+ switch (rc)
+ {
+ case WAIT_OBJECT_0: /* child reattached */
+ done = true;
+ break;
+ case WAIT_OBJECT_0 + 1: /* child exited */
+ done = true;
+ if (GetExitCodeProcess(pi.hProcess, &exitcode) &&
+ exitcode == 4)
+ {
+ /* child already made a log entry */
+ CloseHandle(pi.hProcess);
+ CloseHandle(pi.hThread);
+ if (++retry_count < 100)
+ goto retry;
+ ereport(LOG,
+ (errmsg("giving up after too many tries to reattach to shared memory"),
+ errhint("This might be caused by ASLR or antivirus software.")));
+ return -1;
+ }
+ /* else, let pgwin32_deadchild_callback() handle the exit */
+ break;
+ case WAIT_IO_COMPLETION:
+
+ /*
+ * The system interrupted the wait to execute an I/O
+ * completion routine or asynchronous procedure call in this
+ * thread. PostgreSQL does not provoke either of these, but
+ * atypical loaded DLLs or even other processes might do so.
+ * Now, resume waiting.
+ */
+ break;
+ case WAIT_FAILED:
+ ereport(FATAL,
+ (errmsg("could not wait for new child to start: error code %lu",
+ GetLastError())));
+ break;
+ default:
+ elog(FATAL,
+ "unexpected return code from WaitForMultipleObjectsEx(): %lu",
+ rc);
+ break;
+ }
+ }
+
+ /*
* Queue a waiter for to signal when this child dies. The wait will be
* handled automatically by an operating system thread pool.
*
@@ -4787,6 +4868,14 @@ SubPostmasterMain(int argc, char *argv[])
else
PGSharedMemoryNoReAttach();
+#ifdef WIN32
+ if (!SetEvent(win32ReAttach))
+ elog(FATAL,
+ "could not indicate, to postmaster, reattach to shared memory: error code %lu",
+ GetLastError());
+ /* postmaster is now free to return from internal_forkexec() */
+#endif
+
/* autovacuum needs this set before calling InitProcess */
if (strcmp(argv[1], "--forkavlauncher") == 0)
AutovacuumLauncherIAm();
@@ -6030,6 +6119,7 @@ save_backend_variables(BackendParameters *param, Port *port,
param->MaxBackends = MaxBackends;
#ifdef WIN32
+ param->win32ReAttach = win32ReAttach;
param->PostmasterHandle = PostmasterHandle;
if (!write_duplicated_handle(¶m->initial_signal_pipe,
pgwin32_create_signal_listener(childPid),
@@ -6262,6 +6352,7 @@ restore_backend_variables(BackendParameters *param, Port *port)
MaxBackends = param->MaxBackends;
#ifdef WIN32
+ win32ReAttach = param->win32ReAttach;
PostmasterHandle = param->PostmasterHandle;
pgwin32_initial_signal_pipe = param->initial_signal_pipe;
#else
[text/plain] w32attach-retry-nonblock-v0.patch (23.7K, ../../20180819020007.GD3795674@rfd.leadboat.com/3-w32attach-retry-nonblock-v0.patch)
download | inline diff:
diff --git a/src/backend/port/win32_shmem.c b/src/backend/port/win32_shmem.c
index f8ca52e..e84e2fc 100644
--- a/src/backend/port/win32_shmem.c
+++ b/src/backend/port/win32_shmem.c
@@ -382,6 +382,7 @@ PGSharedMemoryReAttach(void)
Assert(UsedShmemSegAddr != NULL);
Assert(IsUnderPostmaster);
+ /* FIXME change FATAL to proc_exit(1) */
/*
* Release memory region reservation that was made by the postmaster
diff --git a/src/backend/postmaster/postmaster.c b/src/backend/postmaster/postmaster.c
index a4b53b3..eadf7f1 100644
--- a/src/backend/postmaster/postmaster.c
+++ b/src/backend/postmaster/postmaster.c
@@ -180,6 +180,14 @@ typedef struct bkend
bool dead_end; /* is it going to send an error and quit? */
bool bgworker_notify; /* gets bgworker start/stop notifications */
dlist_node elem; /* list link in BackendList */
+
+#ifdef EXEC_BACKEND
+#define RETRYABLE_FORK
+#endif
+#ifdef RETRYABLE_FORK
+ Port *port; /* for still-opening backends */
+ int retry_count;
+#endif
} Backend;
static dlist_head BackendList = DLIST_STATIC_INIT(BackendList);
@@ -412,6 +420,7 @@ static void BackendInitialize(Port *port);
static void BackendRun(Port *port) pg_attribute_noreturn();
static void ExitPostmaster(int status) pg_attribute_noreturn();
static int ServerLoop(void);
+static pid_t BackendFork(Backend *bn);
static int BackendStartup(Port *port);
static int ProcessStartupPacket(Port *port, bool SSLdone);
static void SendNegotiateProtocolVersion(List *unrecognized_protocol_options);
@@ -427,6 +436,7 @@ static void TerminateChildren(int signal);
#define SignalChildren(sig) SignalSomeChildren(sig, BACKEND_TYPE_ALL)
static int CountChildren(int target);
+static pid_t do_fork_bgworker(RegisteredBgWorker *rw);
static bool assign_backendlist_entry(RegisteredBgWorker *rw);
static void maybe_start_bgworkers(void);
static bool CreateOptsFile(int argc, char *argv[], char *fullprogname);
@@ -543,6 +553,7 @@ static bool save_backend_variables(BackendParameters *param, Port *port,
#endif
static void ShmemBackendArrayAdd(Backend *bn);
+static void ShmemBackendArrayUpdate(Backend *bn, pid_t oldpid);
static void ShmemBackendArrayRemove(Backend *bn);
#endif /* EXEC_BACKEND */
@@ -556,6 +567,7 @@ static void ShmemBackendArrayRemove(Backend *bn);
#define EXIT_STATUS_0(st) ((st) == 0)
#define EXIT_STATUS_1(st) (WIFEXITED(st) && WEXITSTATUS(st) == 1)
#define EXIT_STATUS_3(st) (WIFEXITED(st) && WEXITSTATUS(st) == 3)
+#define EXIT_STATUS_4(st) (WIFEXITED(st) && WEXITSTATUS(st) == 4)
#ifndef WIN32
/*
@@ -1703,14 +1715,8 @@ ServerLoop(void)
port = ConnCreate(ListenSocket[i]);
if (port)
{
+ /* FIXME refactor more to happen in BackendStartup() */
BackendStartup(port);
-
- /*
- * We no longer need the open socket or port structure
- * in this process
- */
- StreamClose(port->sock);
- ConnFree(port);
}
}
}
@@ -2311,7 +2317,11 @@ processCancelRequest(Port *port, void *pkt)
backendPID)));
return;
}
+#ifndef EXEC_BACKEND
+ }
+#else
}
+#endif
/* No matching backend */
ereport(LOG,
@@ -2393,8 +2403,6 @@ ConnCreate(int serverFd)
if (StreamConnection(serverFd, port) != STATUS_OK)
{
- if (port->sock != PGINVALID_SOCKET)
- StreamClose(port->sock);
ConnFree(port);
return NULL;
}
@@ -2425,6 +2433,8 @@ ConnCreate(int serverFd)
static void
ConnFree(Port *conn)
{
+ if (conn->sock != PGINVALID_SOCKET)
+ StreamClose(conn->sock);
#ifdef USE_SSL
secure_close(conn);
#endif
@@ -2814,6 +2824,8 @@ reaper(SIGNAL_ARGS)
continue;
}
+ /* FIXME restart startup process after status 4 */
+
/*
* Unexpected exit of startup process (including FATAL exit)
* during PM_STARTUP is treated as catastrophic. There are no
@@ -2897,14 +2909,15 @@ reaper(SIGNAL_ARGS)
}
/*
- * Was it the bgwriter? Normal exit can be ignored; we'll start a new
- * one at the next iteration of the postmaster's main loop, if
- * necessary. Any other exit condition is treated as a crash.
+ * Was it the bgwriter? Normal exit and retryable startup failure can
+ * be ignored; we'll start a new one at the next iteration of the
+ * postmaster's main loop, if necessary. Any other exit condition is
+ * treated as a crash.
*/
if (pid == BgWriterPID)
{
BgWriterPID = 0;
- if (!EXIT_STATUS_0(exitstatus))
+ if (!EXIT_STATUS_0(exitstatus) && !EXIT_STATUS_4(exitstatus))
HandleChildCrash(pid, exitstatus,
_("background writer process"));
continue;
@@ -2953,6 +2966,7 @@ reaper(SIGNAL_ARGS)
if (PgStatPID != 0)
signal_child(PgStatPID, SIGQUIT);
}
+ /* FIXME retry checkpointer */
else
{
/*
@@ -2967,44 +2981,47 @@ reaper(SIGNAL_ARGS)
}
/*
- * Was it the wal writer? Normal exit can be ignored; we'll start a
- * new one at the next iteration of the postmaster's main loop, if
- * necessary. Any other exit condition is treated as a crash.
+ * Was it the wal writer? Normal exit and retryable startup failure
+ * can be ignored; we'll start a new one at the next iteration of the
+ * postmaster's main loop, if necessary. Any other exit condition is
+ * treated as a crash.
*/
if (pid == WalWriterPID)
{
WalWriterPID = 0;
- if (!EXIT_STATUS_0(exitstatus))
+ if (!EXIT_STATUS_0(exitstatus) && !EXIT_STATUS_4(exitstatus))
HandleChildCrash(pid, exitstatus,
_("WAL writer process"));
continue;
}
/*
- * Was it the wal receiver? If exit status is zero (normal) or one
- * (FATAL exit), we assume everything is all right just like normal
- * backends. (If we need a new wal receiver, we'll start one at the
- * next iteration of the postmaster's main loop.)
+ * Was it the wal receiver? If exit status is zero (normal), one
+ * (FATAL exit) or four (retryable startup failure), we assume
+ * everything is all right just like normal backends. (If we need a
+ * new wal receiver, we'll start one at the next iteration of the
+ * postmaster's main loop.)
*/
if (pid == WalReceiverPID)
{
WalReceiverPID = 0;
- if (!EXIT_STATUS_0(exitstatus) && !EXIT_STATUS_1(exitstatus))
+ if (!EXIT_STATUS_0(exitstatus) && !EXIT_STATUS_1(exitstatus) &&
+ !EXIT_STATUS_4(exitstatus))
HandleChildCrash(pid, exitstatus,
_("WAL receiver process"));
continue;
}
/*
- * Was it the autovacuum launcher? Normal exit can be ignored; we'll
- * start a new one at the next iteration of the postmaster's main
- * loop, if necessary. Any other exit condition is treated as a
- * crash.
+ * Was it the autovacuum launcher? Normal exit and retryable startup
+ * failure can be ignored; we'll start a new one at the next iteration
+ * of the postmaster's main loop, if necessary. Any other exit
+ * condition is treated as a crash.
*/
if (pid == AutoVacPID)
{
AutoVacPID = 0;
- if (!EXIT_STATUS_0(exitstatus))
+ if (!EXIT_STATUS_0(exitstatus) && !EXIT_STATUS_4(exitstatus))
HandleChildCrash(pid, exitstatus,
_("autovacuum launcher process"));
continue;
@@ -3116,33 +3133,60 @@ CleanupBackgroundWorker(int pid,
snprintf(namebuf, MAXPGPATH, _("background worker \"%s\""),
rw->rw_worker.bgw_type);
+ LogChildExit(DEBUG2, namebuf, pid, exitstatus);
- if (!EXIT_STATUS_0(exitstatus))
- {
- /* Record timestamp, so we know when to restart the worker. */
- rw->rw_crashed_at = GetCurrentTimestamp();
- }
- else
+ if (EXIT_STATUS_0(exitstatus))
{
/* Zero exit status means terminate */
rw->rw_crashed_at = 0;
rw->rw_terminate = true;
}
+ else if (!EXIT_STATUS_4(exitstatus))
+ {
+ /* Record timestamp, so we know when to restart the worker. */
+ rw->rw_crashed_at = GetCurrentTimestamp();
+ }
/*
* Additionally, for shared-memory-connected workers, just like a
- * backend, any exit status other than 0 or 1 is considered a crash
+ * backend, any exit status other than 0, 1 or 4 is considered a crash
* and causes a system-wide restart.
*/
if ((rw->rw_worker.bgw_flags & BGWORKER_SHMEM_ACCESS) != 0)
{
- if (!EXIT_STATUS_0(exitstatus) && !EXIT_STATUS_1(exitstatus))
+ if (!EXIT_STATUS_0(exitstatus) && !EXIT_STATUS_1(exitstatus) &&
+ !EXIT_STATUS_4(exitstatus))
{
HandleChildCrash(pid, exitstatus, namebuf);
return true;
}
}
+#ifdef RETRYABLE_FORK
+ if (EXIT_STATUS_4(exitstatus))
+ {
+ Backend *bp = rw->rw_backend;
+
+ if (++bp->retry_count >= 100) /* FIXME use a constant */
+ {
+ ereport(LOG,
+ (errmsg("giving up after too many tries to reserve shared memory"),
+ errhint("This might be caused by ASLR or antivirus software.")));
+ }
+ else
+ {
+ bp->pid = do_fork_bgworker(rw);
+ if (bp->pid > 0)
+ {
+ ShmemBackendArrayUpdate(bp, pid);
+ return true;
+ }
+ }
+ /* mark entry as crashed, so we'll try again later */
+ rw->rw_crashed_at = GetCurrentTimestamp();
+ }
+#endif
+
/*
* We must release the postmaster child slot whether this worker is
* connected to shared memory or not, but we only treat it as a crash
@@ -3187,7 +3231,8 @@ CleanupBackgroundWorker(int pid,
/*
* CleanupBackend -- cleanup after terminated backend.
*
- * Remove all local state associated with backend.
+ * If the backend failed during early startup, retry forking it. Otherwise,
+ * remove all local state associated with backend.
*
* If you change this, see also CleanupBackgroundWorker.
*/
@@ -3222,7 +3267,8 @@ CleanupBackend(int pid,
}
#endif
- if (!EXIT_STATUS_0(exitstatus) && !EXIT_STATUS_1(exitstatus))
+ if (!EXIT_STATUS_0(exitstatus) && !EXIT_STATUS_1(exitstatus) &&
+ !EXIT_STATUS_4(exitstatus))
{
HandleChildCrash(pid, exitstatus, _("server process"));
return;
@@ -3234,6 +3280,40 @@ CleanupBackend(int pid,
if (bp->pid == pid)
{
+#ifdef RETRYABLE_FORK
+ if (EXIT_STATUS_4(exitstatus))
+ {
+ Assert(bp->port);
+
+ if (++bp->retry_count >= 100)
+ {
+ ereport(LOG,
+ (errmsg("giving up after too many tries to reserve shared memory"),
+ errhint("This might be caused by ASLR or antivirus software.")));
+ report_fork_failure_to_client(bp->port, EAGAIN);
+ }
+ else
+ {
+ bp->pid = BackendFork(bp);
+ if (bp->pid > 0)
+ {
+ if (!bp->dead_end)
+ ShmemBackendArrayUpdate(bp, pid);
+ return;
+ }
+ /* Else, fork failed. Fall through to remove local state. */
+ }
+ }
+ /*
+ * For a sufficiently long-running backend, FIXME will have
+ * already called ConnFree().
+ */
+ if (bp->port)
+ {
+ ConnFree(bp->port);
+ bp->port = NULL;
+ }
+#endif
if (!bp->dead_end)
{
if (!ReleasePostmasterChildSlot(bp->child_slot))
@@ -3607,6 +3687,14 @@ LogChildExit(int lev, const char *procname, int pid, int exitstatus)
static void
PostmasterStateMachine(void)
{
+ if (pmState == PM_STARTUP && StartupStatus == STARTUP_NOT_RUNNING)
+ {
+ /* We need a startup process, either the */
+ StartupPID = StartupDataBase();
+ Assert(StartupPID != 0);
+ StartupStatus = STARTUP_RUNNING;
+ }
+
if (pmState == PM_WAIT_BACKUP)
{
/*
@@ -3954,9 +4042,60 @@ TerminateChildren(int signal)
signal_child(PgStatPID, signal);
}
+/* FIXME comment */
+static pid_t
+BackendFork(Backend *bn)
+{
+ pid_t pid;
+
+ MyCancelKey = bn->cancel_key;
+ MyPMChildSlot = bn->child_slot;
+
+#ifdef EXEC_BACKEND
+ pid = backend_forkexec(bn->port);
+#else /* !EXEC_BACKEND */
+ pid = fork_process();
+ if (pid == 0) /* child */
+ {
+ /* Detangle from postmaster */
+ InitPostmasterChild();
+
+ /* Close the postmaster's sockets */
+ ClosePostmasterPorts(false);
+
+ /* Perform additional initialization and collect startup packet */
+ BackendInitialize(bn->port);
+
+ /* And run the backend */
+ BackendRun(bn->port);
+ }
+#endif /* EXEC_BACKEND */
+
+ if (pid < 0)
+ {
+ /* in parent, fork failed */
+ int save_errno = errno;
+
+ errno = save_errno;
+ ereport(LOG,
+ (errmsg("could not fork new process for connection: %m")));
+ report_fork_failure_to_client(bn->port, save_errno);
+ }
+ else
+ {
+ /* in parent, successful fork */
+ ereport(DEBUG2,
+ (errmsg_internal("forked new backend, pid=%d socket=%d",
+ (int) pid, (int) bn->port->sock)));
+ }
+
+ return pid;
+}
+
/*
* BackendStartup -- start backend process
*
+ * FIXME
* returns: STATUS_ERROR if the fork failed, STATUS_OK otherwise.
*
* Note: if you change this code, also consider StartAutovacuumWorker.
@@ -3965,7 +4104,6 @@ static int
BackendStartup(Port *port)
{
Backend *bn; /* for backend cleanup */
- pid_t pid;
/*
* Create backend data structure. Better before the fork() so we can
@@ -3977,24 +4115,13 @@ BackendStartup(Port *port)
ereport(LOG,
(errcode(ERRCODE_OUT_OF_MEMORY),
errmsg("out of memory")));
- return STATUS_ERROR;
- }
-
- /*
- * Compute the cancel key that will be assigned to this backend. The
- * backend will have its own copy in the forked-off process' value of
- * MyCancelKey, so that it can transmit the key to the frontend.
- */
- if (!RandomCancelKey(&MyCancelKey))
- {
- free(bn);
- ereport(LOG,
- (errcode(ERRCODE_INTERNAL_ERROR),
- errmsg("could not generate random cancel key")));
- return STATUS_ERROR;
+ goto err;
}
- bn->cancel_key = MyCancelKey;
+#ifdef RETRYABLE_FORK
+ bn->port = port;
+ bn->retry_count = 0;
+#endif
/* Pass down canAcceptConnections state */
port->canAcceptConnections = canAcceptConnections();
@@ -4005,60 +4132,38 @@ BackendStartup(Port *port)
* Unless it's a dead_end child, assign it a child slot number
*/
if (!bn->dead_end)
- bn->child_slot = MyPMChildSlot = AssignPostmasterChildSlot();
+ bn->child_slot = AssignPostmasterChildSlot();
else
bn->child_slot = 0;
/* Hasn't asked to be notified about any bgworkers yet */
bn->bgworker_notify = false;
-#ifdef EXEC_BACKEND
- pid = backend_forkexec(port);
-#else /* !EXEC_BACKEND */
- pid = fork_process();
- if (pid == 0) /* child */
+ /* Compute the cancel key that will be assigned to this backend. */
+ if (!RandomCancelKey(&bn->cancel_key))
{
- free(bn);
-
- /* Detangle from postmaster */
- InitPostmasterChild();
-
- /* Close the postmaster's sockets */
- ClosePostmasterPorts(false);
-
- /* Perform additional initialization and collect startup packet */
- BackendInitialize(port);
-
- /* And run the backend */
- BackendRun(port);
- }
-#endif /* EXEC_BACKEND */
-
- if (pid < 0)
- {
- /* in parent, fork failed */
- int save_errno = errno;
-
- if (!bn->dead_end)
- (void) ReleasePostmasterChildSlot(bn->child_slot);
- free(bn);
- errno = save_errno;
ereport(LOG,
- (errmsg("could not fork new process for connection: %m")));
- report_fork_failure_to_client(port, save_errno);
- return STATUS_ERROR;
+ (errcode(ERRCODE_INTERNAL_ERROR),
+ errmsg("could not generate random cancel key")));
+ goto err;
}
- /* in parent, successful fork */
- ereport(DEBUG2,
- (errmsg_internal("forked new backend, pid=%d socket=%d",
- (int) pid, (int) port->sock)));
+ bn->pid = BackendFork(bn);
+ if (bn->pid < 0)
+ goto err;
+
+#ifndef RETRYABLE_FORK
+ /*
+ * We no longer need the open socket or port structure
+ * in this process
+ */
+ ConnFree(port);
+#endif
/*
* Everything's been successful, it's safe to add this backend to our list
* of backends.
*/
- bn->pid = pid;
bn->bkend_type = BACKEND_TYPE_NORMAL; /* Can change later to WALSND */
dlist_push_head(&BackendList, &bn->elem);
@@ -4068,6 +4173,16 @@ BackendStartup(Port *port)
#endif
return STATUS_OK;
+
+err:
+ ConnFree(port);
+ if (bn)
+ {
+ if (bn->child_slot)
+ (void) ReleasePostmasterChildSlot(bn->child_slot);
+ free(bn);
+ }
+ return STATUS_ERROR;
}
/*
@@ -4708,6 +4823,15 @@ retry:
#endif /* WIN32 */
+static void
+MarkSharedStateComplete(void)
+{
+#ifdef FIXME_WIN32
+ SendPostmasterSignal(PMSIGNAL_BACKEND_SHMAT);
+#endif
+}
+
+
/*
* SubPostmasterMain -- Get the fork/exec'd process into a state equivalent
* to what it would be if we'd simply forked on Unix, and then
@@ -4783,7 +4907,24 @@ SubPostmasterMain(int argc, char *argv[])
strcmp(argv[1], "--forkavworker") == 0 ||
strcmp(argv[1], "--forkboot") == 0 ||
strncmp(argv[1], "--forkbgworker=", 15) == 0)
+ {
+ /* test retryable starts */
+ if (0 && (strcmp(argv[1], "--forkbackend") == 0 ||
+ strcmp(argv[1], "--forkavlauncher") == 0 ||
+ strcmp(argv[1], "--forkavworker") == 0 ||
+ /* strcmp(argv[1], "--forkboot") == 0 || */
+ strncmp(argv[1], "--forkbgworker=", 15) == 0))
+ {
+ MyProcPid = getpid(); /* FIXME not needed? */
+ srandom((unsigned int ) (MyProcPid ^ MyStartTime));
+ if (random() % 10 != 0)
+ {
+ elog(LOG, "failing startup for testing purposes");
+ proc_exit(4);
+ }
+ }
PGSharedMemoryReAttach();
+ }
else
PGSharedMemoryNoReAttach();
@@ -4878,6 +5019,7 @@ SubPostmasterMain(int argc, char *argv[])
/* Attach process to shared data structures */
CreateSharedMemoryAndSemaphores(false, 0);
+ MarkSharedStateComplete();
/* And run the backend */
BackendRun(&port); /* does not return */
@@ -4892,6 +5034,7 @@ SubPostmasterMain(int argc, char *argv[])
/* Attach process to shared data structures */
CreateSharedMemoryAndSemaphores(false, 0);
+ MarkSharedStateComplete();
AuxiliaryProcessMain(argc - 2, argv + 2); /* does not return */
}
@@ -4905,6 +5048,7 @@ SubPostmasterMain(int argc, char *argv[])
/* Attach process to shared data structures */
CreateSharedMemoryAndSemaphores(false, 0);
+ MarkSharedStateComplete();
AutoVacLauncherMain(argc - 2, argv + 2); /* does not return */
}
@@ -4918,6 +5062,7 @@ SubPostmasterMain(int argc, char *argv[])
/* Attach process to shared data structures */
CreateSharedMemoryAndSemaphores(false, 0);
+ MarkSharedStateComplete();
AutoVacWorkerMain(argc - 2, argv + 2); /* does not return */
}
@@ -4936,6 +5081,7 @@ SubPostmasterMain(int argc, char *argv[])
/* Attach process to shared data structures */
CreateSharedMemoryAndSemaphores(false, 0);
+ MarkSharedStateComplete();
/* Fetch MyBgworkerEntry from shared memory */
shmem_slot = atoi(argv[1] + 15);
@@ -4952,12 +5098,14 @@ SubPostmasterMain(int argc, char *argv[])
if (strcmp(argv[1], "--forkcol") == 0)
{
/* Do not want to attach to shared memory */
+ MarkSharedStateComplete();
PgstatCollectorMain(argc, argv); /* does not return */
}
if (strcmp(argv[1], "--forklog") == 0)
{
/* Do not want to attach to shared memory */
+ MarkSharedStateComplete();
SysLoggerMain(argc, argv); /* does not return */
}
@@ -5620,40 +5768,14 @@ bgworker_forkexec(int shmem_slot)
}
#endif
-/*
- * Start a new bgworker.
- * Starting time conditions must have been checked already.
- *
- * Returns true on success, false on failure.
- * In either case, update the RegisteredBgWorker's state appropriately.
- *
- * This code is heavily based on autovacuum.c, q.v.
- */
-static bool
-do_start_bgworker(RegisteredBgWorker *rw)
+/* FIXME comment */
+static pid_t
+do_fork_bgworker(RegisteredBgWorker *rw)
{
pid_t worker_pid;
- Assert(rw->rw_pid == 0);
-
- /*
- * Allocate and assign the Backend element. Note we must do this before
- * forking, so that we can handle out of memory properly.
- *
- * Treat failure as though the worker had crashed. That way, the
- * postmaster will wait a bit before attempting to start it again; if it
- * tried again right away, most likely it'd find itself repeating the
- * out-of-memory or fork failure condition.
- */
- if (!assign_backendlist_entry(rw))
- {
- rw->rw_crashed_at = GetCurrentTimestamp();
- return false;
- }
-
- ereport(DEBUG1,
- (errmsg("starting background worker process \"%s\"",
- rw->rw_worker.bgw_name)));
+ MyCancelKey = rw->rw_backend->cancel_key;
+ MyPMChildSlot = rw->rw_backend->child_slot;
#ifdef EXEC_BACKEND
switch ((worker_pid = bgworker_forkexec(rw->rw_shmem_slot)))
@@ -5665,13 +5787,6 @@ do_start_bgworker(RegisteredBgWorker *rw)
/* in postmaster, fork failed ... */
ereport(LOG,
(errmsg("could not fork worker process: %m")));
- /* undo what assign_backendlist_entry did */
- ReleasePostmasterChildSlot(rw->rw_child_slot);
- rw->rw_child_slot = 0;
- free(rw->rw_backend);
- rw->rw_backend = NULL;
- /* mark entry as crashed, so we'll try again later */
- rw->rw_crashed_at = GetCurrentTimestamp();
break;
#ifndef EXEC_BACKEND
@@ -5702,15 +5817,73 @@ do_start_bgworker(RegisteredBgWorker *rw)
#endif
default:
/* in postmaster, fork successful ... */
- rw->rw_pid = worker_pid;
+ rw->rw_pid = worker_pid; /* FIXME move later */
rw->rw_backend->pid = rw->rw_pid;
- ReportBackgroundWorkerPID(rw);
- /* add new worker to lists of backends */
- dlist_push_head(&BackendList, &rw->rw_backend->elem);
+ ReportBackgroundWorkerPID(rw); /* FIXME move later */
+
+ ereport(DEBUG2,
+ (errmsg_internal("FIXME forked new bgworker, pid=%d",
+ (int) worker_pid)));
+ }
+
+ return worker_pid;
+}
+
+/*
+ * Start a new bgworker.
+ * Starting time conditions must have been checked already.
+ *
+ * Returns true on success, false on failure.
+ * In either case, update the RegisteredBgWorker's state appropriately.
+ * FIXME responsible for BackendList lifecycle
+ *
+ * This code is heavily based on autovacuum.c, q.v.
+ */
+static bool
+do_start_bgworker(RegisteredBgWorker *rw)
+{
+ pid_t worker_pid;
+
+ Assert(rw->rw_pid == 0);
+
+ /*
+ * Allocate and assign the Backend element. Note we must do this before
+ * forking, so that we can handle out of memory properly.
+ *
+ * Treat failure as though the worker had crashed. That way, the
+ * postmaster will wait a bit before attempting to start it again; if it
+ * tried again right away, most likely it'd find itself repeating the
+ * out-of-memory or fork failure condition.
+ */
+ if (!assign_backendlist_entry(rw))
+ {
+ rw->rw_crashed_at = GetCurrentTimestamp();
+ return false;
+ }
+
+ ereport(DEBUG1,
+ (errmsg("starting background worker process \"%s\"",
+ rw->rw_worker.bgw_name)));
+
+ worker_pid = do_fork_bgworker(rw);
+ if (worker_pid == -1)
+ {
+ /* undo what assign_backendlist_entry did */
+ ReleasePostmasterChildSlot(rw->rw_child_slot);
+ rw->rw_child_slot = 0;
+ free(rw->rw_backend);
+ rw->rw_backend = NULL;
+ /* mark entry as crashed, so we'll try again later */
+ rw->rw_crashed_at = GetCurrentTimestamp();
+ }
+ else
+ {
+ /* add new worker to lists of backends */
+ dlist_push_head(&BackendList, &rw->rw_backend->elem);
#ifdef EXEC_BACKEND
- ShmemBackendArrayAdd(rw->rw_backend);
+ ShmemBackendArrayAdd(rw->rw_backend);
#endif
- return true;
+ return true;
}
return false;
@@ -5797,6 +5970,10 @@ assign_backendlist_entry(RegisteredBgWorker *rw)
bn->bkend_type = BACKEND_TYPE_BGWORKER;
bn->dead_end = false;
bn->bgworker_notify = false;
+#ifdef RETRYABLE_FORK
+ bn->port = NULL;
+ bn->retry_count = 0;
+#endif
rw->rw_backend = bn;
rw->rw_child_slot = bn->child_slot;
@@ -6306,6 +6483,15 @@ ShmemBackendArrayAdd(Backend *bn)
}
static void
+ShmemBackendArrayUpdate(Backend *bn, pid_t oldpid)
+{
+ int i = bn->child_slot - 1;
+
+ Assert(ShmemBackendArray[i].pid == oldpid);
+ ShmemBackendArray[i] = *bn;
+}
+
+static void
ShmemBackendArrayRemove(Backend *bn)
{
int i = bn->child_slot - 1;
diff --git a/src/include/storage/pmsignal.h b/src/include/storage/pmsignal.h
index 0747341..1a6a46c 100644
--- a/src/include/storage/pmsignal.h
+++ b/src/include/storage/pmsignal.h
@@ -35,6 +35,7 @@ typedef enum
PMSIGNAL_RECOVERY_STARTED, /* recovery has started */
PMSIGNAL_BEGIN_HOT_STANDBY, /* begin Hot Standby */
PMSIGNAL_WAKEN_ARCHIVER, /* send a NOTIFY signal to xlog archiver */
+ PMSIGNAL_BACKEND_SHMAT, /* newest backend no longer seeks shmem attachment */
PMSIGNAL_ROTATE_LOGFILE, /* send SIGUSR1 to syslogger to rotate logfile */
PMSIGNAL_START_AUTOVAC_LAUNCHER, /* start an autovacuum launcher */
PMSIGNAL_START_AUTOVAC_WORKER, /* start an autovacuum worker */
^ permalink raw reply [nested|flat] 33+ messages in thread
* Re: "could not reattach to shared memory" on buildfarm member dory
@ 2018-09-24 17:53 Tom Lane <tgl@sss.pgh.pa.us>
parent: Noah Misch <noah@leadboat.com>
0 siblings, 1 reply; 33+ messages in thread
From: Tom Lane @ 2018-09-24 17:53 UTC (permalink / raw)
To: Noah Misch <noah@leadboat.com>; +Cc: Heath Lord <heath.lord@crunchydata.com>; Andres Freund <andres@anarazel.de>; Thomas Munro <thomas.munro@enterprisedb.com>; Stephen Frost <sfrost@snowman.net>; PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
Noah Misch <noah@leadboat.com> writes:
> On Tue, May 01, 2018 at 11:31:50AM -0400, Tom Lane wrote:
>> Well, at this point the only thing that's entirely clear is that none
>> of the ideas I had work. I think we are going to be forced to pursue
>> Noah's idea of doing an end-to-end retry. Somebody else will need to
>> take point on that; I lack a Windows environment and have already done
>> a lot more blind patch-pushing than I like in this effort.
> Having tried this, I find a choice between performance and complexity. Both
> of my designs use proc_exit(4) to indicate failure to reattach. The simpler,
> slower design has WIN32 internal_forkexec() block until the child reports (via
> SetEvent()) that it reattached to shared memory. This caused a fivefold
> reduction in process creation performance[1].
Ouch.
> The less-simple, faster design
> stashes the Port structure and retry count in the BackendList entry, which
> reaper() uses to retry the fork upon seeing status 4. Notably, this requires
> new code for regular backends, for bgworkers, and for others.
Messy as that is, I think actually the worse problem with it is:
> In this proof of concept, the
> postmaster does not close its copy of a backend socket until the backend
> exits.
That seems unworkable because it would interfere with detection of client
connection drops. But since you say this is just a POC, maybe you
intended to fix that? It'd probably be all right for the postmaster to
hold onto the socket until the new backend reports successful attach,
using the same signaling mechanism you had in mind for the other way.
Overall, I agree that neither of these approaches are exactly attractive.
We're paying a heck of a lot of performance or complexity to solve a
problem that shouldn't even be there, and that we don't understand well.
In particular, the theory that some privileged code is injecting a thread
into every new process doesn't square with my results at
https://www.postgresql.org/message-id/15345.1525145612%40sss.pgh.pa.us
I think our best course of action at this point is to do nothing until
we have a clearer understanding of what's actually happening on dory.
Perhaps such understanding will yield an idea for a less painful fix.
regards, tom lane
^ permalink raw reply [nested|flat] 33+ messages in thread
* Re: "could not reattach to shared memory" on buildfarm member dory
@ 2018-09-25 15:05 Noah Misch <noah@leadboat.com>
parent: Tom Lane <tgl@sss.pgh.pa.us>
0 siblings, 1 reply; 33+ messages in thread
From: Noah Misch @ 2018-09-25 15:05 UTC (permalink / raw)
To: Tom Lane <tgl@sss.pgh.pa.us>; +Cc: Heath Lord <heath.lord@crunchydata.com>; Andres Freund <andres@anarazel.de>; Thomas Munro <thomas.munro@enterprisedb.com>; Stephen Frost <sfrost@snowman.net>; PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
On Mon, Sep 24, 2018 at 01:53:05PM -0400, Tom Lane wrote:
> Noah Misch <noah@leadboat.com> writes:
> > In this proof of concept, the
> > postmaster does not close its copy of a backend socket until the backend
> > exits.
>
> That seems unworkable because it would interfere with detection of client
> connection drops. But since you say this is just a POC, maybe you
> intended to fix that? It'd probably be all right for the postmaster to
> hold onto the socket until the new backend reports successful attach,
> using the same signaling mechanism you had in mind for the other way.
It wasn't relevant to the concept being proven, so I suspended decisions in that
area. Arranging for socket closure is a simple matter of programming.
> Overall, I agree that neither of these approaches are exactly attractive.
> We're paying a heck of a lot of performance or complexity to solve a
> problem that shouldn't even be there, and that we don't understand well.
> In particular, the theory that some privileged code is injecting a thread
> into every new process doesn't square with my results at
> https://www.postgresql.org/message-id/15345.1525145612%40sss.pgh.pa.us
>
> I think our best course of action at this point is to do nothing until
> we have a clearer understanding of what's actually happening on dory.
> Perhaps such understanding will yield an idea for a less painful fix.
I see.
^ permalink raw reply [nested|flat] 33+ messages in thread
* Re: "could not reattach to shared memory" on buildfarm member dory
@ 2018-12-03 05:35 Noah Misch <noah@leadboat.com>
parent: Noah Misch <noah@leadboat.com>
0 siblings, 2 replies; 33+ messages in thread
From: Noah Misch @ 2018-12-03 05:35 UTC (permalink / raw)
To: Heath Lord <heath.lord@crunchydata.com>; Stephen Frost <sfrost@snowman.net>; Tom Lane <tgl@sss.pgh.pa.us>; +Cc: Andres Freund <andres@anarazel.de>; Thomas Munro <thomas.munro@enterprisedb.com>; PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
On Tue, Sep 25, 2018 at 08:05:12AM -0700, Noah Misch wrote:
> On Mon, Sep 24, 2018 at 01:53:05PM -0400, Tom Lane wrote:
> > Overall, I agree that neither of these approaches are exactly attractive.
> > We're paying a heck of a lot of performance or complexity to solve a
> > problem that shouldn't even be there, and that we don't understand well.
> > In particular, the theory that some privileged code is injecting a thread
> > into every new process doesn't square with my results at
> > https://www.postgresql.org/message-id/15345.1525145612%40sss.pgh.pa.us
> >
> > I think our best course of action at this point is to do nothing until
> > we have a clearer understanding of what's actually happening on dory.
> > Perhaps such understanding will yield an idea for a less painful fix.
>
> I see.
Could one of you having a dory login use
https://live.sysinternals.com/Procmon.exe to capture process events during
backend startup? The ideal would be one capture where startup failed reattach
and another where it succeeded, but having the successful run alone would be a
good start. The procedure is roughly this:
- Install PostgreSQL w/ debug symbols.
- Start a postmaster.
- procmon /nomonitor
- procmon "Filter" menu -> Enable Advanced Output
- Ctrl-l, add filter for "Process Name" is "postgres.exe"
- Ctrl-e (starts collecting data)
- psql (leave it running)
- After ~60s, Ctrl-e again in procmon (stops collecting data)
- File -> Save -> PML
- File -> Save -> XML, include stack traces, resolve stack symbols
- Compress the PML and XML files, and mail them here
I'm attaching the data from a system not having the problem. On this system,
backend startup sees six thread creations:
1. main thread
2. thread created before postgres.exe has control
3. thread created before postgres.exe has control
4. thread created before postgres.exe has control
5. in pgwin32_signal_initialize()
6. in src\backend\port\win32\timer.c:setitimer()
Threads 2-4 exit exactly 30s after creation. If we fail to reattach to shared
memory, we'll exit before reaching code to start 5 or 6. It would be quite
interesting if dory makes a different number of threads or if threads 2-4 live
some duration other than 30s. It would also be interesting if dory has "Load
Image" events after postgres.exe code has started running. This unaffected
system loads mswsock.dll during read_inheritable_socket().
Thanks,
nm
Attachments:
[application/octet-stream] unaffected-gustnado.XML.xz (91.2K, ../../20181203053506.GB2860387@rfd.leadboat.com/2-unaffected-gustnado.XML.xz)
download
[application/octet-stream] unaffected-gustnado.PML.xz (122.5K, ../../20181203053506.GB2860387@rfd.leadboat.com/3-unaffected-gustnado.PML.xz)
download
^ permalink raw reply [nested|flat] 33+ messages in thread
* Re: "could not reattach to shared memory" on buildfarm member dory
@ 2019-01-17 08:27 Noah Misch <noah@leadboat.com>
parent: Noah Misch <noah@leadboat.com>
1 sibling, 1 reply; 33+ messages in thread
From: Noah Misch @ 2019-01-17 08:27 UTC (permalink / raw)
To: Heath Lord <heath.lord@crunchydata.com>; Stephen Frost <sfrost@snowman.net>; +Cc: Andres Freund <andres@anarazel.de>; Thomas Munro <thomas.munro@enterprisedb.com>; PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>; Tom Lane <tgl@sss.pgh.pa.us>
On Sun, Dec 02, 2018 at 09:35:06PM -0800, Noah Misch wrote:
> On Tue, Sep 25, 2018 at 08:05:12AM -0700, Noah Misch wrote:
> > On Mon, Sep 24, 2018 at 01:53:05PM -0400, Tom Lane wrote:
> > > Overall, I agree that neither of these approaches are exactly attractive.
> > > We're paying a heck of a lot of performance or complexity to solve a
> > > problem that shouldn't even be there, and that we don't understand well.
> > > In particular, the theory that some privileged code is injecting a thread
> > > into every new process doesn't square with my results at
> > > https://www.postgresql.org/message-id/15345.1525145612%40sss.pgh.pa.us
> > >
> > > I think our best course of action at this point is to do nothing until
> > > we have a clearer understanding of what's actually happening on dory.
> > > Perhaps such understanding will yield an idea for a less painful fix.
> >
> > I see.
>
> Could one of you having a dory login use
> https://live.sysinternals.com/Procmon.exe to capture process events during
> backend startup?
Ping.
> The ideal would be one capture where startup failed reattach
> and another where it succeeded, but having the successful run alone would be a
> good start. The procedure is roughly this:
>
> - Install PostgreSQL w/ debug symbols.
> - Start a postmaster.
> - procmon /nomonitor
> - procmon "Filter" menu -> Enable Advanced Output
> - Ctrl-l, add filter for "Process Name" is "postgres.exe"
> - Ctrl-e (starts collecting data)
> - psql (leave it running)
> - After ~60s, Ctrl-e again in procmon (stops collecting data)
> - File -> Save -> PML
> - File -> Save -> XML, include stack traces, resolve stack symbols
> - Compress the PML and XML files, and mail them here
^ permalink raw reply [nested|flat] 33+ messages in thread
* Re: "could not reattach to shared memory" on buildfarm member dory
@ 2019-01-29 16:28 Heath Lord <heath.lord@crunchydata.com>
parent: Noah Misch <noah@leadboat.com>
0 siblings, 1 reply; 33+ messages in thread
From: Heath Lord @ 2019-01-29 16:28 UTC (permalink / raw)
To: Noah Misch <noah@leadboat.com>; +Cc: Stephen Frost <sfrost@snowman.net>; Andres Freund <andres@anarazel.de>; Thomas Munro <thomas.munro@enterprisedb.com>; PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>; Tom Lane <tgl@sss.pgh.pa.us>; Joseph Ayers <joseph.ayers@crunchydata.com>
On Thu, Jan 17, 2019 at 3:27 AM Noah Misch <noah@leadboat.com> wrote:
> On Sun, Dec 02, 2018 at 09:35:06PM -0800, Noah Misch wrote:
> > On Tue, Sep 25, 2018 at 08:05:12AM -0700, Noah Misch wrote:
> > > On Mon, Sep 24, 2018 at 01:53:05PM -0400, Tom Lane wrote:
> > > > Overall, I agree that neither of these approaches are exactly
> attractive.
> > > > We're paying a heck of a lot of performance or complexity to solve a
> > > > problem that shouldn't even be there, and that we don't understand
> well.
> > > > In particular, the theory that some privileged code is injecting a
> thread
> > > > into every new process doesn't square with my results at
> > > >
> https://www.postgresql.org/message-id/15345.1525145612%40sss.pgh.pa.us
> > > >
> > > > I think our best course of action at this point is to do nothing
> until
> > > > we have a clearer understanding of what's actually happening on dory.
> > > > Perhaps such understanding will yield an idea for a less painful fix.
> > >
> > > I see.
> >
> > Could one of you having a dory login use
> > https://live.sysinternals.com/Procmon.exe to capture process events
> during
> > backend startup?
>
> Ping.
>
> > The ideal would be one capture where startup failed reattach
> > and another where it succeeded, but having the successful run alone
> would be a
> > good start. The procedure is roughly this:
> >
> > - Install PostgreSQL w/ debug symbols.
> > - Start a postmaster.
> > - procmon /nomonitor
> > - procmon "Filter" menu -> Enable Advanced Output
> > - Ctrl-l, add filter for "Process Name" is "postgres.exe"
> > - Ctrl-e (starts collecting data)
> > - psql (leave it running)
> > - After ~60s, Ctrl-e again in procmon (stops collecting data)
> > - File -> Save -> PML
> > - File -> Save -> XML, include stack traces, resolve stack symbols
> > - Compress the PML and XML files, and mail them here
>
Noah,
I apologize for not responding earlier, but we attempting to capture the
information you requested. However, where would you like us to pull the
install for PostgreSQL with the debug symbols in it? We are aware of the
BigSQL and EDB installers, but are unsure if these contain the debug
symbols. Currently this machine has nothing on it other than the necessary
dependencies to build postgres for the community. If you could please give
us some more information on what you would like us to install to gather
this information to help debug the issue we are seeing we would really
appreciate it. Also, if there is any additional information on what we
have installed on the server or how we are configured please just let us
know and we will get you that as soon as possible. Thank you again for
your interest in this issue.
-Heath
^ permalink raw reply [nested|flat] 33+ messages in thread
* Re: "could not reattach to shared memory" on buildfarm member dory
@ 2019-01-31 05:04 Noah Misch <noah@leadboat.com>
parent: Heath Lord <heath.lord@crunchydata.com>
0 siblings, 0 replies; 33+ messages in thread
From: Noah Misch @ 2019-01-31 05:04 UTC (permalink / raw)
To: Heath Lord <heath.lord@crunchydata.com>; +Cc: Stephen Frost <sfrost@snowman.net>; Andres Freund <andres@anarazel.de>; Thomas Munro <thomas.munro@enterprisedb.com>; PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>; Tom Lane <tgl@sss.pgh.pa.us>; Joseph Ayers <joseph.ayers@crunchydata.com>
On Tue, Jan 29, 2019 at 11:28:56AM -0500, Heath Lord wrote:
> On Thu, Jan 17, 2019 at 3:27 AM Noah Misch <noah@leadboat.com> wrote:
> > On Sun, Dec 02, 2018 at 09:35:06PM -0800, Noah Misch wrote:
> > > Could one of you having a dory login use
> > > https://live.sysinternals.com/Procmon.exe to capture process events
> > during
> > > backend startup?
> > > The ideal would be one capture where startup failed reattach
> > > and another where it succeeded, but having the successful run alone
> > would be a
> > > good start. The procedure is roughly this:
> > >
> > > - Install PostgreSQL w/ debug symbols.
> > > - Start a postmaster.
> > > - procmon /nomonitor
> > > - procmon "Filter" menu -> Enable Advanced Output
> > > - Ctrl-l, add filter for "Process Name" is "postgres.exe"
> > > - Ctrl-e (starts collecting data)
> > > - psql (leave it running)
> > > - After ~60s, Ctrl-e again in procmon (stops collecting data)
> > > - File -> Save -> PML
> > > - File -> Save -> XML, include stack traces, resolve stack symbols
> > > - Compress the PML and XML files, and mail them here
>
> I apologize for not responding earlier, but we attempting to capture the
> information you requested. However, where would you like us to pull the
> install for PostgreSQL with the debug symbols in it? We are aware of the
> BigSQL and EDB installers, but are unsure if these contain the debug
> symbols.
Please use a locally-built PostgreSQL as similar as possible to what dory's
buildfarm runs produce, except building with debug symbols. A
potentially-convenient way to achieve this would be to add CONFIG=>'Debug' to
build_env of your buildfarm configuration, then run the buildfarm client with
--keepall. By the end of the run, $buildroot/$branch/inst will contain an
installation. (I haven't actually tested that.) You can copy that installation
to any convenient place (e.g. your home directory) and use that copy for the
remaining steps.
> Currently this machine has nothing on it other than the necessary
> dependencies to build postgres for the community. If you could please give
> us some more information on what you would like us to install to gather
> this information to help debug the issue we are seeing we would really
> appreciate it.
At the moment, I think the only other thing you'll need is to download
https://live.sysinternals.com/Procmon.exe.
> Also, if there is any additional information on what we
> have installed on the server or how we are configured please just let us
> know and we will get you that as soon as possible.
Thanks. I don't have specific questions about that at this time, but feel free
to share any lists or config dumps that seem relevant.
^ permalink raw reply [nested|flat] 33+ messages in thread
* Re: "could not reattach to shared memory" on buildfarm member dory
@ 2019-04-02 13:54 Noah Misch <noah@leadboat.com>
parent: Noah Misch <noah@leadboat.com>
1 sibling, 1 reply; 33+ messages in thread
From: Noah Misch @ 2019-04-02 13:54 UTC (permalink / raw)
To: Heath Lord <heath.lord@crunchydata.com>; Stephen Frost <sfrost@snowman.net>; Tom Lane <tgl@sss.pgh.pa.us>; +Cc: Andres Freund <andres@anarazel.de>; Thomas Munro <thomas.munro@enterprisedb.com>; PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>; joseph.ayers@crunchydata.com
On Sun, Dec 02, 2018 at 09:35:06PM -0800, Noah Misch wrote:
> On Tue, Sep 25, 2018 at 08:05:12AM -0700, Noah Misch wrote:
> > On Mon, Sep 24, 2018 at 01:53:05PM -0400, Tom Lane wrote:
> > > Overall, I agree that neither of these approaches are exactly attractive.
> > > We're paying a heck of a lot of performance or complexity to solve a
> > > problem that shouldn't even be there, and that we don't understand well.
> > > In particular, the theory that some privileged code is injecting a thread
> > > into every new process doesn't square with my results at
> > > https://www.postgresql.org/message-id/15345.1525145612%40sss.pgh.pa.us
> > >
> > > I think our best course of action at this point is to do nothing until
> > > we have a clearer understanding of what's actually happening on dory.
> > > Perhaps such understanding will yield an idea for a less painful fix.
>
> Could one of you having a dory login use
> https://live.sysinternals.com/Procmon.exe to capture process events during
> backend startup? The ideal would be one capture where startup failed reattach
> and another where it succeeded, but having the successful run alone would be a
> good start.
Joseph Ayers provided, off-list, the capture from a successful startup. It
wasn't materially different from the one my system generates, so I abandoned
that line of inquiry. Having explored other aspects of the problem, I expect
the attached fix will work. I can reproduce the 4 MiB allocations described
in https://postgr.es/m/29823.1525132900@sss.pgh.pa.us; a few times per
"vcregress check", they emerge in the middle of PGSharedMemoryReAttach(). On
my system, there's 5.7 MiB of free address space just before UsedShmemSegAddr,
so the 4 MiB allocation fits in there, and PGSharedMemoryReAttach() does not
fail. Still, it's easy to imagine that boring variations between environments
could surface dory's problem by reducing that free 5.7 MiB to, say, 3.9 MiB.
The 4 MiB allocations are stacks for new threads of the default thread
pool[1]. (I confirmed that by observing their size change when I changed
StackReserveSize in MSBuildProject.pm and by checking all stack pointers with
"thread apply all info frame" in gdb.) The API calls in
PGSharedMemoryReAttach() don't cause the thread creation; it's a timing
coincidence. Commit 2307868 would have worked around the problem, but
pg_usleep() is essentially a no-op on Windows before
pgwin32_signal_initialize() runs. (I'll push Assert(pgwin32_signal_event) to
some functions.) While one fix is to block until all expected threads have
started, that could be notably slow, and I don't know how to implement it
cleanly. I think a better fix is to arrange for the system to prefer a
different address space region for these thread stacks; for details, see the
first comment the patch adds to win32_shmem.c. This works here.
> backend startup sees six thread creations:
>
> 1. main thread
> 2. thread created before postgres.exe has control
> 3. thread created before postgres.exe has control
> 4. thread created before postgres.exe has control
> 5. in pgwin32_signal_initialize()
> 6. in src\backend\port\win32\timer.c:setitimer()
>
> Threads 2-4 exit exactly 30s after creation. If we fail to reattach to shared
> memory, we'll exit before reaching code to start 5 or 6.
Threads 2-4 proved to be worker threads of the default thread pool.
[1] https://docs.microsoft.com/en-us/windows/desktop/ProcThread/thread-pools
diff --git a/src/backend/port/win32/signal.c b/src/backend/port/win32/signal.c
index dd91fe5..7d8961f 100644
diff --git a/src/backend/port/win32_shmem.c b/src/backend/port/win32_shmem.c
index bc1e946..eed17c5 100644
--- a/src/backend/port/win32_shmem.c
+++ b/src/backend/port/win32_shmem.c
@@ -17,6 +17,28 @@
#include "storage/ipc.h"
#include "storage/pg_shmem.h"
+/*
+ * Early in a process's life, Windows asynchronously creates threads for the
+ * process's "default thread pool"
+ * (https://docs.microsoft.com/en-us/windows/desktop/ProcThread/thread-pools).
+ * Occasionally, thread creation allocates a stack after
+ * PGSharedMemoryReAttach() has released UsedShmemSegAddr and before it has
+ * mapped shared memory at UsedShmemSegAddr. This would cause mapping to fail
+ * if the allocator preferred the just-released region for allocating the new
+ * thread stack. We observed such failures in some Windows Server 2016
+ * configurations. To give the system another region to prefer, reserve and
+ * release an additional, protective region immediately before reserving or
+ * releasing shared memory. The idea is that, if the allocator handed out
+ * REGION1 pages before REGION2 pages at one occasion, it will do so whenever
+ * both regions are free. Windows Server 2016 exhibits that behavior, and a
+ * system behaving differently would have less need to protect
+ * UsedShmemSegAddr. The protective region must be at least large enough for
+ * one thread stack. However, ten times as much is less than 2% of the 32-bit
+ * address space and is negligible relative to the 64-bit address space.
+ */
+#define PROTECTIVE_REGION_SIZE (10 * WIN32_STACK_RLIMIT)
+void *ShmemProtectiveRegion = NULL;
+
HANDLE UsedShmemSegID = INVALID_HANDLE_VALUE;
void *UsedShmemSegAddr = NULL;
static Size UsedShmemSegSize = 0;
@@ -192,6 +214,12 @@ PGSharedMemoryCreate(Size size, bool makePrivate, int port,
Size orig_size = size;
DWORD flProtect = PAGE_READWRITE;
+ ShmemProtectiveRegion = VirtualAlloc(NULL, PROTECTIVE_REGION_SIZE,
+ MEM_RESERVE, PAGE_NOACCESS);
+ if (ShmemProtectiveRegion == NULL)
+ elog(FATAL, "could not reserve memory region: error code %lu",
+ GetLastError());
+
/* Room for a header? */
Assert(size > MAXALIGN(sizeof(PGShmemHeader)));
@@ -370,9 +398,9 @@ retry:
* an already existing shared memory segment, using the handle inherited from
* the postmaster.
*
- * UsedShmemSegID and UsedShmemSegAddr are implicit parameters to this
- * routine. The caller must have already restored them to the postmaster's
- * values.
+ * ShmemProtectiveRegion, UsedShmemSegID and UsedShmemSegAddr are implicit
+ * parameters to this routine. The caller must have already restored them to
+ * the postmaster's values.
*/
void
PGSharedMemoryReAttach(void)
@@ -380,12 +408,16 @@ PGSharedMemoryReAttach(void)
PGShmemHeader *hdr;
void *origUsedShmemSegAddr = UsedShmemSegAddr;
+ Assert(ShmemProtectiveRegion != NULL);
Assert(UsedShmemSegAddr != NULL);
Assert(IsUnderPostmaster);
/*
- * Release memory region reservation that was made by the postmaster
+ * Release memory region reservations made by the postmaster
*/
+ if (VirtualFree(ShmemProtectiveRegion, 0, MEM_RELEASE) == 0)
+ elog(FATAL, "failed to release reserved memory region (addr=%p): error code %lu",
+ ShmemProtectiveRegion, GetLastError());
if (VirtualFree(UsedShmemSegAddr, 0, MEM_RELEASE) == 0)
elog(FATAL, "failed to release reserved memory region (addr=%p): error code %lu",
UsedShmemSegAddr, GetLastError());
@@ -414,13 +446,14 @@ PGSharedMemoryReAttach(void)
* The child process startup logic might or might not call PGSharedMemoryDetach
* after this; make sure that it will be a no-op if called.
*
- * UsedShmemSegID and UsedShmemSegAddr are implicit parameters to this
- * routine. The caller must have already restored them to the postmaster's
- * values.
+ * ShmemProtectiveRegion, UsedShmemSegID and UsedShmemSegAddr are implicit
+ * parameters to this routine. The caller must have already restored them to
+ * the postmaster's values.
*/
void
PGSharedMemoryNoReAttach(void)
{
+ Assert(ShmemProtectiveRegion != NULL);
Assert(UsedShmemSegAddr != NULL);
Assert(IsUnderPostmaster);
@@ -447,12 +480,25 @@ PGSharedMemoryNoReAttach(void)
* Rather, this is for subprocesses that have inherited an attachment and want
* to get rid of it.
*
- * UsedShmemSegID and UsedShmemSegAddr are implicit parameters to this
- * routine.
+ * ShmemProtectiveRegion, UsedShmemSegID and UsedShmemSegAddr are implicit
+ * parameters to this routine.
*/
void
PGSharedMemoryDetach(void)
{
+ /*
+ * Releasing the protective region liberates an unimportant quantity of
+ * address space, but be tidy.
+ */
+ if (ShmemProtectiveRegion != NULL)
+ {
+ if (VirtualFree(ShmemProtectiveRegion, 0, MEM_RELEASE) == 0)
+ elog(LOG, "failed to release reserved memory region (addr=%p): error code %lu",
+ ShmemProtectiveRegion, GetLastError());
+
+ ShmemProtectiveRegion = NULL;
+ }
+
/* Unmap the view, if it's mapped */
if (UsedShmemSegAddr != NULL)
{
@@ -510,19 +556,22 @@ pgwin32_ReserveSharedMemoryRegion(HANDLE hChild)
{
void *address;
+ Assert(ShmemProtectiveRegion != NULL);
Assert(UsedShmemSegAddr != NULL);
Assert(UsedShmemSegSize != 0);
- address = VirtualAllocEx(hChild, UsedShmemSegAddr, UsedShmemSegSize,
- MEM_RESERVE, PAGE_READWRITE);
+ /* ShmemProtectiveRegion */
+ address = VirtualAllocEx(hChild, ShmemProtectiveRegion,
+ PROTECTIVE_REGION_SIZE,
+ MEM_RESERVE, PAGE_NOACCESS);
if (address == NULL)
{
/* Don't use FATAL since we're running in the postmaster */
elog(LOG, "could not reserve shared memory region (addr=%p) for child %p: error code %lu",
- UsedShmemSegAddr, hChild, GetLastError());
+ ShmemProtectiveRegion, hChild, GetLastError());
return false;
}
- if (address != UsedShmemSegAddr)
+ if (address != ShmemProtectiveRegion)
{
/*
* Should never happen - in theory if allocation granularity causes
@@ -531,8 +580,23 @@ pgwin32_ReserveSharedMemoryRegion(HANDLE hChild)
* Don't use FATAL since we're running in the postmaster.
*/
elog(LOG, "reserved shared memory region got incorrect address %p, expected %p",
+ address, ShmemProtectiveRegion);
+ return false;
+ }
+
+ /* UsedShmemSegAddr */
+ address = VirtualAllocEx(hChild, UsedShmemSegAddr, UsedShmemSegSize,
+ MEM_RESERVE, PAGE_READWRITE);
+ if (address == NULL)
+ {
+ elog(LOG, "could not reserve shared memory region (addr=%p) for child %p: error code %lu",
+ UsedShmemSegAddr, hChild, GetLastError());
+ return false;
+ }
+ if (address != UsedShmemSegAddr)
+ {
+ elog(LOG, "reserved shared memory region got incorrect address %p, expected %p",
address, UsedShmemSegAddr);
- VirtualFreeEx(hChild, address, 0, MEM_RELEASE);
return false;
}
diff --git a/src/backend/postmaster/postmaster.c b/src/backend/postmaster/postmaster.c
index fe59963..6ab1dda 100644
--- a/src/backend/postmaster/postmaster.c
+++ b/src/backend/postmaster/postmaster.c
@@ -484,6 +484,7 @@ typedef struct
#ifndef WIN32
unsigned long UsedShmemSegID;
#else
+ void *ShmemProtectiveRegion;
HANDLE UsedShmemSegID;
#endif
void *UsedShmemSegAddr;
@@ -6011,6 +6012,9 @@ save_backend_variables(BackendParameters *param, Port *port,
param->MyCancelKey = MyCancelKey;
param->MyPMChildSlot = MyPMChildSlot;
+#ifdef WIN32
+ param->ShmemProtectiveRegion = ShmemProtectiveRegion;
+#endif
param->UsedShmemSegID = UsedShmemSegID;
param->UsedShmemSegAddr = UsedShmemSegAddr;
@@ -6244,6 +6248,9 @@ restore_backend_variables(BackendParameters *param, Port *port)
MyCancelKey = param->MyCancelKey;
MyPMChildSlot = param->MyPMChildSlot;
+#ifdef WIN32
+ ShmemProtectiveRegion = param->ShmemProtectiveRegion;
+#endif
UsedShmemSegID = param->UsedShmemSegID;
UsedShmemSegAddr = param->UsedShmemSegAddr;
diff --git a/src/include/storage/pg_shmem.h b/src/include/storage/pg_shmem.h
index 57d3107..4e1e13d 100644
--- a/src/include/storage/pg_shmem.h
+++ b/src/include/storage/pg_shmem.h
@@ -65,6 +65,7 @@ typedef enum
extern unsigned long UsedShmemSegID;
#else
extern HANDLE UsedShmemSegID;
+extern void *ShmemProtectiveRegion;
#endif
extern void *UsedShmemSegAddr;
diff --git a/src/port/open.c b/src/port/open.c
index 436cbae..f37afc7 100644
Attachments:
[text/plain] shmem-protective-region-v1.patch (8.2K, ../../20190402135442.GA1173872@rfd.leadboat.com/2-shmem-protective-region-v1.patch)
download | inline diff:
diff --git a/src/backend/port/win32/signal.c b/src/backend/port/win32/signal.c
index dd91fe5..7d8961f 100644
diff --git a/src/backend/port/win32_shmem.c b/src/backend/port/win32_shmem.c
index bc1e946..eed17c5 100644
--- a/src/backend/port/win32_shmem.c
+++ b/src/backend/port/win32_shmem.c
@@ -17,6 +17,28 @@
#include "storage/ipc.h"
#include "storage/pg_shmem.h"
+/*
+ * Early in a process's life, Windows asynchronously creates threads for the
+ * process's "default thread pool"
+ * (https://docs.microsoft.com/en-us/windows/desktop/ProcThread/thread-pools).
+ * Occasionally, thread creation allocates a stack after
+ * PGSharedMemoryReAttach() has released UsedShmemSegAddr and before it has
+ * mapped shared memory at UsedShmemSegAddr. This would cause mapping to fail
+ * if the allocator preferred the just-released region for allocating the new
+ * thread stack. We observed such failures in some Windows Server 2016
+ * configurations. To give the system another region to prefer, reserve and
+ * release an additional, protective region immediately before reserving or
+ * releasing shared memory. The idea is that, if the allocator handed out
+ * REGION1 pages before REGION2 pages at one occasion, it will do so whenever
+ * both regions are free. Windows Server 2016 exhibits that behavior, and a
+ * system behaving differently would have less need to protect
+ * UsedShmemSegAddr. The protective region must be at least large enough for
+ * one thread stack. However, ten times as much is less than 2% of the 32-bit
+ * address space and is negligible relative to the 64-bit address space.
+ */
+#define PROTECTIVE_REGION_SIZE (10 * WIN32_STACK_RLIMIT)
+void *ShmemProtectiveRegion = NULL;
+
HANDLE UsedShmemSegID = INVALID_HANDLE_VALUE;
void *UsedShmemSegAddr = NULL;
static Size UsedShmemSegSize = 0;
@@ -192,6 +214,12 @@ PGSharedMemoryCreate(Size size, bool makePrivate, int port,
Size orig_size = size;
DWORD flProtect = PAGE_READWRITE;
+ ShmemProtectiveRegion = VirtualAlloc(NULL, PROTECTIVE_REGION_SIZE,
+ MEM_RESERVE, PAGE_NOACCESS);
+ if (ShmemProtectiveRegion == NULL)
+ elog(FATAL, "could not reserve memory region: error code %lu",
+ GetLastError());
+
/* Room for a header? */
Assert(size > MAXALIGN(sizeof(PGShmemHeader)));
@@ -370,9 +398,9 @@ retry:
* an already existing shared memory segment, using the handle inherited from
* the postmaster.
*
- * UsedShmemSegID and UsedShmemSegAddr are implicit parameters to this
- * routine. The caller must have already restored them to the postmaster's
- * values.
+ * ShmemProtectiveRegion, UsedShmemSegID and UsedShmemSegAddr are implicit
+ * parameters to this routine. The caller must have already restored them to
+ * the postmaster's values.
*/
void
PGSharedMemoryReAttach(void)
@@ -380,12 +408,16 @@ PGSharedMemoryReAttach(void)
PGShmemHeader *hdr;
void *origUsedShmemSegAddr = UsedShmemSegAddr;
+ Assert(ShmemProtectiveRegion != NULL);
Assert(UsedShmemSegAddr != NULL);
Assert(IsUnderPostmaster);
/*
- * Release memory region reservation that was made by the postmaster
+ * Release memory region reservations made by the postmaster
*/
+ if (VirtualFree(ShmemProtectiveRegion, 0, MEM_RELEASE) == 0)
+ elog(FATAL, "failed to release reserved memory region (addr=%p): error code %lu",
+ ShmemProtectiveRegion, GetLastError());
if (VirtualFree(UsedShmemSegAddr, 0, MEM_RELEASE) == 0)
elog(FATAL, "failed to release reserved memory region (addr=%p): error code %lu",
UsedShmemSegAddr, GetLastError());
@@ -414,13 +446,14 @@ PGSharedMemoryReAttach(void)
* The child process startup logic might or might not call PGSharedMemoryDetach
* after this; make sure that it will be a no-op if called.
*
- * UsedShmemSegID and UsedShmemSegAddr are implicit parameters to this
- * routine. The caller must have already restored them to the postmaster's
- * values.
+ * ShmemProtectiveRegion, UsedShmemSegID and UsedShmemSegAddr are implicit
+ * parameters to this routine. The caller must have already restored them to
+ * the postmaster's values.
*/
void
PGSharedMemoryNoReAttach(void)
{
+ Assert(ShmemProtectiveRegion != NULL);
Assert(UsedShmemSegAddr != NULL);
Assert(IsUnderPostmaster);
@@ -447,12 +480,25 @@ PGSharedMemoryNoReAttach(void)
* Rather, this is for subprocesses that have inherited an attachment and want
* to get rid of it.
*
- * UsedShmemSegID and UsedShmemSegAddr are implicit parameters to this
- * routine.
+ * ShmemProtectiveRegion, UsedShmemSegID and UsedShmemSegAddr are implicit
+ * parameters to this routine.
*/
void
PGSharedMemoryDetach(void)
{
+ /*
+ * Releasing the protective region liberates an unimportant quantity of
+ * address space, but be tidy.
+ */
+ if (ShmemProtectiveRegion != NULL)
+ {
+ if (VirtualFree(ShmemProtectiveRegion, 0, MEM_RELEASE) == 0)
+ elog(LOG, "failed to release reserved memory region (addr=%p): error code %lu",
+ ShmemProtectiveRegion, GetLastError());
+
+ ShmemProtectiveRegion = NULL;
+ }
+
/* Unmap the view, if it's mapped */
if (UsedShmemSegAddr != NULL)
{
@@ -510,19 +556,22 @@ pgwin32_ReserveSharedMemoryRegion(HANDLE hChild)
{
void *address;
+ Assert(ShmemProtectiveRegion != NULL);
Assert(UsedShmemSegAddr != NULL);
Assert(UsedShmemSegSize != 0);
- address = VirtualAllocEx(hChild, UsedShmemSegAddr, UsedShmemSegSize,
- MEM_RESERVE, PAGE_READWRITE);
+ /* ShmemProtectiveRegion */
+ address = VirtualAllocEx(hChild, ShmemProtectiveRegion,
+ PROTECTIVE_REGION_SIZE,
+ MEM_RESERVE, PAGE_NOACCESS);
if (address == NULL)
{
/* Don't use FATAL since we're running in the postmaster */
elog(LOG, "could not reserve shared memory region (addr=%p) for child %p: error code %lu",
- UsedShmemSegAddr, hChild, GetLastError());
+ ShmemProtectiveRegion, hChild, GetLastError());
return false;
}
- if (address != UsedShmemSegAddr)
+ if (address != ShmemProtectiveRegion)
{
/*
* Should never happen - in theory if allocation granularity causes
@@ -531,8 +580,23 @@ pgwin32_ReserveSharedMemoryRegion(HANDLE hChild)
* Don't use FATAL since we're running in the postmaster.
*/
elog(LOG, "reserved shared memory region got incorrect address %p, expected %p",
+ address, ShmemProtectiveRegion);
+ return false;
+ }
+
+ /* UsedShmemSegAddr */
+ address = VirtualAllocEx(hChild, UsedShmemSegAddr, UsedShmemSegSize,
+ MEM_RESERVE, PAGE_READWRITE);
+ if (address == NULL)
+ {
+ elog(LOG, "could not reserve shared memory region (addr=%p) for child %p: error code %lu",
+ UsedShmemSegAddr, hChild, GetLastError());
+ return false;
+ }
+ if (address != UsedShmemSegAddr)
+ {
+ elog(LOG, "reserved shared memory region got incorrect address %p, expected %p",
address, UsedShmemSegAddr);
- VirtualFreeEx(hChild, address, 0, MEM_RELEASE);
return false;
}
diff --git a/src/backend/postmaster/postmaster.c b/src/backend/postmaster/postmaster.c
index fe59963..6ab1dda 100644
--- a/src/backend/postmaster/postmaster.c
+++ b/src/backend/postmaster/postmaster.c
@@ -484,6 +484,7 @@ typedef struct
#ifndef WIN32
unsigned long UsedShmemSegID;
#else
+ void *ShmemProtectiveRegion;
HANDLE UsedShmemSegID;
#endif
void *UsedShmemSegAddr;
@@ -6011,6 +6012,9 @@ save_backend_variables(BackendParameters *param, Port *port,
param->MyCancelKey = MyCancelKey;
param->MyPMChildSlot = MyPMChildSlot;
+#ifdef WIN32
+ param->ShmemProtectiveRegion = ShmemProtectiveRegion;
+#endif
param->UsedShmemSegID = UsedShmemSegID;
param->UsedShmemSegAddr = UsedShmemSegAddr;
@@ -6244,6 +6248,9 @@ restore_backend_variables(BackendParameters *param, Port *port)
MyCancelKey = param->MyCancelKey;
MyPMChildSlot = param->MyPMChildSlot;
+#ifdef WIN32
+ ShmemProtectiveRegion = param->ShmemProtectiveRegion;
+#endif
UsedShmemSegID = param->UsedShmemSegID;
UsedShmemSegAddr = param->UsedShmemSegAddr;
diff --git a/src/include/storage/pg_shmem.h b/src/include/storage/pg_shmem.h
index 57d3107..4e1e13d 100644
--- a/src/include/storage/pg_shmem.h
+++ b/src/include/storage/pg_shmem.h
@@ -65,6 +65,7 @@ typedef enum
extern unsigned long UsedShmemSegID;
#else
extern HANDLE UsedShmemSegID;
+extern void *ShmemProtectiveRegion;
#endif
extern void *UsedShmemSegAddr;
diff --git a/src/port/open.c b/src/port/open.c
index 436cbae..f37afc7 100644
^ permalink raw reply [nested|flat] 33+ messages in thread
* Re: "could not reattach to shared memory" on buildfarm member dory
@ 2019-04-02 14:09 Tom Lane <tgl@sss.pgh.pa.us>
parent: Noah Misch <noah@leadboat.com>
0 siblings, 1 reply; 33+ messages in thread
From: Tom Lane @ 2019-04-02 14:09 UTC (permalink / raw)
To: Noah Misch <noah@leadboat.com>; +Cc: Heath Lord <heath.lord@crunchydata.com>; Stephen Frost <sfrost@snowman.net>; Andres Freund <andres@anarazel.de>; Thomas Munro <thomas.munro@enterprisedb.com>; PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>; joseph.ayers@crunchydata.com
Noah Misch <noah@leadboat.com> writes:
> I can reproduce the 4 MiB allocations described
> in https://postgr.es/m/29823.1525132900@sss.pgh.pa.us; a few times per
> "vcregress check", they emerge in the middle of PGSharedMemoryReAttach().
> The 4 MiB allocations are stacks for new threads of the default thread
> pool[1].
Hah! It is great to finally have an understanding of what is happening
here.
I worry that your proposed fix is unstable, in particular this assumption
seems shaky:
> + * ... The idea is that, if the allocator handed out
> + * REGION1 pages before REGION2 pages at one occasion, it will do so whenever
> + * both regions are free.
I wonder whether it's possible to address this by configuring the "default
thread pool" to have only one thread? It seems like the extra threads are
just adding backend startup overhead to no benefit, since we won't use 'em.
regards, tom lane
^ permalink raw reply [nested|flat] 33+ messages in thread
* Re: "could not reattach to shared memory" on buildfarm member dory
@ 2019-04-07 04:32 Noah Misch <noah@leadboat.com>
parent: Tom Lane <tgl@sss.pgh.pa.us>
0 siblings, 1 reply; 33+ messages in thread
From: Noah Misch @ 2019-04-07 04:32 UTC (permalink / raw)
To: Tom Lane <tgl@sss.pgh.pa.us>; +Cc: Heath Lord <heath.lord@crunchydata.com>; Stephen Frost <sfrost@snowman.net>; Andres Freund <andres@anarazel.de>; Thomas Munro <thomas.munro@enterprisedb.com>; PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>; joseph.ayers@crunchydata.com
On Tue, Apr 02, 2019 at 10:09:00AM -0400, Tom Lane wrote:
> Noah Misch <noah@leadboat.com> writes:
> > I can reproduce the 4 MiB allocations described
> > in https://postgr.es/m/29823.1525132900@sss.pgh.pa.us; a few times per
> > "vcregress check", they emerge in the middle of PGSharedMemoryReAttach().
> > The 4 MiB allocations are stacks for new threads of the default thread
> > pool[1].
>
> Hah! It is great to finally have an understanding of what is happening
> here.
>
> I worry that your proposed fix is unstable, in particular this assumption
> seems shaky:
>
> > + * ... The idea is that, if the allocator handed out
> > + * REGION1 pages before REGION2 pages at one occasion, it will do so whenever
> > + * both regions are free.
True. If Windows changes to prefer allocating from the most recently freed
region, shmem-protective-region-v1.patch would cease to help. It's not
impossible.
> I wonder whether it's possible to address this by configuring the "default
> thread pool" to have only one thread? It seems like the extra threads are
> just adding backend startup overhead to no benefit, since we won't use 'em.
I didn't find a way to configure the pool's size.
Another option is to reattach shared memory earlier, before the default thread
pool starts. A Windows application using only the unavoidable DLLs
(kernel32.dll, ntdll.dll, kernelbase.dll) doesn't get a default thread pool;
the pool starts when one loads ws2_32.dll, ucrtbased.dll, etc. Hence, the
DllMain() function of a DLL that loads early could avoid the problem. (Cygwin
fork() uses that route to remap shared memory, though it also retries failed
forks.) If we proceed that way, we'd add a tiny pg_shmem.dll that appears
early in the load order, just after the unavoidable DLLs. It would extract
applicable parameters from the command line and reattach shared memory. When
it fails, it would set a flag so the usual code can raise an error. Does that
sound more or less attractive than shmem-protective-region-v1.patch?
^ permalink raw reply [nested|flat] 33+ messages in thread
* Re: "could not reattach to shared memory" on buildfarm member dory
@ 2019-04-07 04:43 Tom Lane <tgl@sss.pgh.pa.us>
parent: Noah Misch <noah@leadboat.com>
0 siblings, 1 reply; 33+ messages in thread
From: Tom Lane @ 2019-04-07 04:43 UTC (permalink / raw)
To: Noah Misch <noah@leadboat.com>; +Cc: Heath Lord <heath.lord@crunchydata.com>; Stephen Frost <sfrost@snowman.net>; Andres Freund <andres@anarazel.de>; Thomas Munro <thomas.munro@enterprisedb.com>; PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>; joseph.ayers@crunchydata.com
Noah Misch <noah@leadboat.com> writes:
> On Tue, Apr 02, 2019 at 10:09:00AM -0400, Tom Lane wrote:
>> I worry that your proposed fix is unstable, in particular this assumption
>> seems shaky:
>>> + * ... The idea is that, if the allocator handed out
>>> + * REGION1 pages before REGION2 pages at one occasion, it will do so whenever
>>> + * both regions are free.
> True. If Windows changes to prefer allocating from the most recently freed
> region, shmem-protective-region-v1.patch would cease to help. It's not
> impossible.
>> I wonder whether it's possible to address this by configuring the "default
>> thread pool" to have only one thread? It seems like the extra threads are
>> just adding backend startup overhead to no benefit, since we won't use 'em.
> I didn't find a way to configure the pool's size.
Seems odd ...
> Another option is to reattach shared memory earlier, before the default thread
> pool starts. A Windows application using only the unavoidable DLLs
> (kernel32.dll, ntdll.dll, kernelbase.dll) doesn't get a default thread pool;
> the pool starts when one loads ws2_32.dll, ucrtbased.dll, etc. Hence, the
> DllMain() function of a DLL that loads early could avoid the problem. (Cygwin
> fork() uses that route to remap shared memory, though it also retries failed
> forks.) If we proceed that way, we'd add a tiny pg_shmem.dll that appears
> early in the load order, just after the unavoidable DLLs. It would extract
> applicable parameters from the command line and reattach shared memory. When
> it fails, it would set a flag so the usual code can raise an error. Does that
> sound more or less attractive than shmem-protective-region-v1.patch?
Well, it definitely sounds like more work. Let's not do such work
until we have to. Your patch has the advantages of being (a) small
and (b) done, and there's much to be said for that.
regards, tom lane
^ permalink raw reply [nested|flat] 33+ messages in thread
* Re: "could not reattach to shared memory" on buildfarm member dory
@ 2019-04-09 04:54 Noah Misch <noah@leadboat.com>
parent: Tom Lane <tgl@sss.pgh.pa.us>
0 siblings, 0 replies; 33+ messages in thread
From: Noah Misch @ 2019-04-09 04:54 UTC (permalink / raw)
To: Tom Lane <tgl@sss.pgh.pa.us>; +Cc: Heath Lord <heath.lord@crunchydata.com>; Stephen Frost <sfrost@snowman.net>; Andres Freund <andres@anarazel.de>; Thomas Munro <thomas.munro@enterprisedb.com>; PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>; joseph.ayers@crunchydata.com
On Sun, Apr 07, 2019 at 12:43:23AM -0400, Tom Lane wrote:
> Noah Misch <noah@leadboat.com> writes:
> > On Tue, Apr 02, 2019 at 10:09:00AM -0400, Tom Lane wrote:
> >> I worry that your proposed fix is unstable, in particular this assumption
> >> seems shaky:
> >>> + * ... The idea is that, if the allocator handed out
> >>> + * REGION1 pages before REGION2 pages at one occasion, it will do so whenever
> >>> + * both regions are free.
>
> > True. If Windows changes to prefer allocating from the most recently freed
> > region, shmem-protective-region-v1.patch would cease to help. It's not
> > impossible.
>
> >> I wonder whether it's possible to address this by configuring the "default
> >> thread pool" to have only one thread? It seems like the extra threads are
> >> just adding backend startup overhead to no benefit, since we won't use 'em.
>
> > I didn't find a way to configure the pool's size.
>
> Seems odd ...
I think you're expected to create your own thread pool if you're picky about
the size. The default thread pool is for non-picky callers and for Windows
libraries to use internally. The fact that the pool starts before main() also
narrows the use case for configuring it. If there were an API for "block
until all threads of the pool are alive", that would meet our needs nicely.
Needless to say, I didn't find that, either.
> > Another option is to reattach shared memory earlier, before the default thread
> > pool starts. A Windows application using only the unavoidable DLLs
> > (kernel32.dll, ntdll.dll, kernelbase.dll) doesn't get a default thread pool;
> > the pool starts when one loads ws2_32.dll, ucrtbased.dll, etc. Hence, the
> > DllMain() function of a DLL that loads early could avoid the problem. (Cygwin
> > fork() uses that route to remap shared memory, though it also retries failed
> > forks.) If we proceed that way, we'd add a tiny pg_shmem.dll that appears
> > early in the load order, just after the unavoidable DLLs. It would extract
> > applicable parameters from the command line and reattach shared memory. When
> > it fails, it would set a flag so the usual code can raise an error. Does that
> > sound more or less attractive than shmem-protective-region-v1.patch?
>
> Well, it definitely sounds like more work. Let's not do such work
> until we have to. Your patch has the advantages of being (a) small
> and (b) done, and there's much to be said for that.
Works for me. Pushed.
^ permalink raw reply [nested|flat] 33+ messages in thread
end of thread, other threads:[~2019-04-09 04:54 UTC | newest]
Thread overview: 33+ messages (download: mbox mbox.gz follow: Atom feed)
-- links below jump to the message on this page --
2018-04-23 21:10 "could not reattach to shared memory" on buildfarm member dory Tom Lane <tgl@sss.pgh.pa.us>
2018-04-23 23:18 ` Stephen Frost <sfrost@snowman.net>
2018-04-23 23:37 ` Thomas Munro <thomas.munro@enterprisedb.com>
2018-04-24 07:06 ` Noah Misch <noah@leadboat.com>
2018-04-28 02:06 ` Tom Lane <tgl@sss.pgh.pa.us>
2018-04-28 13:59 ` Stephen Frost <sfrost@snowman.net>
2018-04-30 17:07 ` Tom Lane <tgl@sss.pgh.pa.us>
2018-04-30 18:34 ` Stephen Frost <sfrost@snowman.net>
2018-04-30 19:25 ` Heath Lord <heath.lord@crunchydata.com>
2018-04-30 19:29 ` Stephen Frost <sfrost@snowman.net>
2018-04-30 19:45 ` Tom Lane <tgl@sss.pgh.pa.us>
2018-05-01 00:01 ` Tom Lane <tgl@sss.pgh.pa.us>
2018-05-01 00:24 ` Andres Freund <andres@anarazel.de>
2018-05-01 04:13 ` Tom Lane <tgl@sss.pgh.pa.us>
2018-05-01 15:31 ` Tom Lane <tgl@sss.pgh.pa.us>
2018-08-19 02:00 ` Noah Misch <noah@leadboat.com>
2018-09-24 17:53 ` Tom Lane <tgl@sss.pgh.pa.us>
2018-09-25 15:05 ` Noah Misch <noah@leadboat.com>
2018-12-03 05:35 ` Noah Misch <noah@leadboat.com>
2019-01-17 08:27 ` Noah Misch <noah@leadboat.com>
2019-01-29 16:28 ` Heath Lord <heath.lord@crunchydata.com>
2019-01-31 05:04 ` Noah Misch <noah@leadboat.com>
2019-04-02 13:54 ` Noah Misch <noah@leadboat.com>
2019-04-02 14:09 ` Tom Lane <tgl@sss.pgh.pa.us>
2019-04-07 04:32 ` Noah Misch <noah@leadboat.com>
2019-04-07 04:43 ` Tom Lane <tgl@sss.pgh.pa.us>
2019-04-09 04:54 ` Noah Misch <noah@leadboat.com>
2018-05-01 02:59 ` Noah Misch <noah@leadboat.com>
2018-05-01 03:33 ` Tom Lane <tgl@sss.pgh.pa.us>
2018-05-01 03:52 ` Thomas Munro <thomas.munro@enterprisedb.com>
2018-04-24 07:18 ` Magnus Hagander <magnus@hagander.net>
2018-04-26 10:48 ` Craig Ringer <craig@2ndquadrant.com>
2018-04-27 12:26 ` Stephen Frost <sfrost@snowman.net>
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox