Re: Crash during backend start when low on memory - Mailing list pgsql-bugs
| From | Mats Kindahl |
|---|---|
| Subject | Re: Crash during backend start when low on memory |
| Date | |
| Msg-id | CA+14425-gk-3pRx1A1m8bo4kOteiA1XyxGCarETuQzqdF9yBfA@mail.gmail.com Whole thread Raw |
| In response to | Re: Crash during backend start when low on memory (Alvaro Herrera <alvherre@alvh.no-ip.org>) |
| Responses |
Re: Crash during backend start when low on memory
|
| List | pgsql-bugs |
On Fri, Jan 13, 2023 at 12:58 PM Alvaro Herrera <alvherre@alvh.no-ip.org> wrote:
On 2023-Jan-13, Mats Kindahl wrote:
> I have an improved patch based on raising an error when malloc fails. I
> used the version of mallocfail that is available at
> https://github.com/ralight/mallocfail which allows you to run the program
> with incrementally failing memory allocations but since the count is around
> 6886 or so when the postmaster starts to allow backend starts and other
> processes are starting in between, this count is not always accurate and
> something "smarter" is needed to get a reliable failure test.
Hmm, but I'm not sure I like what you did to BackendStartup:
@@ -4054,14 +4073,7 @@ BackendStartup(Port *port)
* Create backend data structure. Better before the fork() so we can
* handle failure cleanly.
*/
- bn = (Backend *) malloc(sizeof(Backend));
- if (!bn)
- {
- ereport(LOG,
- (errcode(ERRCODE_OUT_OF_MEMORY),
- errmsg("out of memory")));
- return STATUS_ERROR;
- }
+ CHECKED_CALL(bn = malloc(sizeof(Backend)));
With this code, now we will have postmaster raise ERROR if this malloc
fails, rather than returning STATUS_ERROR as previously. How does
postmaster react to this? I think it won't go very well.
(Stylistically, I'd rather have the test and corresponding reaction at
each call site rather than using a macro to hide it. I especially don't
like the fact that the errdetail you propose would report a line of C
code to the user, rather than an intelligible explanation of what that
code is trying to do.)
> It is quite straightforward to test it by attaching a debugger and setting
> the return value of malloc, but to be able to have some "smarter" failures
> only when, for example, starting a backend it would be necessary to have
> something better integrated with the postgres code. Being able to attach a
> uprobe using bpftrace and override the return value would be the most
> elegant solution, but that is currently not possible, so considering
> whether to add something like the mallocfail above to the regular testing.
Hmm, I'm not sure that this type of testing is something to do on an
ongoing basis.
> Note: there are a bunch of other core dumps that occur during startup
> before the postmaster is ready to accept connections, but I think that is
> less of a problem since it fails during startup before the system is ready
> to accept connections.
That sounds reasonable. The service watcher should restart postmaster
appropriately in that case (be it old SysV init, systemd, your container
management layer, or whatever.)
Hi all,
I have gone over the two different approaches and have patches for both attached so that we can compare. The second approach, using palloc() and friends, is in my opinion safer so this is where I have spent most of the time.
- The first approach allocates memory in the postmaster using malloc and raises an error if that fails. If this happens in the backend after it is started, a FATAL error is raised, which will abort the backend and send a useful message to the connected client, but not bring down the postmaster. The errors in the postmaster are caught using a PG_TRY(), which will prevent the postmaster from aborting, and continue executing.
- The second approach creates a new memory context BackendStartupContext where objects that need to live after the backend is forked are allocated. Replace all uses of malloc() and friends with palloc() and friends. Since palloc() can throw an error, we need to catch this using a PG_TRY() inside the postmaster. In the event that the backend process throws an error, we convert this to a FATAL error and re-throw it, which will cause the backend process to terminate and send a useful message to the connected client.
The second approach, using palloc() and friends, is in my opinion less error prone since it will capture any raised errors that prevent the backend from starting, not just memory allocation errors. I have included a file PATCH.md containing information about the manual tests I've done (using a debugger) for both patches. There are a few pieces of memory that are allocated in the PostmasterContext and then copied out from it before destroying it. It would be possible to allocate this in the BackendStartupContext instead (for the palloc() approach) and destroy the PostmasterContext directly after the fork(), which provides a small advantage in being slightly less error prone and slightly easier to maintain.
Using malloc() and friends can be changed to not use ERROR-level reports and in that sense do not require using PG_TRY() and can be re-written to not use that. For palloc() and friends it seems they always run the risk of throwing an ERROR, so without making it behave differently in the postmaster (which seems to add unnecessary complications to the function), it will require PG_TRY() to capture the errors.
Tom had some concerns about using PG_TRY() inside the postmaster ServerLoop, but I cannot see how that can cause problems since it is "just" a setjmp() and longjmp(). It allocates a structure on the stack that contains the values of the registers and the signal set; it is big, but not exceedingly so. If somebody could enlighten me about if there are any problems with this, I would be very grateful.
Best wishes,
Mats Kindahl, Timescale
--
Álvaro Herrera PostgreSQL Developer — https://www.EnterpriseDB.com/
"Ellos andaban todos desnudos como su madre los parió, y también las mujeres,
aunque no vi más que una, harto moza, y todos los que yo vi eran todos
mancebos, que ninguno vi de edad de más de XXX años" (Cristóbal Colón)
Attachment
pgsql-bugs by date: