Daniel Gustafsson <daniel@yesql.se> writes: >> On 14 Dec 2022, at 13:58, Mats Kindahl <mats@timescale.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.
