public inbox for pgsql-bugs@postgresql.org
help / color / mirror / Atom feedFrom: Tom Lane <tgl@sss.pgh.pa.us>
To: Alexander Lakhin <exclusion@gmail.com>
Cc: pgsql-bugs@lists.postgresql.org
Subject: Re: BUG #18374: Printing memory contexts on OOM condition might lead to segmentation fault
Date: Sun, 03 Mar 2024 16:39:29 -0500
Message-ID: <3399097.1709501969@sss.pgh.pa.us> (raw)
In-Reply-To: <3148162.1709409519@sss.pgh.pa.us>
References: <18374-ebb8113ce4d02f0d@postgresql.org>
<3120721.1709395887@sss.pgh.pa.us>
<b1a1eaf3-d5b7-da52-6bb7-c5b3fbe47f3e@gmail.com>
<3140126.1709405398@sss.pgh.pa.us>
<3148162.1709409519@sss.pgh.pa.us>
I wrote:
> I find this in [1]:
>
> The C language stack growth does an implicit mremap. If you want absolute
> guarantees and run close to the edge you MUST mmap your stack for the
> largest size you think you will need. For typical stack usage this does
> not matter much but it's a corner case if you really really care
>
> Seems like we need to do some more work at startup to enforce that
> we have the amount of stack we think we do, if we're on Linux.
After thinking about that some more, I'm really quite unenthused about
trying to remap the stack for ourselves. It'd be both platform- and
architecture-dependent, and I'm afraid it'd introduce as many failure
modes as it removes. (Notably, I'm not sure we could guarantee
there's a guard page below the stack.) Since we've not seen reports
of this failure from the wild, I doubt it's worth the trouble.
I do think it's probably worth reducing MemoryContextDelete's stack
usage to O(1), just to ensure we can't get into stack trouble during
transaction abort. That's not hard at all, as attached.
I tried to make MemoryContextResetChildren work similarly, but that
doesn't work because if we're not removing child contexts then we
need extra state to tell which ones we've done already. For the
same reason my idea for bounding the stack space needed by
MemoryContextStats doesn't seem to work. We could possibly make it
work if we were willing to add a temporary-use pointer field to all
MemoryContext headers, but I'm unconvinced that'd be a good tradeoff.
regards, tom lane
Attachments:
[text/x-diff] memory-context-delete-with-fixed-stack-space.patch (1.4K, 2-memory-context-delete-with-fixed-stack-space.patch)
download | inline diff:
diff --git a/src/backend/utils/mmgr/mcxt.c b/src/backend/utils/mmgr/mcxt.c
index 41f2390fb8..7cc220bca0 100644
--- a/src/backend/utils/mmgr/mcxt.c
+++ b/src/backend/utils/mmgr/mcxt.c
@@ -447,14 +447,37 @@ MemoryContextDelete(MemoryContext context)
void
MemoryContextDeleteChildren(MemoryContext context)
{
+ MemoryContext cur,
+ next;
+
Assert(MemoryContextIsValid(context));
/*
- * MemoryContextDelete will delink the child from me, so just iterate as
- * long as there is a child.
+ * We iterate rather than recursing, so that cleaning up a deep nest of
+ * contexts doesn't require unbounded stack space. (This avoids possible
+ * failure during transaction cleanup, which would be bad.) This works
+ * because by the time we traverse back up to a parent context, it will
+ * have no remaining children and will be seen as deletable.
*/
- while (context->firstchild != NULL)
- MemoryContextDelete(context->firstchild);
+ cur = context->firstchild;
+ while (cur != NULL)
+ {
+ /* Descend until we find a childless context */
+ if (cur->firstchild != NULL)
+ {
+ cur = cur->firstchild;
+ continue;
+ }
+ /* We can delete cur, but first remember the next deletion target */
+ if (cur->nextchild != NULL)
+ next = cur->nextchild;
+ else if (cur->parent != context)
+ next = cur->parent;
+ else
+ next = NULL;
+ MemoryContextDelete(cur);
+ cur = next;
+ }
}
/*
reply
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: pgsql-bugs@postgresql.org
Cc: tgl@sss.pgh.pa.us, exclusion@gmail.com, pgsql-bugs@lists.postgresql.org
Subject: Re: BUG #18374: Printing memory contexts on OOM condition might lead to segmentation fault
In-Reply-To: <3399097.1709501969@sss.pgh.pa.us>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox