Re: Crash during backend start when low on memory

On Fri, Jan 13, 2023 at 12:58 PM Alvaro Herrera <alvherre(at)alvh(dot)no-ip(dot)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)
>