From: Tom Lane <tgl@sss.pgh.pa.us>
To: Daniel Gustafsson <daniel@yesql.se>
cc: Mats Kindahl <mats@timescale.com>, pgsql-bugs@lists.postgresql.org
Subject: Re: Crash during backend start when low on memory
In-reply-to: <D79B2B42-E76C-4F07-985A-895312747CE8@yesql.se>
References: 
 <CA+144248yrDNSLFUvRfWZShbX7TWY5seKig2iOm2S5hrvjOYuA@mail.gmail.com>
 <D79B2B42-E76C-4F07-985A-895312747CE8@yesql.se>
Comments: In-reply-to Daniel Gustafsson <daniel@yesql.se>
	message dated "Wed, 14 Dec 2022 14:23:29 +0100"
MIME-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-ID: <2686250.1671033262.1@sss.pgh.pa.us>
Content-Transfer-Encoding: quoted-printable
Date: Wed, 14 Dec 2022 10:54:22 -0500
Message-ID: <2686251.1671033262@sss.pgh.pa.us>
Archived-At: 
 <https://www.postgresql.org/message-id/2686251.1671033262%40sss.pgh.pa.us>
Precedence: bulk

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.

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.)

			regards, tom lane