Re: [HACKERS] unchecked out of memory in postmaster.c
Tom Lane wrote: > Alvaro Herrera writes: > > Tom Lane wrote: > >> If you're really intent on doing something about this, my inclination > >> would be to get rid of the dependence on DLNewElem altogether. Add > >> a Dlelem field to the Backend struct and use DLInitElem (compare > >> the way catcache uses that module). > > > Hmm, yeah, I had seen that code. So it looks like this instead. > > Huh, didn't you commit this yet? Sorry, I just did. -- Alvaro Herrerahttp://www.CommandPrompt.com/ PostgreSQL Replication, Consulting, Custom Development, 24x7 support -- 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] unchecked out of memory in postmaster.c
Alvaro Herrera writes: > Tom Lane wrote: >> If you're really intent on doing something about this, my inclination >> would be to get rid of the dependence on DLNewElem altogether. Add >> a Dlelem field to the Backend struct and use DLInitElem (compare >> the way catcache uses that module). > Hmm, yeah, I had seen that code. So it looks like this instead. Huh, didn't you commit this yet? 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] unchecked out of memory in postmaster.c
Alvaro Herrera writes: > Tom Lane wrote: >> If you're really intent on doing something about this, my inclination >> would be to get rid of the dependence on DLNewElem altogether. Add >> a Dlelem field to the Backend struct and use DLInitElem (compare >> the way catcache uses that module). > Hmm, yeah, I had seen that code. So it looks like this instead. Looks a lot nicer to 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] unchecked out of memory in postmaster.c
Tom Lane wrote: > Alvaro Herrera writes: > > Tom Lane wrote: > >> I guess I need to point out that those ereport calls already pose a far > >> more substantial risk of palloc failure than the DLNewElem represents. > > > Hmm, do they? I mean, don't they use ErrorContext, which is supposed to > > be preallocated? > > Well, we'd like to think that they pose an insignificant risk, but it's > hard to credit that DLNewElem isn't insignificant as well. Yeah, actually as I said earlier, I haven't ever seen this. > If you're really intent on doing something about this, my inclination > would be to get rid of the dependence on DLNewElem altogether. Add > a Dlelem field to the Backend struct and use DLInitElem (compare > the way catcache uses that module). Hmm, yeah, I had seen that code. So it looks like this instead. -- Alvaro Herrerahttp://www.CommandPrompt.com/ The PostgreSQL Company - Command Prompt, Inc. Index: src/backend/postmaster/postmaster.c === RCS file: /home/alvherre/Code/cvs/pgsql/src/backend/postmaster/postmaster.c,v retrieving revision 1.551.2.1 diff -c -p -r1.551.2.1 postmaster.c *** src/backend/postmaster/postmaster.c 27 Jun 2008 01:53:31 - 1.551.2.1 --- src/backend/postmaster/postmaster.c 7 Apr 2009 02:25:51 - *** typedef struct bkend *** 142,147 --- 142,148 long cancel_key; /* cancel key for cancels for this backend */ bool is_autovacuum; /* is it an autovacuum process? */ bool dead_end; /* is it going to send an error and quit? */ + Dlelem elem; /* self pointer into BackendList */ } Backend; static Dllist *BackendList; *** CleanupBackend(int pid, *** 2343,2349 #endif DLRemove(curr); free(bp); - DLFreeElem(curr); break; } } --- 2344,2349 *** HandleChildCrash(int pid, int exitstatus *** 2390,2396 #endif DLRemove(curr); free(bp); - DLFreeElem(curr); /* Keep looping so we can signal remaining backends */ } else --- 2390,2395 *** BackendStartup(Port *port) *** 2857,2863 bn->cancel_key = MyCancelKey; bn->is_autovacuum = false; bn->dead_end = (port->canAcceptConnections != CAC_OK); ! DLAddHead(BackendList, DLNewElem(bn)); #ifdef EXEC_BACKEND if (!bn->dead_end) ShmemBackendArrayAdd(bn); --- 2856,2864 bn->cancel_key = MyCancelKey; bn->is_autovacuum = false; bn->dead_end = (port->canAcceptConnections != CAC_OK); ! ! DLInitElem(&bn->elem, bn); ! DLAddHead(BackendList, &bn->elem); #ifdef EXEC_BACKEND if (!bn->dead_end) ShmemBackendArrayAdd(bn); -- 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] unchecked out of memory in postmaster.c
Alvaro Herrera writes: > Tom Lane wrote: >> I guess I need to point out that those ereport calls already pose a far >> more substantial risk of palloc failure than the DLNewElem represents. > Hmm, do they? I mean, don't they use ErrorContext, which is supposed to > be preallocated? Well, we'd like to think that they pose an insignificant risk, but it's hard to credit that DLNewElem isn't insignificant as well. If you're really intent on doing something about this, my inclination would be to get rid of the dependence on DLNewElem altogether. Add a Dlelem field to the Backend struct and use DLInitElem (compare the way catcache uses that module). 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] unchecked out of memory in postmaster.c
Tom Lane wrote: > Alvaro Herrera writes: > > I think a patch to solve this is as simple as the attached. > > I guess I need to point out that those ereport calls already pose a far > more substantial risk of palloc failure than the DLNewElem represents. Hmm, do they? I mean, don't they use ErrorContext, which is supposed to be preallocated? > You seem to have forgotten about releasing the DLElem if the fork fails, > too. Doh, sorry about that. -- Alvaro Herrerahttp://www.CommandPrompt.com/ PostgreSQL Replication, Consulting, Custom Development, 24x7 support -- 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] unchecked out of memory in postmaster.c
Alvaro Herrera writes: > I think a patch to solve this is as simple as the attached. I guess I need to point out that those ereport calls already pose a far more substantial risk of palloc failure than the DLNewElem represents. You seem to have forgotten about releasing the DLElem if the fork fails, too. 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] unchecked out of memory in postmaster.c
Tom Lane wrote:
> Alvaro Herrera writes:
> > Some time ago I noticed that in postmaster.c there's a corner case which
> > probably causes postmaster to exit in out-of-memory condition. See
> > BackendStartup, near the bottom, there's a call to DLNewElem(). The
> > problem is that this function calls palloc() and thus can elog(ERROR) on
> > OOM, but postmaster has no way to defend itself from this and would die.
>
> So? There are probably hundreds of palloc calls that are reachable from
> the postmaster main loop. If this were allocating more than a few bytes
> of memory, it might be worth worrying about.
Hundreds? I think you'd be hard pressed to find as much as a dozen :-)
I mean stuff that's called inside ServerLoop, of course. There are a
few places calling alloc-type functions, but as far as I see they use
calloc or malloc, and behave "sanely" (i.e. not elog(ERROR)) in OOM.
Note that BackendStartup itself is very careful about allocating a
Backend struct, even when it's just ~10 bytes on 32 bits machines.
I think a patch to solve this is as simple as the attached.
--
Alvaro Herrerahttp://www.CommandPrompt.com/
The PostgreSQL Company - Command Prompt, Inc.
Index: src/backend/postmaster/postmaster.c
===
RCS file: /home/alvherre/Code/cvs/pgsql/src/backend/postmaster/postmaster.c,v
retrieving revision 1.551.2.1
diff -c -p -r1.551.2.1 postmaster.c
*** src/backend/postmaster/postmaster.c 27 Jun 2008 01:53:31 - 1.551.2.1
--- src/backend/postmaster/postmaster.c 6 Apr 2009 23:14:00 -
*** BackendStartup(Port *port)
*** 2774,2779
--- 2774,2780
{
Backend*bn;/* for backend cleanup */
pid_t pid;
+ Dlelem *newel;
/*
* Compute the cancel key that will be assigned to this backend. The
*** BackendStartup(Port *port)
*** 2794,2799
--- 2795,2818
errmsg("out of memory")));
return STATUS_ERROR;
}
+ PG_TRY();
+ {
+ newel = DLNewElem(bn);
+ elog(ERROR, "out of luck :-(");
+ }
+ PG_CATCH();
+ {
+ /*
+ * it would be great if we could just reduce the error severity of the
+ * current error
+ */
+ FlushErrorState();
+ ereport(LOG,
+ (errcode(ERRCODE_OUT_OF_MEMORY),
+ errmsg("out of memory")));
+ return STATUS_ERROR;
+ }
+ PG_END_TRY();
/* Pass down canAcceptConnections state (kluge for EXEC_BACKEND case) */
port->canAcceptConnections = canAcceptConnections();
*** BackendStartup(Port *port)
*** 2857,2863
bn->cancel_key = MyCancelKey;
bn->is_autovacuum = false;
bn->dead_end = (port->canAcceptConnections != CAC_OK);
! DLAddHead(BackendList, DLNewElem(bn));
#ifdef EXEC_BACKEND
if (!bn->dead_end)
ShmemBackendArrayAdd(bn);
--- 2876,2883
bn->cancel_key = MyCancelKey;
bn->is_autovacuum = false;
bn->dead_end = (port->canAcceptConnections != CAC_OK);
!
! DLAddHead(BackendList, newel);
#ifdef EXEC_BACKEND
if (!bn->dead_end)
ShmemBackendArrayAdd(bn);
--
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] unchecked out of memory in postmaster.c
Alvaro Herrera writes: > Some time ago I noticed that in postmaster.c there's a corner case which > probably causes postmaster to exit in out-of-memory condition. See > BackendStartup, near the bottom, there's a call to DLNewElem(). The > problem is that this function calls palloc() and thus can elog(ERROR) on > OOM, but postmaster has no way to defend itself from this and would die. So? There are probably hundreds of palloc calls that are reachable from the postmaster main loop. If this were allocating more than a few bytes of memory, it might be worth worrying about. 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] unchecked out of memory in postmaster.c
Hi, Some time ago I noticed that in postmaster.c there's a corner case which probably causes postmaster to exit in out-of-memory condition. See BackendStartup, near the bottom, there's a call to DLNewElem(). The problem is that this function calls palloc() and thus can elog(ERROR) on OOM, but postmaster has no way to defend itself from this and would die. I haven't ever seen postmaster die from this, but I don't think it's a good idea to let it be like this, given the strict promises we make about its reliability. Probably a simple PG_TRY block around the DLNewElem call suffices ...? -- Alvaro Herrerahttp://www.CommandPrompt.com/ The PostgreSQL Company - Command Prompt, Inc. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
