Re: [HACKERS] Re: [COMMITTERS] pgsql: Reserve the shared memory region during backend startup on
On Wed, Aug 5, 2009 at 16:53, Tom Lanet...@sss.pgh.pa.us wrote: Magnus Hagander mag...@hagander.net writes: But. I'll look into cleaning those up for HEAD anyway, but due to lack of reports I think we should skip backpatch. Reasonable? Fair enough. Here's what I came up with. Seems ok? Works for me. Applied. -- Magnus Hagander Me: http://www.hagander.net/ Work: http://www.redpill-linpro.com/ -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Re: [COMMITTERS] pgsql: Reserve the shared memory region during backend startup on
On Tue, Jul 28, 2009 at 15:45, Tom Lanet...@sss.pgh.pa.us wrote:
Magnus Hagander mag...@hagander.net writes:
On Mon, Jul 27, 2009 at 16:14, Tom Lanet...@sss.pgh.pa.us wrote:
I'm not really insisting on a redesign. I'm talking about the places
where the code author appears not to have understood that ERROR means
FATAL, because the code keeps plugging on after it anyway. As far as
I can see, using ERROR at lines 3630, 3657, 3674, and 3693 is just
plain bogus, and changing to LOG there requires no other fixing.
But. I'll look into cleaning those up for HEAD anyway, but due to lack
of reports I think we should skip backpatch. Reasonable?
Fair enough.
Here's what I came up with. Seems ok?
--
Magnus Hagander
Me: http://www.hagander.net/
Work: http://www.redpill-linpro.com/
*** a/src/backend/postmaster/postmaster.c
--- b/src/backend/postmaster/postmaster.c
***
*** 3627,3633 internal_forkexec(int argc, char *argv[], Port *port)
* mess with the half-started process
*/
if (!TerminateProcess(pi.hProcess, 255))
! ereport(ERROR,
(errmsg_internal(could not terminate unstarted process: error code %d,
(int) GetLastError(;
CloseHandle(pi.hProcess);
--- 3627,3633
* mess with the half-started process
*/
if (!TerminateProcess(pi.hProcess, 255))
! ereport(LOG,
(errmsg_internal(could not terminate unstarted process: error code %d,
(int) GetLastError(;
CloseHandle(pi.hProcess);
***
*** 3654,3660 internal_forkexec(int argc, char *argv[], Port *port)
* process and give up.
*/
if (!TerminateProcess(pi.hProcess, 255))
! ereport(ERROR,
(errmsg_internal(could not terminate process that failed to reserve memory: error code %d,
(int) GetLastError(;
CloseHandle(pi.hProcess);
--- 3654,3660
* process and give up.
*/
if (!TerminateProcess(pi.hProcess, 255))
! ereport(LOG,
(errmsg_internal(could not terminate process that failed to reserve memory: error code %d,
(int) GetLastError(;
CloseHandle(pi.hProcess);
***
*** 3671,3677 internal_forkexec(int argc, char *argv[], Port *port)
{
if (!TerminateProcess(pi.hProcess, 255))
{
! ereport(ERROR,
(errmsg_internal(could not terminate unstartable process: error code %d,
(int) GetLastError(;
CloseHandle(pi.hProcess);
--- 3671,3677
{
if (!TerminateProcess(pi.hProcess, 255))
{
! ereport(LOG,
(errmsg_internal(could not terminate unstartable process: error code %d,
(int) GetLastError(;
CloseHandle(pi.hProcess);
***
*** 3680,3686 internal_forkexec(int argc, char *argv[], Port *port)
}
CloseHandle(pi.hProcess);
CloseHandle(pi.hThread);
! ereport(ERROR,
(errmsg_internal(could not resume thread of unstarted process: error code %d,
(int) GetLastError(;
return -1;
--- 3680,3686
}
CloseHandle(pi.hProcess);
CloseHandle(pi.hThread);
! ereport(LOG,
(errmsg_internal(could not resume thread of unstarted process: error code %d,
(int) GetLastError(;
return -1;
***
*** 4430,4437 extern int pgStatSock;
#define write_inheritable_socket(dest, src, childpid) (*(dest) = (src))
#define read_inheritable_socket(dest, src) (*(dest) = *(src))
#else
! static void write_duplicated_handle(HANDLE *dest, HANDLE src, HANDLE child);
! static void write_inheritable_socket(InheritableSocket *dest, SOCKET src,
pid_t childPid);
static void read_inheritable_socket(SOCKET *dest, InheritableSocket *src);
#endif
--- 4430,4437
#define write_inheritable_socket(dest, src, childpid) (*(dest) = (src))
#define read_inheritable_socket(dest, src) (*(dest) = *(src))
#else
! static bool write_duplicated_handle(HANDLE *dest, HANDLE src, HANDLE child);
! static bool write_inheritable_socket(InheritableSocket *dest, SOCKET src,
pid_t childPid);
static void read_inheritable_socket(SOCKET *dest, InheritableSocket *src);
#endif
***
*** 4448,4454 save_backend_variables(BackendParameters *param, Port *port,
#endif
{
memcpy(param-port, port, sizeof(Port));
! write_inheritable_socket(param-portsocket, port-sock, childPid);
strlcpy(param-DataDir, DataDir, MAXPGPATH);
--- 4448,4455
#endif
{
memcpy(param-port, port, sizeof(Port));
! if (!write_inheritable_socket(param-portsocket, port-sock, childPid))
! return false;
strlcpy(param-DataDir, DataDir, MAXPGPATH);
***
*** 4469,4475 save_backend_variables(BackendParameters *param, Port *port,
param-ProcGlobal = ProcGlobal;
param-AuxiliaryProcs = AuxiliaryProcs;
param-PMSignalState = PMSignalState;
! write_inheritable_socket(param-pgStatSock, pgStatSock, childPid);
param-PostmasterPid = PostmasterPid;
param-PgStartTime =
Re: [HACKERS] Re: [COMMITTERS] pgsql: Reserve the shared memory region during backend startup on
Magnus Hagander mag...@hagander.net writes: But. I'll look into cleaning those up for HEAD anyway, but due to lack of reports I think we should skip backpatch. Reasonable? Fair enough. Here's what I came up with. Seems ok? Works for me. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Re: [COMMITTERS] pgsql: Reserve the shared memory region during backend startup on
On Mon, Jul 27, 2009 at 16:14, Tom Lanet...@sss.pgh.pa.us wrote: Magnus Hagander mag...@hagander.net writes: To fix that we'd just have to turn those functions all into returning boolean and log with LOG instead. AFAIK, we've had zero reports of this actually happening, so I'm not sure it's worth redesigning. Thoughts? I'm not really insisting on a redesign. I'm talking about the places where the code author appears not to have understood that ERROR means FATAL, because the code keeps plugging on after it anyway. As far as I can see, using ERROR at lines 3630, 3657, 3674, and 3693 is just plain bogus, and changing to LOG there requires no other fixing. 3630: can't happen, because we already elog(ERROR) deep in the function, which is what I tried to outline above. That's the one requiring a redesign - because the errors *inside* the function it calls can certainly happen. 3657: is one of those should never happen issues - which we haven't had any reports of. 3674: see above 3693 is a comment, I assume you mean 3683, which is also one of those can't happen. But. I'll look into cleaning those up for HEAD anyway, but due to lack of reports I think we should skip backpatch. Reasonable? -- Magnus Hagander Self: http://www.hagander.net/ Work: http://www.redpill-linpro.com/ -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Re: [COMMITTERS] pgsql: Reserve the shared memory region during backend startup on
Magnus Hagander mag...@hagander.net writes: On Mon, Jul 27, 2009 at 16:14, Tom Lanet...@sss.pgh.pa.us wrote: I'm not really insisting on a redesign. I'm talking about the places where the code author appears not to have understood that ERROR means FATAL, because the code keeps plugging on after it anyway. As far as I can see, using ERROR at lines 3630, 3657, 3674, and 3693 is just plain bogus, and changing to LOG there requires no other fixing. But. I'll look into cleaning those up for HEAD anyway, but due to lack of reports I think we should skip backpatch. Reasonable? Fair enough. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] Re: [COMMITTERS] pgsql: Reserve the shared memory region during backend startup on
On Sat, Jul 25, 2009 at 19:50, Tom Lanet...@sss.pgh.pa.us wrote: m...@postgresql.org (Magnus Hagander) writes: Log Message: --- Reserve the shared memory region during backend startup on Windows, so that memory allocated by starting third party DLLs doesn't end up conflicting with it. I am wondering why failure of the various TerminateProcess calls in postmaster.c is elog(ERROR) and not elog(LOG). While that probably shouldn't happen, aborting the postmaster isn't a good response if it does. This patch introduces a new occurrence, but I see it is just copying what was there already. The case where it's doing it now is really a can't happen place, so I don't think it's a big issue there. It could be argued that if we can't terminate a process we just created (but never even started!), something is very very badly broken on the system and we might as well give up. Same for the part where we fail to ResumeThread() on the main thread of a new process. However, it seems that for example the one at line 3629 - where we're just failing to save our backend state - shouldn't be such a FATAL error. But if you actually look up into the function save_backend_variables(), it's actually hardcoded to return true... In case something goes wrong in there, there's an actual ereport(ERROR) happening deep down already (write_inheritable_socket/write_duplicated_handle). To fix that we'd just have to turn those functions all into returning boolean and log with LOG instead. AFAIK, we've had zero reports of this actually happening, so I'm not sure it's worth redesigning. Thoughts? -- Magnus Hagander Self: http://www.hagander.net/ Work: http://www.redpill-linpro.com/ -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Re: [COMMITTERS] pgsql: Reserve the shared memory region during backend startup on
Magnus Hagander mag...@hagander.net writes: To fix that we'd just have to turn those functions all into returning boolean and log with LOG instead. AFAIK, we've had zero reports of this actually happening, so I'm not sure it's worth redesigning. Thoughts? I'm not really insisting on a redesign. I'm talking about the places where the code author appears not to have understood that ERROR means FATAL, because the code keeps plugging on after it anyway. As far as I can see, using ERROR at lines 3630, 3657, 3674, and 3693 is just plain bogus, and changing to LOG there requires no other fixing. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
