From: | Mats Kindahl <mats(at)timescale(dot)com> |
---|---|
To: | Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> |
Cc: | Daniel Gustafsson <daniel(at)yesql(dot)se>, pgsql-bugs(at)lists(dot)postgresql(dot)org |
Subject: | Re: Crash during backend start when low on memory |
Date: | 2022-12-14 21:21:10 |
Message-ID: | CA+14424FHsFJpgW1Qb22qCtM1DKJgRNUL0c7-XMRSfZDGT97rA@mail.gmail.com |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | pgsql-bugs |
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
>
From | Date | Subject | |
---|---|---|---|
Next Message | Nathaniel Hazelton | 2022-12-14 22:14:07 | Re: BUG #17721: A completely unused CTE negatively affect Query Plan |
Previous Message | Mats Kindahl | 2022-12-14 21:14:19 | Re: Crash during backend start when low on memory |