Re: Crash during backend start when low on memory

On Wed, Dec 14, 2022 at 4:54 PM Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:

> Daniel Gustafsson <daniel(at)yesql(dot)se> writes:
> >> On 14 Dec 2022, at 13:58, Mats Kindahl <mats(at)timescale(dot)com> wrote:
> >> A tentative patch to fix this just to check the allocation and exit the
> process if there is not enough memory, taking care to not try to allocate
> more memory. I used this patch (attached as well).
>
> > Something like this (with the comment in the right syntax) seems like an
> > appropriate fix.
>
> I don't like that approach one bit. Extending it to all the places where
> this could theoretically happen would require a large amount of thought,
> because you'd need a custom solution for each place. That is completely
> unwarranted for what's basically (IMO) an academic exercise. The right
> thing is to s/strdup/something/g, which we can do pretty mindlessly once
> we settle on the "something". I'd prefer that the something be pstrdup,
> because otherwise future hackers will have to stop and think whether
> they should be using pstrdup or something else when hacking in session
> startup code. As you say, we do have to think through which context
> needs to be active for that to work. I'd also be okay with deciding that
> we need an explicit MemoryContextStrdup in some places, as long as
> it's pretty clear which places need that.
>

I think we can use the PostmasterContext and I agree that it is a lot
clearer and consistent to use pstrdup() and friends everywhere.

>
> BTW, I think it's also pretty tenable to decide "this is a non-problem,
> let's ignore it". Yeah, the backend will crash and provoke a DB reset
> cycle, but if you're that hard up for memory (a) that is likely to happen
> somewhere else soon anyway, and (b) a reset cycle might not be such a
> bad thing, as it could relieve the memory pressure. I'm especially not
> pleased with the prospect of writing a bunch of nigh-untestable code
> that makes extremely dubious claims of being able to cope. (Hint:
> proc_exit() almost certainly results in memory allocations. And
> I think the claim that neither strerror_r nor write_stderr does
> is based on little beyond wishful thinking. Especially so if
> write_stderr is forwarding to the log collector.)
>

I don't think the first patch should be used, but added it to the previous
mail so that you can look at it. Using pstrdup() makes the code consistent,
is a trivial patch, and avoids a lot of unnecessary discussions and
questions, so I think there is value in adding this patch, even if this is
not a common occurrence and even if this is an edge case.

Best wishes,
Mats Kindahl, Timescale

> regards, tom lane
>