pg.ddx.io pgsql-bugs@postgresql.org mailing list archive
help / color / mirror / Atom feedCrash during backend start when low on memory
21+ messages / 5 participants
[nested] [flat]
* Crash during backend start when low on memory
@ 2022-12-14 12:58 Mats Kindahl <mats@timescale.com>
2022-12-14 13:23 ` Re: Crash during backend start when low on memory Daniel Gustafsson <daniel@yesql.se>
0 siblings, 1 reply; 21+ messages in thread
From: Mats Kindahl @ 2022-12-14 12:58 UTC (permalink / raw)
To: pgsql-bugs@lists.postgresql.org
Hi all,
We have a crash when starting up a backend that can happen when the system
is low on memory. Even though the system is probably not in good shape, it
seems unnecessary to trigger a crash and it would be preferable to just
deny creating the backend and throw an error.
Inside BackendInitialize, there is this code:
/*
* Save remote_host and remote_port in port structure (after this, they
* will appear in log_line_prefix data for log messages).
*/
port->remote_host = strdup(remote_host);
port->remote_port = strdup(remote_port);
.
.
.
appendStringInfoString(&ps_data, port->remote_host);
if (port->remote_port[0] != '\0')
appendStringInfo(&ps_data, "(%s)", port->remote_port);
There is no assignment to remote_port or remote_host in the lines between.
If strdup() fails to allocate memory for the strings, it will return NULL
and the dereference at the later lines will cause a segmentation fault,
which will bring down the server. There seems to be code that reads the
start packet between these two lines of code, but I can trigger a
segmentation fault without having a correct user. This seems unnecessary.
I can trigger the crash using a debugger:
1. Connect a debugger to the postmaster
2. Set a breakpoint on BackendStartup and continue.
3. Try to connect to the server using psql and use a non-existing user
name.
4. Set follow-fork-mode to child and set a breakpoint on
BackendInitialize and continue executing.
5. Step to the first lines above and set port->remote_port and
port->remote_host to NULL simulating a failed strdup.
6. Continue executing, and the backend will get a segfault and crash the
server.
This is probably not a big problem in normal installations. It requires you
to be out of memory to trigger this and you will run out of backends long
before you run out of memory, but if you're running in constrained
environments or in a container that is pushing the limits of the amount of
memory to allocate, this can trigger the crash.
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).
diff --git a/src/backend/postmaster/postmaster.c
b/src/backend/postmaster/postmaster.c
index 1da5752047..e4b3d1bd59 100644
inline patch 1: src/backend/postmaster/postmaster.c (+2/-0 lines) download inline-1.patch
--- a/src/backend/postmaster/postmaster.c
+++ b/src/backend/postmaster/postmaster.c
@@ -4327,6 +4327,14 @@ BackendInitialize(Port *port)
port->remote_host = strdup(remote_host);
port->remote_port = strdup(remote_port);
+ if (port->remote_host == NULL || port->remote_port == NULL) {
+ /* Since we are out of memory, we use strerror_r and
0001-Use-pstrdup-instead-of-strdup-in-postmaster.patch (text/x-patch, 2.1K) download
From 48814cea13df4591474cd96a90655bf9007ca69d Mon Sep 17 00:00:00 2001
From: Mats Kindahl <mats@timescale.com>
Date: Wed, 14 Dec 2022 13:51:40 +0100
Subject: Use pstrdup instead of strdup in postmaster
There is no check for the return value of strdup() in
BackendInitialize() which means that it is possible to get a
segmentation fault when starting a backend if the system is low on
memory. Instead, use pstrdup() for the allocations that are within the
same process since this cannot return NULL and instead fail with an
error, aborting the start of a backend.
---
src/backend/postmaster/postmaster.c | 10 +++++-----
1 file changed, 5 insertions(+), 5 deletions(-)
diff --git a/src/backend/postmaster/postmaster.c b/src/backend/postmaster/postmaster.c
index f459dab360..8ea09a62f6 100644
--- a/src/backend/postmaster/postmaster.c
+++ b/src/backend/postmaster/postmaster.c
@@ -704,7 +704,7 @@ PostmasterMain(int argc, char *argv[])
break;
case 'C':
- output_config_variable = strdup(optarg);
+ output_config_variable = pstrdup(optarg);
break;
case 'c':
@@ -735,7 +735,7 @@ PostmasterMain(int argc, char *argv[])
}
case 'D':
- userDoption = strdup(optarg);
+ userDoption = pstrdup(optarg);
break;
case 'd':
@@ -4275,8 +4275,8 @@ BackendInitialize(Port *port)
* Save remote_host and remote_port in port structure (after this, they
* will appear in log_line_prefix data for log messages).
*/
- port->remote_host = strdup(remote_host);
- port->remote_port = strdup(remote_port);
+ port->remote_host = pstrdup(remote_host);
+ port->remote_port = pstrdup(remote_port);
/* And now we can issue the Log_connections message, if wanted */
if (Log_connections)
@@ -4307,7 +4307,7 @@ BackendInitialize(Port *port)
ret == 0 &&
strspn(remote_host, "0123456789.") < strlen(remote_host) &&
strspn(remote_host, "0123456789ABCDEFabcdef:") < strlen(remote_host))
- port->remote_hostname = strdup(remote_host);
+ port->remote_hostname = pstrdup(remote_host);
/*
* Ready to begin client interaction. We will give up and _exit(1) after
--
2.34.1
0001-Check-return-value-of-strdup-when-starting-backend.patch (text/x-patch, 2.0K) download
From 5f5684523f47e7d3b20c4efad5a9c335aade7831 Mon Sep 17 00:00:00 2001
From: Mats Kindahl <mats@timescale.com>
Date: Thu, 8 Dec 2022 10:00:23 +0100
Subject: Check return value of strdup when starting backend
Inside `BackendInitialize`, if `strdup` returns NULL because it is out
of memory, this will later trigger a server crash because of
dereferencing a null pointer.
This commit fixes this by checking the return values of `strdup` and
exit the backend with exit status 1 if any of the allocations failed.
This will be handled as a normal shutdown in the postmaster, which will
report the exiting backend and continue executing.
---
src/backend/postmaster/postmaster.c | 14 ++++++++++++++
1 file changed, 14 insertions(+)
diff --git a/src/backend/postmaster/postmaster.c b/src/backend/postmaster/postmaster.c
index f459dab360..eef3b7f85c 100644
--- a/src/backend/postmaster/postmaster.c
+++ b/src/backend/postmaster/postmaster.c
@@ -4278,6 +4278,14 @@ BackendInitialize(Port *port)
port->remote_host = strdup(remote_host);
port->remote_port = strdup(remote_port);
+ if (port->remote_host == NULL || port->remote_port == NULL) {
+ /* Since we are out of memory, we use strerror_r and write_stderr
+ here. They do not allocate memory and just use the stack. */
+ char sebuf[PG_STRERROR_R_BUFLEN];
+ write_stderr("out of memory: %s", strerror_r(errno, sebuf, sizeof(sebuf)));
+ proc_exit(1);
+ }
+
/* And now we can issue the Log_connections message, if wanted */
if (Log_connections)
{
@@ -4307,7 +4315,13 @@ BackendInitialize(Port *port)
ret == 0 &&
strspn(remote_host, "0123456789.") < strlen(remote_host) &&
strspn(remote_host, "0123456789ABCDEFabcdef:") < strlen(remote_host))
+ {
+ /* It is not necessary to check the return value since code that uses
+ * remote_hostname assumes that if it is NULL, the reverse lookup was
+ * not done. */
port->remote_hostname = strdup(remote_host);
+ }
+
/*
* Ready to begin client interaction. We will give up and _exit(1) after
--
2.34.1
* Re: Crash during backend start when low on memory
2022-12-14 12:58 Crash during backend start when low on memory Mats Kindahl <mats@timescale.com>
@ 2022-12-14 13:23 ` Daniel Gustafsson <daniel@yesql.se>
2022-12-14 15:54 ` Re: Crash during backend start when low on memory Tom Lane <tgl@sss.pgh.pa.us>
2022-12-14 21:14 ` Re: Crash during backend start when low on memory Mats Kindahl <mats@timescale.com>
0 siblings, 2 replies; 21+ messages in thread
From: Daniel Gustafsson @ 2022-12-14 13:23 UTC (permalink / raw)
To: Mats Kindahl <mats@timescale.com>; +Cc: pgsql-bugs@lists.postgresql.org
> On 14 Dec 2022, at 13:58, Mats Kindahl <mats@timescale.com> wrote:
> If strdup() fails to allocate memory for the strings, it will return NULL and the dereference at the later lines will cause a segmentation fault, which will bring down the server. There seems to be code that reads the start packet between these two lines of code, but I can trigger a segmentation fault without having a correct user. This seems unnecessary.
Ugh.
> 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).
>
> diff --git a/src/backend/postmaster/postmaster.c b/src/backend/postmaster/postmaster.c
> index 1da5752047..e4b3d1bd59 100644
> --- a/src/backend/postmaster/postmaster.c
> +++ b/src/backend/postmaster/postmaster.c
> @@ -4327,6 +4327,14 @@ BackendInitialize(Port *port)
> port->remote_host = strdup(remote_host);
> port->remote_port = strdup(remote_port);
>
> + if (port->remote_host == NULL || port->remote_port == NULL) {
> + /* Since we are out of memory, we use strerror_r and write_stderr
> + here. They do not allocate memory and just use the stack. */
> + char sebuf[PG_STRERROR_R_BUFLEN];
> + write_stderr("out of memory: %s", strerror_r(errno, sebuf, sizeof(sebuf)));
> + proc_exit(1);
> + }
> +
Something like this (with the comment in the right syntax) seems like an
appropriate fix.
> However, an alternative patch is to not use strdup() or any other functions that allocate memory on the heap and instead use pstrdup(), pmalloc() and friends. Not sure if there are any reasons to not use those in this function, but it seems like the memory context is correctly set up when BackendInitialize() is called. I have attached a patch for that as well. In this case, it is not necessary to check the return value since MemoryContextAlloc() will report an error if it fails to allocate memory and not return NULL.
I think using pstrdup would require changing over to PostmasterContext (or
TopMemoryContext even?) to ensure these allocations are around for the lifetime
of the backend.
A related issue is that we in PostmasterMain strdup into output_config_variable
during option parsing, but we don't check for success. When we then go about
checking if -C was set, we can't tell if it wasn't at all set or if the
strdup() call failed. Another one is the -D option where a failure to strdup
will silently fall back to $PGDATA. Both of these should IMO check for strdup
returning NULL and exit in case it does.
--
Daniel Gustafsson https://vmware.com/
^ permalink raw reply [nested|flat] 21+ messages in thread
* Re: Crash during backend start when low on memory
2022-12-14 12:58 Crash during backend start when low on memory Mats Kindahl <mats@timescale.com>
2022-12-14 13:23 ` Re: Crash during backend start when low on memory Daniel Gustafsson <daniel@yesql.se>
@ 2022-12-14 15:54 ` Tom Lane <tgl@sss.pgh.pa.us>
2022-12-14 21:21 ` Re: Crash during backend start when low on memory Mats Kindahl <mats@timescale.com>
1 sibling, 1 reply; 21+ messages in thread
From: Tom Lane @ 2022-12-14 15:54 UTC (permalink / raw)
To: Daniel Gustafsson <daniel@yesql.se>; +Cc: Mats Kindahl <mats@timescale.com>; pgsql-bugs@lists.postgresql.org
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
^ permalink raw reply [nested|flat] 21+ messages in thread
* Re: Crash during backend start when low on memory
2022-12-14 12:58 Crash during backend start when low on memory Mats Kindahl <mats@timescale.com>
2022-12-14 13:23 ` Re: Crash during backend start when low on memory Daniel Gustafsson <daniel@yesql.se>
2022-12-14 15:54 ` Re: Crash during backend start when low on memory Tom Lane <tgl@sss.pgh.pa.us>
@ 2022-12-14 21:21 ` Mats Kindahl <mats@timescale.com>
0 siblings, 0 replies; 21+ messages in thread
From: Mats Kindahl @ 2022-12-14 21:21 UTC (permalink / raw)
To: Tom Lane <tgl@sss.pgh.pa.us>; +Cc: Daniel Gustafsson <daniel@yesql.se>; pgsql-bugs@lists.postgresql.org
On Wed, Dec 14, 2022 at 4:54 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> 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.
Best wishes,
Mats Kindahl, Timescale
> regards, tom lane
>
^ permalink raw reply [nested|flat] 21+ messages in thread
* Re: Crash during backend start when low on memory
2022-12-14 12:58 Crash during backend start when low on memory Mats Kindahl <mats@timescale.com>
2022-12-14 13:23 ` Re: Crash during backend start when low on memory Daniel Gustafsson <daniel@yesql.se>
@ 2022-12-14 21:14 ` Mats Kindahl <mats@timescale.com>
2022-12-15 17:20 ` Re: Crash during backend start when low on memory Alvaro Herrera <alvherre@alvh.no-ip.org>
1 sibling, 1 reply; 21+ messages in thread
From: Mats Kindahl @ 2022-12-14 21:14 UTC (permalink / raw)
To: daniel@yesql.se; +Cc: pgsql-bugs@lists.postgresql.org
On Wed, Dec 14, 2022 at 2:23 PM Daniel Gustafsson <daniel@yesql.se> wrote:
> > On 14 Dec 2022, at 13:58, Mats Kindahl <mats@timescale.com> wrote:
>
> > If strdup() fails to allocate memory for the strings, it will return
> NULL and the dereference at the later lines will cause a segmentation
> fault, which will bring down the server. There seems to be code that reads
> the start packet between these two lines of code, but I can trigger a
> segmentation fault without having a correct user. This seems unnecessary.
>
> Ugh.
>
> > 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).
> >
> > diff --git a/src/backend/postmaster/postmaster.c
> b/src/backend/postmaster/postmaster.c
> > index 1da5752047..e4b3d1bd59 100644
> > --- a/src/backend/postmaster/postmaster.c
> > +++ b/src/backend/postmaster/postmaster.c
> > @@ -4327,6 +4327,14 @@ BackendInitialize(Port *port)
> > port->remote_host = strdup(remote_host);
> > port->remote_port = strdup(remote_port);
> >
> > + if (port->remote_host == NULL || port->remote_port == NULL) {
> > + /* Since we are out of memory, we use strerror_r and
> write_stderr
> > + here. They do not allocate memory and just use the
> stack. */
> > + char sebuf[PG_STRERROR_R_BUFLEN];
> > + write_stderr("out of memory: %s", strerror_r(errno,
> sebuf, sizeof(sebuf)));
> > + proc_exit(1);
> > + }
> > +
>
> Something like this (with the comment in the right syntax) seems like an
> appropriate fix.
>
Yeah, I fixed the comments. Patch attached. I am actually sceptical of this
approach and think using pstrdup() and friends is better. With this
approach, there are two different mechanisms for allocating memory, so
there is an inconsistency in the code that is just confusing and raises
questions rather than solves problems. Better to have a consistent approach
and use pstrdup() everywhere.
>
> > However, an alternative patch is to not use strdup() or any other
> functions that allocate memory on the heap and instead use pstrdup(),
> pmalloc() and friends. Not sure if there are any reasons to not use those
> in this function, but it seems like the memory context is correctly set up
> when BackendInitialize() is called. I have attached a patch for that as
> well. In this case, it is not necessary to check the return value since
> MemoryContextAlloc() will report an error if it fails to allocate memory
> and not return NULL.
>
> I think using pstrdup would require changing over to PostmasterContext (or
> TopMemoryContext even?) to ensure these allocations are around for the
> lifetime
> of the backend.
>
PostmasterContext is set quite early in the procedure so it should not be a
problem to use it. I think we can use PostmasterContext rather than
TopMemoryContext. The memory for the allocations are passed down to
PostmasterMain() and used inside InitPostgres() but as far as I can tell,
they are not used after the InitPostgres() call. The PostmasterContex is
destroyed right after the call to InitPostgres() inside PostmasterMain()
and then the rest is allocated in other contexts.
>
> A related issue is that we in PostmasterMain strdup into
> output_config_variable
> during option parsing, but we don't check for success. When we then go
> about
> checking if -C was set, we can't tell if it wasn't at all set or if the
> strdup() call failed. Another one is the -D option where a failure to
> strdup
> will silently fall back to $PGDATA. Both of these should IMO check for
> strdup
> returning NULL and exit in case it does.
>
For the strdup() in PostmasterMain() we have this code quite early in the
function, so it should be safe to use pstrdup() consistently.
/*
* By default, palloc() requests in the postmaster will be allocated in
* the PostmasterContext, which is space that can be recycled by backends.
* Allocated data that needs to be available to backends should be
* allocated in TopMemoryContext.
*/
PostmasterContext = AllocSetContextCreate(TopMemoryContext,
"Postmaster",
ALLOCSET_DEFAULT_SIZES);
MemoryContextSwitchTo(PostmasterContext);
I have attached a patch for that as well, which is the approach I think is
better.
Best wishes,
Mats Kindahl, Timescale
>
> --
> Daniel Gustafsson https://vmware.com/
>
>
Attachments: download all (2 files, .tar.gz)
0001-Check-return-value-of-strdup-when-starting-backend.patch (text/x-patch, 2.8K) download
From 69d2cbd0a43d673b03cb80b680b4c919a27057c7 Mon Sep 17 00:00:00 2001
From: Mats Kindahl <mats@timescale.com>
Date: Thu, 8 Dec 2022 10:00:23 +0100
Subject: Check return value of strdup when starting backend
Inside `BackendInitialize`, if `strdup` returns NULL because it is out
of memory, this will later trigger a server crash because of
dereferencing a null pointer.
This commit fixes this by checking the return values of `strdup` and
exit the backend with exit status 1 if any of the allocations failed.
This will be handled as a normal shutdown in the postmaster, which will
report the exiting backend and continue executing.
---
src/backend/postmaster/postmaster.c | 24 +++++++++++++++++++-----
1 file changed, 19 insertions(+), 5 deletions(-)
diff --git a/src/backend/postmaster/postmaster.c b/src/backend/postmaster/postmaster.c
index f459dab360..e7739794ae 100644
--- a/src/backend/postmaster/postmaster.c
+++ b/src/backend/postmaster/postmaster.c
@@ -420,6 +420,7 @@ static pid_t StartChildProcess(AuxProcType type);
static void StartAutovacuumWorker(void);
static void MaybeStartWalReceiver(void);
static void InitPostmasterDeathWatchHandle(void);
+static char *safe_strdup(const char*);
/*
* Archiver is allowed to start up at the current postmaster state?
@@ -704,7 +705,7 @@ PostmasterMain(int argc, char *argv[])
break;
case 'C':
- output_config_variable = strdup(optarg);
+ output_config_variable = safe_strdup(optarg);
break;
case 'c':
@@ -735,7 +736,7 @@ PostmasterMain(int argc, char *argv[])
}
case 'D':
- userDoption = strdup(optarg);
+ userDoption = safe_strdup(optarg);
break;
case 'd':
@@ -4275,8 +4276,8 @@ BackendInitialize(Port *port)
* Save remote_host and remote_port in port structure (after this, they
* will appear in log_line_prefix data for log messages).
*/
- port->remote_host = strdup(remote_host);
- port->remote_port = strdup(remote_port);
+ port->remote_host = safe_strdup(remote_host);
+ port->remote_port = safe_strdup(remote_port);
/* And now we can issue the Log_connections message, if wanted */
if (Log_connections)
@@ -4307,7 +4308,10 @@ BackendInitialize(Port *port)
ret == 0 &&
strspn(remote_host, "0123456789.") < strlen(remote_host) &&
strspn(remote_host, "0123456789ABCDEFabcdef:") < strlen(remote_host))
- port->remote_hostname = strdup(remote_host);
+ {
+ port->remote_hostname = safe_strdup(remote_host);
+ }
+
/*
* Ready to begin client interaction. We will give up and _exit(1) after
@@ -6475,3 +6479,13 @@ InitPostmasterDeathWatchHandle(void)
GetLastError())));
#endif /* WIN32 */
}
+
+char *safe_strdup(const char *str) {
+ char *result = strdup(str);
+ if (result == NULL) {
+ ereport(ERROR,
+ (errcode(ERRCODE_OUT_OF_MEMORY),
+ errmsg("out of memory")));
+ }
+ return result;
+}
--
2.34.1
0001-Use-pstrdup-instead-of-strdup-in-postmaster.patch (text/x-patch, 2.1K) download
From 48814cea13df4591474cd96a90655bf9007ca69d Mon Sep 17 00:00:00 2001
From: Mats Kindahl <mats@timescale.com>
Date: Wed, 14 Dec 2022 13:51:40 +0100
Subject: Use pstrdup instead of strdup in postmaster
There is no check for the return value of strdup() in
BackendInitialize() which means that it is possible to get a
segmentation fault when starting a backend if the system is low on
memory. Instead, use pstrdup() for the allocations that are within the
same process since this cannot return NULL and instead fail with an
error, aborting the start of a backend.
---
src/backend/postmaster/postmaster.c | 10 +++++-----
1 file changed, 5 insertions(+), 5 deletions(-)
diff --git a/src/backend/postmaster/postmaster.c b/src/backend/postmaster/postmaster.c
index f459dab360..8ea09a62f6 100644
--- a/src/backend/postmaster/postmaster.c
+++ b/src/backend/postmaster/postmaster.c
@@ -704,7 +704,7 @@ PostmasterMain(int argc, char *argv[])
break;
case 'C':
- output_config_variable = strdup(optarg);
+ output_config_variable = pstrdup(optarg);
break;
case 'c':
@@ -735,7 +735,7 @@ PostmasterMain(int argc, char *argv[])
}
case 'D':
- userDoption = strdup(optarg);
+ userDoption = pstrdup(optarg);
break;
case 'd':
@@ -4275,8 +4275,8 @@ BackendInitialize(Port *port)
* Save remote_host and remote_port in port structure (after this, they
* will appear in log_line_prefix data for log messages).
*/
- port->remote_host = strdup(remote_host);
- port->remote_port = strdup(remote_port);
+ port->remote_host = pstrdup(remote_host);
+ port->remote_port = pstrdup(remote_port);
/* And now we can issue the Log_connections message, if wanted */
if (Log_connections)
@@ -4307,7 +4307,7 @@ BackendInitialize(Port *port)
ret == 0 &&
strspn(remote_host, "0123456789.") < strlen(remote_host) &&
strspn(remote_host, "0123456789ABCDEFabcdef:") < strlen(remote_host))
- port->remote_hostname = strdup(remote_host);
+ port->remote_hostname = pstrdup(remote_host);
/*
* Ready to begin client interaction. We will give up and _exit(1) after
--
2.34.1
* Re: Crash during backend start when low on memory
2022-12-14 12:58 Crash during backend start when low on memory Mats Kindahl <mats@timescale.com>
2022-12-14 13:23 ` Re: Crash during backend start when low on memory Daniel Gustafsson <daniel@yesql.se>
2022-12-14 21:14 ` Re: Crash during backend start when low on memory Mats Kindahl <mats@timescale.com>
@ 2022-12-15 17:20 ` Alvaro Herrera <alvherre@alvh.no-ip.org>
2022-12-16 08:46 ` Re: Crash during backend start when low on memory Mats Kindahl <mats@timescale.com>
0 siblings, 1 reply; 21+ messages in thread
From: Alvaro Herrera @ 2022-12-15 17:20 UTC (permalink / raw)
To: Mats Kindahl <mats@timescale.com>; +Cc: daniel@yesql.se; pgsql-bugs@lists.postgresql.org
On 2022-Dec-14, Mats Kindahl wrote:
> PostmasterContext is set quite early in the procedure so it should not be a
> problem to use it. I think we can use PostmasterContext rather than
> TopMemoryContext. The memory for the allocations are passed down to
> PostmasterMain() and used inside InitPostgres() but as far as I can tell,
> they are not used after the InitPostgres() call. The PostmasterContex is
> destroyed right after the call to InitPostgres() inside PostmasterMain()
> and then the rest is allocated in other contexts.
Well, this is what PostgresMain has to say about it:
/*
* If the PostmasterContext is still around, recycle the space; we don't
* need it anymore after InitPostgres completes. Note this does not trash
* *MyProcPort, because ConnCreate() allocated that space with malloc()
* ... else we'd need to copy the Port data first. Also, subsidiary data
* such as the username isn't lost either; see ProcessStartupPacket().
*/
if (PostmasterContext)
{
MemoryContextDelete(PostmasterContext);
So I think blowing away part of struct Port independently of the
containing struct is a bad idea. Why isn't more appropriate to do what
ConnCreate does?
if (!(port = (Port *) calloc(1, sizeof(Port))))
{
ereport(LOG,
(errcode(ERRCODE_OUT_OF_MEMORY),
errmsg("out of memory")));
ExitPostmaster(1);
}
... ugh. I have to admit that killing postmaster due to a transient
out-of-memory makes me a bit nervous -- do note that this
ExitPostmaster() happens before we've forked the backend, so this code
does bring the server down (!!). If we want to make postmaster more
robust in the face of OOM, we should also change this code somehow, yes?
I agree with Mats' premise when starting the thread -- even if the
server is not in great shape, it doesn't seem like taking the whole
service down is a great response. It would be better to just refuse the
connection with some error and let existing connections chug along,
poorly though they can. Maybe they'll also die soon, freeing memory;
but that'll be no good if Postmaster is gone altogether.
Now, it would be better if we could test this more thoroughly. Perhaps
it's possible to use something like the mallocfail.c lib that Heikki
wrote a few years ago, teaching it to handle calloc
https://postgr.es/m/547480DE.4040408@vmware.com
--
Álvaro Herrera Breisgau, Deutschland — https://www.EnterpriseDB.com/
"Oh, great altar of passive entertainment, bestow upon me thy discordant images
at such speed as to render linear thought impossible" (Calvin a la TV)
^ permalink raw reply [nested|flat] 21+ messages in thread
* Re: Crash during backend start when low on memory
2022-12-14 12:58 Crash during backend start when low on memory Mats Kindahl <mats@timescale.com>
2022-12-14 13:23 ` Re: Crash during backend start when low on memory Daniel Gustafsson <daniel@yesql.se>
2022-12-14 21:14 ` Re: Crash during backend start when low on memory Mats Kindahl <mats@timescale.com>
2022-12-15 17:20 ` Re: Crash during backend start when low on memory Alvaro Herrera <alvherre@alvh.no-ip.org>
@ 2022-12-16 08:46 ` Mats Kindahl <mats@timescale.com>
2023-01-13 10:05 ` Re: Crash during backend start when low on memory Mats Kindahl <mats@timescale.com>
0 siblings, 1 reply; 21+ messages in thread
From: Mats Kindahl @ 2022-12-16 08:46 UTC (permalink / raw)
To: Alvaro Herrera <alvherre@alvh.no-ip.org>; +Cc: daniel@yesql.se; pgsql-bugs@lists.postgresql.org
On Thu, Dec 15, 2022 at 6:20 PM Alvaro Herrera <alvherre@alvh.no-ip.org>
wrote:
> On 2022-Dec-14, Mats Kindahl wrote:
>
> > PostmasterContext is set quite early in the procedure so it should not
> be a
> > problem to use it. I think we can use PostmasterContext rather than
> > TopMemoryContext. The memory for the allocations are passed down to
> > PostmasterMain() and used inside InitPostgres() but as far as I can tell,
> > they are not used after the InitPostgres() call. The PostmasterContex is
> > destroyed right after the call to InitPostgres() inside PostmasterMain()
> > and then the rest is allocated in other contexts.
>
> Well, this is what PostgresMain has to say about it:
>
> /*
> * If the PostmasterContext is still around, recycle the space; we
> don't
> * need it anymore after InitPostgres completes. Note this does
> not trash
> * *MyProcPort, because ConnCreate() allocated that space with
> malloc()
> * ... else we'd need to copy the Port data first. Also,
> subsidiary data
> * such as the username isn't lost either; see
> ProcessStartupPacket().
> */
> if (PostmasterContext)
> {
> MemoryContextDelete(PostmasterContext);
>
>
Ugh. Ok. Missed that comment.
> So I think blowing away part of struct Port independently of the
> containing struct is a bad idea. Why isn't more appropriate to do what
> ConnCreate does?
>
> if (!(port = (Port *) calloc(1, sizeof(Port))))
> {
> ereport(LOG,
> (errcode(ERRCODE_OUT_OF_MEMORY),
> errmsg("out of memory")));
> ExitPostmaster(1);
> }
>
> ... ugh. I have to admit that killing postmaster due to a transient
> out-of-memory makes me a bit nervous -- do note that this
> ExitPostmaster() happens before we've forked the backend, so this code
> does bring the server down (!!). If we want to make postmaster more
> robust in the face of OOM, we should also change this code somehow, yes?
>
Seems reasonable, yes. I'll see what I can throw together.
>
> I agree with Mats' premise when starting the thread -- even if the
> server is not in great shape, it doesn't seem like taking the whole
> service down is a great response. It would be better to just refuse the
> connection with some error and let existing connections chug along,
> poorly though they can. Maybe they'll also die soon, freeing memory;
> but that'll be no good if Postmaster is gone altogether.
>
> Now, it would be better if we could test this more thoroughly. Perhaps
> it's possible to use something like the mallocfail.c lib that Heikki
> wrote a few years ago, teaching it to handle calloc
> https://postgr.es/m/547480DE.4040408@vmware.com
Thanks. Will take a look at that.
>
> --
> Álvaro Herrera Breisgau, Deutschland —
> https://www.EnterpriseDB.com/
> "Oh, great altar of passive entertainment, bestow upon me thy discordant
> images
> at such speed as to render linear thought impossible" (Calvin a la TV)
>
^ permalink raw reply [nested|flat] 21+ messages in thread
* Re: Crash during backend start when low on memory
2022-12-14 12:58 Crash during backend start when low on memory Mats Kindahl <mats@timescale.com>
2022-12-14 13:23 ` Re: Crash during backend start when low on memory Daniel Gustafsson <daniel@yesql.se>
2022-12-14 21:14 ` Re: Crash during backend start when low on memory Mats Kindahl <mats@timescale.com>
2022-12-15 17:20 ` Re: Crash during backend start when low on memory Alvaro Herrera <alvherre@alvh.no-ip.org>
2022-12-16 08:46 ` Re: Crash during backend start when low on memory Mats Kindahl <mats@timescale.com>
@ 2023-01-13 10:05 ` Mats Kindahl <mats@timescale.com>
2023-01-13 11:53 ` Re: Crash during backend start when low on memory Alvaro Herrera <alvherre@alvh.no-ip.org>
0 siblings, 1 reply; 21+ messages in thread
From: Mats Kindahl @ 2023-01-13 10:05 UTC (permalink / raw)
To: Alvaro Herrera <alvherre@alvh.no-ip.org>; +Cc: daniel@yesql.se; pgsql-bugs@lists.postgresql.org
On Fri, Dec 16, 2022 at 9:46 AM Mats Kindahl <mats@timescale.com> wrote:
> On Thu, Dec 15, 2022 at 6:20 PM Alvaro Herrera <alvherre@alvh.no-ip.org>
> wrote:
>
>> On 2022-Dec-14, Mats Kindahl wrote:
>>
>> > PostmasterContext is set quite early in the procedure so it should not
>> be a
>> > problem to use it. I think we can use PostmasterContext rather than
>> > TopMemoryContext. The memory for the allocations are passed down to
>> > PostmasterMain() and used inside InitPostgres() but as far as I can
>> tell,
>> > they are not used after the InitPostgres() call. The PostmasterContex is
>> > destroyed right after the call to InitPostgres() inside PostmasterMain()
>> > and then the rest is allocated in other contexts.
>>
>> Well, this is what PostgresMain has to say about it:
>>
>> /*
>> * If the PostmasterContext is still around, recycle the space;
>> we don't
>> * need it anymore after InitPostgres completes. Note this does
>> not trash
>> * *MyProcPort, because ConnCreate() allocated that space with
>> malloc()
>> * ... else we'd need to copy the Port data first. Also,
>> subsidiary data
>> * such as the username isn't lost either; see
>> ProcessStartupPacket().
>> */
>> if (PostmasterContext)
>> {
>> MemoryContextDelete(PostmasterContext);
>>
>>
> Ugh. Ok. Missed that comment.
>
>
>> So I think blowing away part of struct Port independently of the
>> containing struct is a bad idea. Why isn't more appropriate to do what
>> ConnCreate does?
>>
>> if (!(port = (Port *) calloc(1, sizeof(Port))))
>> {
>> ereport(LOG,
>> (errcode(ERRCODE_OUT_OF_MEMORY),
>> errmsg("out of memory")));
>> ExitPostmaster(1);
>> }
>>
>> ... ugh. I have to admit that killing postmaster due to a transient
>> out-of-memory makes me a bit nervous -- do note that this
>> ExitPostmaster() happens before we've forked the backend, so this code
>> does bring the server down (!!). If we want to make postmaster more
>> robust in the face of OOM, we should also change this code somehow, yes?
>>
>
> Seems reasonable, yes. I'll see what I can throw together.
>
>
>>
>> I agree with Mats' premise when starting the thread -- even if the
>> server is not in great shape, it doesn't seem like taking the whole
>> service down is a great response. It would be better to just refuse the
>> connection with some error and let existing connections chug along,
>> poorly though they can. Maybe they'll also die soon, freeing memory;
>> but that'll be no good if Postmaster is gone altogether.
>>
>
>> Now, it would be better if we could test this more thoroughly. Perhaps
>> it's possible to use something like the mallocfail.c lib that Heikki
>> wrote a few years ago, teaching it to handle calloc
>> https://postgr.es/m/547480DE.4040408@vmware.com
>
>
> Thanks. Will take a look at that.
>
I have an improved patch based on raising an error when malloc fails. I
used the version of mallocfail that is available at
https://github.com/ralight/mallocfail which allows you to run the program
with incrementally failing memory allocations but since the count is around
6886 or so when the postmaster starts to allow backend starts and other
processes are starting in between, this count is not always accurate and
something "smarter" is needed to get a reliable failure test.
It is quite straightforward to test it by attaching a debugger and setting
the return value of malloc, but to be able to have some "smarter" failures
only when, for example, starting a backend it would be necessary to have
something better integrated with the postgres code. Being able to attach a
uprobe using bpftrace and override the return value would be the most
elegant solution, but that is currently not possible, so considering
whether to add something like the mallocfail above to the regular testing.
Note: there are a bunch of other core dumps that occur during startup
before the postmaster is ready to accept connections, but I think that is
less of a problem since it fails during startup before the system is ready
to accept connections.
Best wishes,
Mats Kindahl, Timescale
>
>>
>> --
>> Álvaro Herrera Breisgau, Deutschland —
>> https://www.EnterpriseDB.com/
>> "Oh, great altar of passive entertainment, bestow upon me thy discordant
>> images
>> at such speed as to render linear thought impossible" (Calvin a la TV)
>>
>
Attachments:
0001-Check-return-value-of-strdup-when-starting-backend.patch (application/x-patch, 6.3K) download
From 9940d2a5a5192fda2633a32055a258ea6a4465cb Mon Sep 17 00:00:00 2001
From: Mats Kindahl <mats@timescale.com>
Date: Thu, 8 Dec 2022 10:00:23 +0100
Subject: Check return value of strdup when starting backend
Inside `BackendInitialize`, if `strdup` returns NULL because it is out
of memory, this will later trigger a server crash because of
dereferencing a null pointer.
This commit fixes this by checking the return values of memory
allocation functions and abort with an error if the memory allocation
fails.
Since the postmaster should not exit when if a memory allocation
failure occurs when trying to start a backend, it will capture any
errors occuring when starting a backend and just continue running.
Fatal errors when starting the backend will still abort the postmaster.
---
src/backend/postmaster/postmaster.c | 89 ++++++++++++++++-------------
1 file changed, 50 insertions(+), 39 deletions(-)
diff --git a/src/backend/postmaster/postmaster.c b/src/backend/postmaster/postmaster.c
index f459dab360..baae6fa0d5 100644
--- a/src/backend/postmaster/postmaster.c
+++ b/src/backend/postmaster/postmaster.c
@@ -148,6 +148,19 @@
#define BACKEND_TYPE_BGWORKER 0x0008 /* bgworker process */
#define BACKEND_TYPE_ALL 0x000F /* OR of all the above */
+/*
+ * Convenience macro to call a function that can allocate memory, check the
+ * return value, and abort with an out of memory error if it is NULL.
+ */
+#define CHECKED_CALL(STMT) \
+ do { \
+ if ((STMT) == NULL) \
+ ereport(ERROR, \
+ (errcode(ERRCODE_OUT_OF_MEMORY), \
+ errmsg("out of memory"), \
+ errdetail("Statement is " #STMT))); \
+ } while (0)
+
/*
* List of active backends (or child processes anyway; we don't actually
* know whether a given child has become a backend or is still in the
@@ -615,8 +628,8 @@ PostmasterMain(int argc, char *argv[])
*
* 1. We tell sigaction() to block all signals for the duration of the
* signal handler. This is faster than our old approach of
- * blocking/unblocking explicitly in the signal handler, and it should also
- * prevent excessive stack consumption if signals arrive quickly.
+ * blocking/unblocking explicitly in the signal handler, and it should
+ * also prevent excessive stack consumption if signals arrive quickly.
*
* 2. We do not set the SA_RESTART flag. This is because signals will be
* blocked at all times except when ServerLoop is waiting for something to
@@ -704,7 +717,7 @@ PostmasterMain(int argc, char *argv[])
break;
case 'C':
- output_config_variable = strdup(optarg);
+ CHECKED_CALL(output_config_variable = strdup(optarg));
break;
case 'c':
@@ -735,7 +748,7 @@ PostmasterMain(int argc, char *argv[])
}
case 'D':
- userDoption = strdup(optarg);
+ CHECKED_CALL(userDoption = strdup(optarg));
break;
case 'd':
@@ -1783,20 +1796,36 @@ ServerLoop(void)
break;
if (FD_ISSET(ListenSocket[i], &rmask))
{
- Port *port;
+ /*
+ * If any errors occur when trying to spawn the backend,
+ * we will not abort the postmaster and rather report the
+ * error and continue running. Note that fatal errors will
+ * not be caught, so do not throw them unless you want to
+ * shut down the postmaster.
+ */
+
+ Port *port = NULL;
- port = ConnCreate(ListenSocket[i]);
- if (port)
+ PG_TRY();
{
+ port = ConnCreate(ListenSocket[i]);
BackendStartup(port);
-
+ }
+ PG_FINALLY();
+ {
/*
* We no longer need the open socket or port structure
- * in this process
+ * in this process, and definitely not if we failed
+ * spawning a backend.
*/
- StreamClose(port->sock);
- ConnFree(port);
+ if (port)
+ {
+ if (port->sock != PGINVALID_SOCKET)
+ StreamClose(port->sock);
+ ConnFree(port);
+ }
}
+ PG_END_TRY();
}
}
}
@@ -2561,21 +2590,11 @@ ConnCreate(int serverFd)
{
Port *port;
- if (!(port = (Port *) calloc(1, sizeof(Port))))
- {
- ereport(LOG,
- (errcode(ERRCODE_OUT_OF_MEMORY),
- errmsg("out of memory")));
- ExitPostmaster(1);
- }
+ CHECKED_CALL(port = calloc(1, sizeof(Port)));
+ /* Caller will deal with freeing anything that needs to be free'd */
if (StreamConnection(serverFd, port) != STATUS_OK)
- {
- if (port->sock != PGINVALID_SOCKET)
- StreamClose(port->sock);
- ConnFree(port);
return NULL;
- }
return port;
}
@@ -4054,14 +4073,7 @@ BackendStartup(Port *port)
* Create backend data structure. Better before the fork() so we can
* handle failure cleanly.
*/
- bn = (Backend *) malloc(sizeof(Backend));
- if (!bn)
- {
- ereport(LOG,
- (errcode(ERRCODE_OUT_OF_MEMORY),
- errmsg("out of memory")));
- return STATUS_ERROR;
- }
+ CHECKED_CALL(bn = malloc(sizeof(Backend)));
/*
* Compute the cancel key that will be assigned to this backend. The
@@ -4275,8 +4287,8 @@ BackendInitialize(Port *port)
* Save remote_host and remote_port in port structure (after this, they
* will appear in log_line_prefix data for log messages).
*/
- port->remote_host = strdup(remote_host);
- port->remote_port = strdup(remote_port);
+ CHECKED_CALL(port->remote_host = strdup(remote_host));
+ CHECKED_CALL(port->remote_port = strdup(remote_port));
/* And now we can issue the Log_connections message, if wanted */
if (Log_connections)
@@ -4307,7 +4319,10 @@ BackendInitialize(Port *port)
ret == 0 &&
strspn(remote_host, "0123456789.") < strlen(remote_host) &&
strspn(remote_host, "0123456789ABCDEFabcdef:") < strlen(remote_host))
- port->remote_hostname = strdup(remote_host);
+ {
+ CHECKED_CALL(port->remote_hostname = strdup(remote_host));
+ }
+
/*
* Ready to begin client interaction. We will give up and _exit(1) after
@@ -4727,11 +4742,7 @@ retry:
* Struct will be free():d from the callback function that runs on a
* different thread.
*/
- childinfo = malloc(sizeof(win32_deadchild_waitinfo));
- if (!childinfo)
- ereport(FATAL,
- (errcode(ERRCODE_OUT_OF_MEMORY),
- errmsg("out of memory")));
+ CHECKED_CALL(childinfo = malloc(sizeof(win32_deadchild_waitinfo)));
childinfo->procHandle = pi.hProcess;
childinfo->procId = pi.dwProcessId;
--
2.34.1
* Re: Crash during backend start when low on memory
2022-12-14 12:58 Crash during backend start when low on memory Mats Kindahl <mats@timescale.com>
2022-12-14 13:23 ` Re: Crash during backend start when low on memory Daniel Gustafsson <daniel@yesql.se>
2022-12-14 21:14 ` Re: Crash during backend start when low on memory Mats Kindahl <mats@timescale.com>
2022-12-15 17:20 ` Re: Crash during backend start when low on memory Alvaro Herrera <alvherre@alvh.no-ip.org>
2022-12-16 08:46 ` Re: Crash during backend start when low on memory Mats Kindahl <mats@timescale.com>
2023-01-13 10:05 ` Re: Crash during backend start when low on memory Mats Kindahl <mats@timescale.com>
@ 2023-01-13 11:53 ` Alvaro Herrera <alvherre@alvh.no-ip.org>
2023-01-13 15:01 ` Re: Crash during backend start when low on memory Tom Lane <tgl@sss.pgh.pa.us>
2023-01-16 10:09 ` Re: Crash during backend start when low on memory Mats Kindahl <mats@timescale.com>
2023-02-05 21:00 ` Re: Crash during backend start when low on memory Mats Kindahl <mats@timescale.com>
0 siblings, 3 replies; 21+ messages in thread
From: Alvaro Herrera @ 2023-01-13 11:53 UTC (permalink / raw)
To: Mats Kindahl <mats@timescale.com>; +Cc: daniel@yesql.se; pgsql-bugs@lists.postgresql.org
On 2023-Jan-13, Mats Kindahl wrote:
> I have an improved patch based on raising an error when malloc fails. I
> used the version of mallocfail that is available at
> https://github.com/ralight/mallocfail which allows you to run the program
> with incrementally failing memory allocations but since the count is around
> 6886 or so when the postmaster starts to allow backend starts and other
> processes are starting in between, this count is not always accurate and
> something "smarter" is needed to get a reliable failure test.
Hmm, but I'm not sure I like what you did to BackendStartup:
@@ -4054,14 +4073,7 @@ BackendStartup(Port *port)
* Create backend data structure. Better before the fork() so we can
* handle failure cleanly.
*/
- bn = (Backend *) malloc(sizeof(Backend));
- if (!bn)
- {
- ereport(LOG,
- (errcode(ERRCODE_OUT_OF_MEMORY),
- errmsg("out of memory")));
- return STATUS_ERROR;
- }
+ CHECKED_CALL(bn = malloc(sizeof(Backend)));
With this code, now we will have postmaster raise ERROR if this malloc
fails, rather than returning STATUS_ERROR as previously. How does
postmaster react to this? I think it won't go very well.
(Stylistically, I'd rather have the test and corresponding reaction at
each call site rather than using a macro to hide it. I especially don't
like the fact that the errdetail you propose would report a line of C
code to the user, rather than an intelligible explanation of what that
code is trying to do.)
> It is quite straightforward to test it by attaching a debugger and setting
> the return value of malloc, but to be able to have some "smarter" failures
> only when, for example, starting a backend it would be necessary to have
> something better integrated with the postgres code. Being able to attach a
> uprobe using bpftrace and override the return value would be the most
> elegant solution, but that is currently not possible, so considering
> whether to add something like the mallocfail above to the regular testing.
Hmm, I'm not sure that this type of testing is something to do on an
ongoing basis.
> Note: there are a bunch of other core dumps that occur during startup
> before the postmaster is ready to accept connections, but I think that is
> less of a problem since it fails during startup before the system is ready
> to accept connections.
That sounds reasonable. The service watcher should restart postmaster
appropriately in that case (be it old SysV init, systemd, your container
management layer, or whatever.)
--
Álvaro Herrera PostgreSQL Developer — https://www.EnterpriseDB.com/
"Ellos andaban todos desnudos como su madre los parió, y también las mujeres,
aunque no vi más que una, harto moza, y todos los que yo vi eran todos
mancebos, que ninguno vi de edad de más de XXX años" (Cristóbal Colón)
^ permalink raw reply [nested|flat] 21+ messages in thread
* Re: Crash during backend start when low on memory
2022-12-14 12:58 Crash during backend start when low on memory Mats Kindahl <mats@timescale.com>
2022-12-14 13:23 ` Re: Crash during backend start when low on memory Daniel Gustafsson <daniel@yesql.se>
2022-12-14 21:14 ` Re: Crash during backend start when low on memory Mats Kindahl <mats@timescale.com>
2022-12-15 17:20 ` Re: Crash during backend start when low on memory Alvaro Herrera <alvherre@alvh.no-ip.org>
2022-12-16 08:46 ` Re: Crash during backend start when low on memory Mats Kindahl <mats@timescale.com>
2023-01-13 10:05 ` Re: Crash during backend start when low on memory Mats Kindahl <mats@timescale.com>
2023-01-13 11:53 ` Re: Crash during backend start when low on memory Alvaro Herrera <alvherre@alvh.no-ip.org>
@ 2023-01-13 15:01 ` Tom Lane <tgl@sss.pgh.pa.us>
2023-01-16 20:52 ` Re: Crash during backend start when low on memory Mats Kindahl <mats@timescale.com>
2 siblings, 1 reply; 21+ messages in thread
From: Tom Lane @ 2023-01-13 15:01 UTC (permalink / raw)
To: Alvaro Herrera <alvherre@alvh.no-ip.org>; +Cc: Mats Kindahl <mats@timescale.com>; daniel@yesql.se; pgsql-bugs@lists.postgresql.org
Alvaro Herrera <alvherre@alvh.no-ip.org> writes:
> On 2023-Jan-13, Mats Kindahl wrote:
>> I have an improved patch based on raising an error when malloc fails.
> Hmm, but I'm not sure I like what you did to BackendStartup:
Yeah, the BackendStartup change is 100% wrong; it is replacing
perfectly good code that recovers correctly with bad code that
will take down the postmaster (not a backend child!) on OOM.
By and large I don't like anything about this patch. You could
get the same results (i.e. elog(ERROR) on OOM) by replacing the
malloc calls with pallocs.
regards, tom lane
^ permalink raw reply [nested|flat] 21+ messages in thread
* Re: Crash during backend start when low on memory
2022-12-14 12:58 Crash during backend start when low on memory Mats Kindahl <mats@timescale.com>
2022-12-14 13:23 ` Re: Crash during backend start when low on memory Daniel Gustafsson <daniel@yesql.se>
2022-12-14 21:14 ` Re: Crash during backend start when low on memory Mats Kindahl <mats@timescale.com>
2022-12-15 17:20 ` Re: Crash during backend start when low on memory Alvaro Herrera <alvherre@alvh.no-ip.org>
2022-12-16 08:46 ` Re: Crash during backend start when low on memory Mats Kindahl <mats@timescale.com>
2023-01-13 10:05 ` Re: Crash during backend start when low on memory Mats Kindahl <mats@timescale.com>
2023-01-13 11:53 ` Re: Crash during backend start when low on memory Alvaro Herrera <alvherre@alvh.no-ip.org>
2023-01-13 15:01 ` Re: Crash during backend start when low on memory Tom Lane <tgl@sss.pgh.pa.us>
@ 2023-01-16 20:52 ` Mats Kindahl <mats@timescale.com>
2023-01-16 20:58 ` Re: Crash during backend start when low on memory Tom Lane <tgl@sss.pgh.pa.us>
0 siblings, 1 reply; 21+ messages in thread
From: Mats Kindahl @ 2023-01-16 20:52 UTC (permalink / raw)
To: Tom Lane <tgl@sss.pgh.pa.us>; +Cc: Alvaro Herrera <alvherre@alvh.no-ip.org>; daniel@yesql.se; pgsql-bugs@lists.postgresql.org
On Fri, Jan 13, 2023 at 4:01 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Alvaro Herrera <alvherre@alvh.no-ip.org> writes:
> > On 2023-Jan-13, Mats Kindahl wrote:
> >> I have an improved patch based on raising an error when malloc fails.
>
> > Hmm, but I'm not sure I like what you did to BackendStartup:
>
> Yeah, the BackendStartup change is 100% wrong; it is replacing
> perfectly good code that recovers correctly with bad code that
> will take down the postmaster (not a backend child!) on OOM.
>
AFAICT, the error is caught by the caller (using PG_TRY), executes some
cleanup code, and then continues executing, so it shouldn't take down the
postmaster.
>
> By and large I don't like anything about this patch. You could
> get the same results (i.e. elog(ERROR) on OOM) by replacing the
> malloc calls with pallocs.
>
Well... as Alvaro pointed out, just replacing it with palloc will blow out
part of the structure being allocated for the port information when the
backend starts running.
However, continuing on an approach using memory contexts instead of
directly allocating memory, something like this could be done:
1. There is already a memory context PostmasterContext for the
postmaster startup code, so only allocate stuff here that is not needed
after a backend has started.
2. Add a new memory context for "backend startup" (say,
BackendStartupContext) and use that for allocating stuff that should exist
after the backend starts. For example, the port information. (It might be
possible to move more information here, such as the HBA structures, which
would make it clearer that this is startup memory for the backend. Not sure
if there are some potential issues with this.)
3. Create the BackendStartupContext when starting the backend.
4. Replace previous malloc and friends with palloc in the
PostmasterContext (where suitable).
5. Replace allocation of port structure using malloc with allocation in
BackendStartupContext
6. When the backend starts, it would toss the memory context from the
postmaster, but that would leave the port structure intact. The backend
already tosses the PostmasterContext, but not until after it has done
authentication.
7. When the postmaster has spawned the backend, it should toss the
BackendStartupContext since it is not relevant any more.
8. Since creating the BackendStartupContext can fail with an error, we
need to wrap this in a PG_TRY block. Otherwise the postmaster will exit on
error.
This provides a few advantages compared to allocating memory using malloc
and friends:
1. We allocate memory for the postmaster context in one place, when the
actual startup is running. If we're low on memory at this point, we can
abort early.
2. We allocate memory for the backend startup in one place, when
starting the backend. If we're low on memory at this point, we can abort
early.
3. Code is significantly easier to read and will not require a lot of
checks during startup.
I have a tentative patch, but it contains some problems so I will solve
these first.
Best wishes,
Mats Kindahl
> regards, tom lane
>
^ permalink raw reply [nested|flat] 21+ messages in thread
* Re: Crash during backend start when low on memory
2022-12-14 12:58 Crash during backend start when low on memory Mats Kindahl <mats@timescale.com>
2022-12-14 13:23 ` Re: Crash during backend start when low on memory Daniel Gustafsson <daniel@yesql.se>
2022-12-14 21:14 ` Re: Crash during backend start when low on memory Mats Kindahl <mats@timescale.com>
2022-12-15 17:20 ` Re: Crash during backend start when low on memory Alvaro Herrera <alvherre@alvh.no-ip.org>
2022-12-16 08:46 ` Re: Crash during backend start when low on memory Mats Kindahl <mats@timescale.com>
2023-01-13 10:05 ` Re: Crash during backend start when low on memory Mats Kindahl <mats@timescale.com>
2023-01-13 11:53 ` Re: Crash during backend start when low on memory Alvaro Herrera <alvherre@alvh.no-ip.org>
2023-01-13 15:01 ` Re: Crash during backend start when low on memory Tom Lane <tgl@sss.pgh.pa.us>
2023-01-16 20:52 ` Re: Crash during backend start when low on memory Mats Kindahl <mats@timescale.com>
@ 2023-01-16 20:58 ` Tom Lane <tgl@sss.pgh.pa.us>
2023-01-16 21:10 ` Re: Crash during backend start when low on memory Mats Kindahl <mats@timescale.com>
0 siblings, 1 reply; 21+ messages in thread
From: Tom Lane @ 2023-01-16 20:58 UTC (permalink / raw)
To: Mats Kindahl <mats@timescale.com>; +Cc: Alvaro Herrera <alvherre@alvh.no-ip.org>; daniel@yesql.se; pgsql-bugs@lists.postgresql.org
Mats Kindahl <mats@timescale.com> writes:
> On Fri, Jan 13, 2023 at 4:01 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> Yeah, the BackendStartup change is 100% wrong; it is replacing
>> perfectly good code that recovers correctly with bad code that
>> will take down the postmaster (not a backend child!) on OOM.
> AFAICT, the error is caught by the caller (using PG_TRY), executes some
> cleanup code, and then continues executing, so it shouldn't take down the
> postmaster.
There are no PG_TRY blocks in the postmaster, and certainly no recovery.
regards, tom lane
^ permalink raw reply [nested|flat] 21+ messages in thread
* Re: Crash during backend start when low on memory
2022-12-14 12:58 Crash during backend start when low on memory Mats Kindahl <mats@timescale.com>
2022-12-14 13:23 ` Re: Crash during backend start when low on memory Daniel Gustafsson <daniel@yesql.se>
2022-12-14 21:14 ` Re: Crash during backend start when low on memory Mats Kindahl <mats@timescale.com>
2022-12-15 17:20 ` Re: Crash during backend start when low on memory Alvaro Herrera <alvherre@alvh.no-ip.org>
2022-12-16 08:46 ` Re: Crash during backend start when low on memory Mats Kindahl <mats@timescale.com>
2023-01-13 10:05 ` Re: Crash during backend start when low on memory Mats Kindahl <mats@timescale.com>
2023-01-13 11:53 ` Re: Crash during backend start when low on memory Alvaro Herrera <alvherre@alvh.no-ip.org>
2023-01-13 15:01 ` Re: Crash during backend start when low on memory Tom Lane <tgl@sss.pgh.pa.us>
2023-01-16 20:52 ` Re: Crash during backend start when low on memory Mats Kindahl <mats@timescale.com>
2023-01-16 20:58 ` Re: Crash during backend start when low on memory Tom Lane <tgl@sss.pgh.pa.us>
@ 2023-01-16 21:10 ` Mats Kindahl <mats@timescale.com>
2023-01-16 21:49 ` Re: Crash during backend start when low on memory Tom Lane <tgl@sss.pgh.pa.us>
0 siblings, 1 reply; 21+ messages in thread
From: Mats Kindahl @ 2023-01-16 21:10 UTC (permalink / raw)
To: Tom Lane <tgl@sss.pgh.pa.us>; +Cc: Alvaro Herrera <alvherre@alvh.no-ip.org>; daniel@yesql.se; pgsql-bugs@lists.postgresql.org
On Mon, Jan 16, 2023 at 9:58 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Mats Kindahl <mats@timescale.com> writes:
> > On Fri, Jan 13, 2023 at 4:01 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> >> Yeah, the BackendStartup change is 100% wrong; it is replacing
> >> perfectly good code that recovers correctly with bad code that
> >> will take down the postmaster (not a backend child!) on OOM.
>
> > AFAICT, the error is caught by the caller (using PG_TRY), executes some
> > cleanup code, and then continues executing, so it shouldn't take down the
> > postmaster.
>
> There are no PG_TRY blocks in the postmaster, and certainly no recovery.
>
I added one in the patch. Doesn't this work? It seemed to work when I tried
it.
Best wishes,
Mats Kindahl
> regards, tom lane
>
^ permalink raw reply [nested|flat] 21+ messages in thread
* Re: Crash during backend start when low on memory
2022-12-14 12:58 Crash during backend start when low on memory Mats Kindahl <mats@timescale.com>
2022-12-14 13:23 ` Re: Crash during backend start when low on memory Daniel Gustafsson <daniel@yesql.se>
2022-12-14 21:14 ` Re: Crash during backend start when low on memory Mats Kindahl <mats@timescale.com>
2022-12-15 17:20 ` Re: Crash during backend start when low on memory Alvaro Herrera <alvherre@alvh.no-ip.org>
2022-12-16 08:46 ` Re: Crash during backend start when low on memory Mats Kindahl <mats@timescale.com>
2023-01-13 10:05 ` Re: Crash during backend start when low on memory Mats Kindahl <mats@timescale.com>
2023-01-13 11:53 ` Re: Crash during backend start when low on memory Alvaro Herrera <alvherre@alvh.no-ip.org>
2023-01-13 15:01 ` Re: Crash during backend start when low on memory Tom Lane <tgl@sss.pgh.pa.us>
2023-01-16 20:52 ` Re: Crash during backend start when low on memory Mats Kindahl <mats@timescale.com>
2023-01-16 20:58 ` Re: Crash during backend start when low on memory Tom Lane <tgl@sss.pgh.pa.us>
2023-01-16 21:10 ` Re: Crash during backend start when low on memory Mats Kindahl <mats@timescale.com>
@ 2023-01-16 21:49 ` Tom Lane <tgl@sss.pgh.pa.us>
0 siblings, 0 replies; 21+ messages in thread
From: Tom Lane @ 2023-01-16 21:49 UTC (permalink / raw)
To: Mats Kindahl <mats@timescale.com>; +Cc: Alvaro Herrera <alvherre@alvh.no-ip.org>; daniel@yesql.se; pgsql-bugs@lists.postgresql.org
Mats Kindahl <mats@timescale.com> writes:
> On Mon, Jan 16, 2023 at 9:58 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> There are no PG_TRY blocks in the postmaster, and certainly no recovery.
> I added one in the patch.
That's VERY unlikely to get accepted.
regards, tom lane
^ permalink raw reply [nested|flat] 21+ messages in thread
* Re: Crash during backend start when low on memory
2022-12-14 12:58 Crash during backend start when low on memory Mats Kindahl <mats@timescale.com>
2022-12-14 13:23 ` Re: Crash during backend start when low on memory Daniel Gustafsson <daniel@yesql.se>
2022-12-14 21:14 ` Re: Crash during backend start when low on memory Mats Kindahl <mats@timescale.com>
2022-12-15 17:20 ` Re: Crash during backend start when low on memory Alvaro Herrera <alvherre@alvh.no-ip.org>
2022-12-16 08:46 ` Re: Crash during backend start when low on memory Mats Kindahl <mats@timescale.com>
2023-01-13 10:05 ` Re: Crash during backend start when low on memory Mats Kindahl <mats@timescale.com>
2023-01-13 11:53 ` Re: Crash during backend start when low on memory Alvaro Herrera <alvherre@alvh.no-ip.org>
@ 2023-01-16 10:09 ` Mats Kindahl <mats@timescale.com>
2 siblings, 0 replies; 21+ messages in thread
From: Mats Kindahl @ 2023-01-16 10:09 UTC (permalink / raw)
To: Alvaro Herrera <alvherre@alvh.no-ip.org>; +Cc: daniel@yesql.se; pgsql-bugs@lists.postgresql.org
On Fri, Jan 13, 2023 at 12:58 PM Alvaro Herrera <alvherre@alvh.no-ip.org>
wrote:
> On 2023-Jan-13, Mats Kindahl wrote:
>
> > I have an improved patch based on raising an error when malloc fails. I
> > used the version of mallocfail that is available at
> > https://github.com/ralight/mallocfail which allows you to run the
> program
> > with incrementally failing memory allocations but since the count is
> around
> > 6886 or so when the postmaster starts to allow backend starts and other
> > processes are starting in between, this count is not always accurate and
> > something "smarter" is needed to get a reliable failure test.
>
> Hmm, but I'm not sure I like what you did to BackendStartup:
>
> @@ -4054,14 +4073,7 @@ BackendStartup(Port *port)
> * Create backend data structure. Better before the fork() so we
> can
> * handle failure cleanly.
> */
> - bn = (Backend *) malloc(sizeof(Backend));
> - if (!bn)
> - {
> - ereport(LOG,
> - (errcode(ERRCODE_OUT_OF_MEMORY),
> - errmsg("out of memory")));
> - return STATUS_ERROR;
> - }
> + CHECKED_CALL(bn = malloc(sizeof(Backend)));
>
> With this code, now we will have postmaster raise ERROR if this malloc
> fails, rather than returning STATUS_ERROR as previously. How does
> postmaster react to this? I think it won't go very well.
>
Well.. this error is caught in the caller using this code:
PG_TRY();
{
port = ConnCreate(ListenSocket[i]);
BackendStartup(port);
}
PG_FINALLY();
{
/*
* We no longer need the open socket or port structure
* in this process, and definitely not if we failed
* spawning a backend.
*/
if (port)
{
if (port->sock != PGINVALID_SOCKET)
StreamClose(port->sock);
ConnFree(port);
}
}
PG_END_TRY();
Unless I am mistaken, this should mean that the error message is printed,
the streams closed and memory free'ed, and PostmasterMain continues as
before. It could just be logged using LOG level, but an ERROR stands out
better and is searchable so it is easy to surface in systems monitoring the
server. (Digging for specific log messages is cumbersome, just printing out
all ERROR messages in the log is easy.)
> (Stylistically, I'd rather have the test and corresponding reaction at
> each call site rather than using a macro to hide it. I especially don't
> like the fact that the errdetail you propose would report a line of C
> code to the user, rather than an intelligible explanation of what that
> code is trying to do.)
>
I agree this is ugly, but since this was rare and the error message is
mostly to understand what went wrong to be able to debug it, I thought this
would be sufficient for that. It is also easier to maintain. Trivial to
replace.
> > It is quite straightforward to test it by attaching a debugger and
> setting
> > the return value of malloc, but to be able to have some "smarter"
> failures
> > only when, for example, starting a backend it would be necessary to have
> > something better integrated with the postgres code. Being able to attach
> a
> > uprobe using bpftrace and override the return value would be the most
> > elegant solution, but that is currently not possible, so considering
> > whether to add something like the mallocfail above to the regular
> testing.
>
> Hmm, I'm not sure that this type of testing is something to do on an
> ongoing basis.
>
Me neither, which is why I didn't spend time on it right now. The argument
for doing it is that it would be good to have some test that can at least
be easily used to check that it behaves correctly on startup even if memory
failures occur.
>
> > Note: there are a bunch of other core dumps that occur during startup
> > before the postmaster is ready to accept connections, but I think that is
> > less of a problem since it fails during startup before the system is
> ready
> > to accept connections.
>
> That sounds reasonable. The service watcher should restart postmaster
> appropriately in that case (be it old SysV init, systemd, your container
> management layer, or whatever.)
>
Yes, that was my thinking as well. Core dumps are ugly, but provide useful
information to figure out where things went wrong.
>
> --
> Álvaro Herrera PostgreSQL Developer —
> https://www.EnterpriseDB.com/
> "Ellos andaban todos desnudos como su madre los parió, y también las
> mujeres,
> aunque no vi más que una, harto moza, y todos los que yo vi eran todos
> mancebos, que ninguno vi de edad de más de XXX años" (Cristóbal Colón)
>
^ permalink raw reply [nested|flat] 21+ messages in thread
* Re: Crash during backend start when low on memory
2022-12-14 12:58 Crash during backend start when low on memory Mats Kindahl <mats@timescale.com>
2022-12-14 13:23 ` Re: Crash during backend start when low on memory Daniel Gustafsson <daniel@yesql.se>
2022-12-14 21:14 ` Re: Crash during backend start when low on memory Mats Kindahl <mats@timescale.com>
2022-12-15 17:20 ` Re: Crash during backend start when low on memory Alvaro Herrera <alvherre@alvh.no-ip.org>
2022-12-16 08:46 ` Re: Crash during backend start when low on memory Mats Kindahl <mats@timescale.com>
2023-01-13 10:05 ` Re: Crash during backend start when low on memory Mats Kindahl <mats@timescale.com>
2023-01-13 11:53 ` Re: Crash during backend start when low on memory Alvaro Herrera <alvherre@alvh.no-ip.org>
@ 2023-02-05 21:00 ` Mats Kindahl <mats@timescale.com>
2023-02-05 23:18 ` Re: Crash during backend start when low on memory Andres Freund <andres@anarazel.de>
2 siblings, 1 reply; 21+ messages in thread
From: Mats Kindahl @ 2023-02-05 21:00 UTC (permalink / raw)
To: Alvaro Herrera <alvherre@alvh.no-ip.org>; +Cc: daniel@yesql.se; pgsql-bugs@lists.postgresql.org
On Fri, Jan 13, 2023 at 12:58 PM Alvaro Herrera <alvherre@alvh.no-ip.org>
wrote:
> On 2023-Jan-13, Mats Kindahl wrote:
>
> > I have an improved patch based on raising an error when malloc fails. I
> > used the version of mallocfail that is available at
> > https://github.com/ralight/mallocfail which allows you to run the
> program
> > with incrementally failing memory allocations but since the count is
> around
> > 6886 or so when the postmaster starts to allow backend starts and other
> > processes are starting in between, this count is not always accurate and
> > something "smarter" is needed to get a reliable failure test.
>
> Hmm, but I'm not sure I like what you did to BackendStartup:
>
> @@ -4054,14 +4073,7 @@ BackendStartup(Port *port)
> * Create backend data structure. Better before the fork() so we
> can
> * handle failure cleanly.
> */
> - bn = (Backend *) malloc(sizeof(Backend));
> - if (!bn)
> - {
> - ereport(LOG,
> - (errcode(ERRCODE_OUT_OF_MEMORY),
> - errmsg("out of memory")));
> - return STATUS_ERROR;
> - }
> + CHECKED_CALL(bn = malloc(sizeof(Backend)));
>
> With this code, now we will have postmaster raise ERROR if this malloc
> fails, rather than returning STATUS_ERROR as previously. How does
> postmaster react to this? I think it won't go very well.
>
> (Stylistically, I'd rather have the test and corresponding reaction at
> each call site rather than using a macro to hide it. I especially don't
> like the fact that the errdetail you propose would report a line of C
> code to the user, rather than an intelligible explanation of what that
> code is trying to do.)
>
> > It is quite straightforward to test it by attaching a debugger and
> setting
> > the return value of malloc, but to be able to have some "smarter"
> failures
> > only when, for example, starting a backend it would be necessary to have
> > something better integrated with the postgres code. Being able to attach
> a
> > uprobe using bpftrace and override the return value would be the most
> > elegant solution, but that is currently not possible, so considering
> > whether to add something like the mallocfail above to the regular
> testing.
>
> Hmm, I'm not sure that this type of testing is something to do on an
> ongoing basis.
>
> > Note: there are a bunch of other core dumps that occur during startup
> > before the postmaster is ready to accept connections, but I think that is
> > less of a problem since it fails during startup before the system is
> ready
> > to accept connections.
>
> That sounds reasonable. The service watcher should restart postmaster
> appropriately in that case (be it old SysV init, systemd, your container
> management layer, or whatever.)
>
Hi all,
I have gone over the two different approaches and have patches for both
attached so that we can compare. The second approach, using palloc() and
friends, is in my opinion safer so this is where I have spent most of the
time.
- The first approach allocates memory in the postmaster using malloc and
raises an error if that fails. If this happens in the backend after it is
started, a FATAL error is raised, which will abort the backend and send a
useful message to the connected client, but not bring down the postmaster.
The errors in the postmaster are caught using a PG_TRY(), which will
prevent the postmaster from aborting, and continue executing.
- The second approach creates a new memory context BackendStartupContext
where objects that need to live after the backend is forked are allocated.
Replace all uses of malloc() and friends with palloc() and friends. Since
palloc() can throw an error, we need to catch this using a PG_TRY() inside
the postmaster. In the event that the backend process throws an error, we
convert this to a FATAL error and re-throw it, which will cause the backend
process to terminate and send a useful message to the connected client.
The second approach, using palloc() and friends, is in my opinion less
error prone since it will capture any raised errors that prevent the
backend from starting, not just memory allocation errors. I have included a
file PATCH.md containing information about the manual tests I've done
(using a debugger) for both patches. There are a few pieces of memory that
are allocated in the PostmasterContext and then copied out from it before
destroying it. It would be possible to allocate this in the
BackendStartupContext instead (for the palloc() approach) and destroy the
PostmasterContext directly after the fork(), which provides a small
advantage in being slightly less error prone and slightly easier to
maintain.
Using malloc() and friends can be changed to not use ERROR-level reports
and in that sense do not require using PG_TRY() and can be re-written to
not use that. For palloc() and friends it seems they always run the risk of
throwing an ERROR, so without making it behave differently in the
postmaster (which seems to add unnecessary complications to the function),
it will require PG_TRY() to capture the errors.
Tom had some concerns about using PG_TRY() inside the postmaster
ServerLoop, but I cannot see how that can cause problems since it is "just"
a setjmp() and longjmp(). It allocates a structure on the stack that
contains the values of the registers and the signal set; it is big, but not
exceedingly so. If somebody could enlighten me about if there are any
problems with this, I would be very grateful.
Best wishes,
Mats Kindahl, Timescale
>
> --
> Álvaro Herrera PostgreSQL Developer —
> https://www.EnterpriseDB.com/
> "Ellos andaban todos desnudos como su madre los parió, y también las
> mujeres,
> aunque no vi más que una, harto moza, y todos los que yo vi eran todos
> mancebos, que ninguno vi de edad de más de XXX años" (Cristóbal Colón)
>
Attachments: download all (2 files, .tar.gz)
0001-Use-memory-context-for-backend-startup-objects.patch (text/x-patch, 15.7K) download
From b114661610387a1dda37d992b16349b49556dbd2 Mon Sep 17 00:00:00 2001
From: Mats Kindahl <mats@timescale.com>
Date: Wed, 14 Dec 2022 13:51:40 +0100
Subject: Use memory context for backend startup objects
There is no check for the return value of several allocations using
malloc(), calloc(), and strdup() in backend startup which means that it
is possible to get a segmentation fault when starting a backend if the
system is low on memory.
To handle backend startup, this commit create a dedicated memory
context BackendStartupContext for the backend startup and allocate
objects that need to be available during backend start there. It then
replaces these memory allocations with corresponding palloc(),
pstrdup(), and friends.
Most of the startup code already uses PostmasterContext to allocate
memory, but some locations where missing and this commit adds
allocation in the PostmasterContext rather than allocating memory
using malloc() and friends.
As a result, this will allocate memory that is only needed in the
postmaster in PostmasterContext, and memory that is used in the backend
in BackendStartupContext.
Since the postmaster should not exit if a memory allocation failure
occurs when trying to start a backend, and palloc() and friends
generate errors on failing to allocate memory, it will capture any
errors occuring when starting a backend and just continue running.
Since errors will automatically abort the backend start after this
change, some log-level messages are replaced with error-level messages
indicating that there were an error in starting the backend.
Fatal errors when starting the backend will still abort the postmaster.
---
PATCH.md | 96 ++++++++++++++++
src/backend/postmaster/postmaster.c | 172 ++++++++++++++++++----------
src/backend/tcop/postgres.c | 6 +-
src/backend/utils/mmgr/README | 6 +
src/backend/utils/mmgr/mcxt.c | 7 +-
src/include/utils/memutils.h | 1 +
6 files changed, 223 insertions(+), 65 deletions(-)
create mode 100644 PATCH.md
diff --git a/PATCH.md b/PATCH.md
new file mode 100644
index 0000000000..25e3e0e781
--- /dev/null
+++ b/PATCH.md
@@ -0,0 +1,96 @@
+# Notes about the patch
+
+This contains a few notes about the patch, in particular what was
+tested manually.
+
+## Manually executed tests
+
+The following tests were executed manually to check what happens when
+starting a backend and failing the memory allocation at various
+places.
+
+The testing was done by starting a debugger, setting a breakpoint in
+`BackendStartup` and `BackendInitialize` and setting the result of the
+returned memory to NULL at suitable points.
+
+### Failing to allocate memory when creating socket structure
+
+When failing to allocate memory in for the port structure, after
+having accepted the connection;
+
+```c
+ if (status == STATUS_OK) {
+ Port *bport;
+ BackendStartupContext = AllocSetContextCreate(TopMemoryContext,
+ "BackendStartup",
+ ALLOCSET_SMALL_SIZES);
+ oldcontext = MemoryContextSwitchTo(BackendStartupContext);
+>>> bport = palloc(sizeof(*bport));
+ memcpy(bport, &tmp_port, sizeof(*bport));
+ BackendStartup(bport);
+ }
+```
+
+We get this error.
+
+```
+$ psql
+psql: error: connection to server on socket "/tmp/.s.PGSQL.5432" failed: server closed the connection unexpectedly
+ This probably means the server terminated abnormally
+ before or while processing the request.
+```
+
+It would be nice to get a better error message on this failure.
+
+
+### Failing to allocate memory in postmaster process
+
+When allocating memory in the postmaster process, We expect the
+connection to be denied and the postmaster to keep on running.
+
+When setting `bn` to null in `BackendStartup` after the `malloc` here
+
+```c
+ bn = malloc(sizeof(Backend));
+ if (!bn)
+ ereport(ERROR,
+ (errcode(ERRCODE_OUT_OF_MEMORY),
+ errmsg("out of memory")));
+```
+
+I get this result when trying to start a backend.
+
+```
+mats@fury:~/work/mkindahl/postgres$ psql
+psql: error: connection to server on socket "/tmp/.s.PGSQL.5432" failed: server closed the connection unexpectedly
+ This probably means the server terminated abnormally
+ before or while processing the request.
+```
+
+It would be nice to get a better error message on this failure.
+
+### Allocating memory in backend process
+
+Second case is allocating memory in the backend and failing. When
+setting `port->remote_host` to NULL in `BackendInitialize` in this
+code:
+
+```c
+ /*
+ * Save remote_host and remote_port in port structure (after this, they
+ * will appear in log_line_prefix data for log messages).
+ */
+ port->remote_host = pstrdup(remote_host);
+ port->remote_port = pstrdup(remote_port);
+```
+
+The following error is printed:
+
+```
+mats@fury:~/work/mkindahl/postgres$ psql
+psql: error: connection to server on socket "/tmp/.s.PGSQL.5432" failed: FATAL: out of memory
+DETAIL: Failed on request of size 8 in memory context "BackendStartup".
+```
+
+This looks good. A FATAL level message will be sent to the client
+telling it what happened.
diff --git a/src/backend/postmaster/postmaster.c b/src/backend/postmaster/postmaster.c
index 2552327d90..52019c189c 100644
--- a/src/backend/postmaster/postmaster.c
+++ b/src/backend/postmaster/postmaster.c
@@ -399,7 +399,6 @@ static void unlink_external_pid_file(int status, Datum arg);
static void getInstallationPaths(const char *argv0);
static void checkControlFile(void);
static Port *ConnCreate(int serverFd);
-static void ConnFree(Port *port);
static void handle_pm_pmsignal_signal(SIGNAL_ARGS);
static void handle_pm_child_exit_signal(SIGNAL_ARGS);
static void handle_pm_reload_request_signal(SIGNAL_ARGS);
@@ -703,7 +702,7 @@ PostmasterMain(int argc, char *argv[])
break;
case 'C':
- output_config_variable = strdup(optarg);
+ output_config_variable = pstrdup(optarg);
break;
case 'c':
@@ -734,7 +733,7 @@ PostmasterMain(int argc, char *argv[])
}
case 'D':
- userDoption = strdup(optarg);
+ userDoption = pstrdup(optarg);
break;
case 'd':
@@ -1771,19 +1770,117 @@ ServerLoop(void)
if (events[i].events & WL_SOCKET_ACCEPT)
{
- Port *port;
+ /*
+ * We need to use volatile qualifier for both these variables
+ * to ensure that they are both stored and read from memory in
+ * the event that there is an error reported.
+ *
+ * Note that the Port structure is allocated on the stack to
+ * avoid triggering an error when we're low on memory. Use use
+ * PGINVALID_SOCKET to signal that the socket was not accepted
+ * so that we can close the socket on error.
+ */
+ volatile Port port = {PGINVALID_SOCKET};
+ volatile MemoryContext oldcontext = NULL;
- port = ConnCreate(events[i].fd);
- if (port)
+ /*
+ * We wrap this in PG_TRY since if any errors occur when
+ * trying to spawn the backend, we do not want to abort the
+ * postmaster and rather report the error and continue
+ * running.
+ *
+ * Note that we can reach the catch statement either in the
+ * postmaster or in the started backend, so we have to handle
+ * both cases.
+ */
+ PG_TRY();
{
- BackendStartup(port);
+ int status;
+ Port tmp_port = {PGINVALID_SOCKET};
/*
- * We no longer need the open socket or port structure in
- * this process
+ * We need to first accept the connection regardless of
+ * whether we can start the backend or not. This is
+ * necessary to be able to send failure message to the
+ * connecting client even if we have problems with the
+ * backend.
+ *
+ * Note that StreamConnection does not throw errors nor
+ * allocate memory, since this can fail.
+ */
+ status = StreamConnection(events[i].fd, &tmp_port);
+
+ /*
+ * We can now copy the port data to volatile memory, so
+ * that we can close the socket if an error is generated
+ * by BackendStartup().
+ */
+ port = tmp_port;
+
+ /*
+ * If everything is fine this far, we set up the backend
+ * context, allocate the port structure in this memory,
+ * and start the backend.
+ */
+ if (status == STATUS_OK) {
+ Port *bport;
+ BackendStartupContext = AllocSetContextCreate(TopMemoryContext,
+ "BackendStartup",
+ ALLOCSET_SMALL_SIZES);
+ oldcontext = MemoryContextSwitchTo(BackendStartupContext);
+ bport = palloc(sizeof(*bport));
+ memcpy(bport, &tmp_port, sizeof(*bport));
+ BackendStartup(bport);
+ }
+ }
+ PG_CATCH();
+ {
+ /*
+ * Note that fatal errors will not be caught by this
+ * catch, so do not throw them in the postmaster unless
+ * you want to shut it down.
+ *
+ * In the child process we can reach this catch since the
+ * backend startup might generate errors. In this case, we
+ * should abort the process and provide a useful message
+ * to the connected client, so we change the error message
+ * to FATAL and re-throw it.
*/
- StreamClose(port->sock);
- ConnFree(port);
+ if (IsUnderPostmaster) {
+ ErrorData *edata;
+
+ MemoryContextSwitchTo(oldcontext);
+ edata = CopyErrorData();
+ FlushErrorState();
+ edata->elevel = FATAL;
+ ReThrowError(edata);
+ }
+ }
+ PG_END_TRY();
+
+ /*
+ * We no longer need the open socket in this process,
+ * regardless of whether the backend startup failed or not. We
+ * do not reach this point if we are in the backend. See
+ * above.
+ *
+ * Also note that we can reach this part due to a failure in
+ * BackendStartup, so we need to check that 'port' is
+ * non-NULL.
+ */
+ Assert(!IsUnderPostmaster);
+ if (port.sock != PGINVALID_SOCKET)
+ StreamClose(port.sock);
+
+ /*
+ * We can now release the backend startup context, including
+ * the port data, since we're done with it in the postmaster.
+ */
+ if (oldcontext)
+ {
+ MemoryContextSwitchTo(oldcontext);
+ MemoryContextDelete(BackendStartupContext);
+ BackendStartupContext = NULL;
}
}
}
@@ -2517,49 +2614,6 @@ canAcceptConnections(int backend_type)
}
-/*
- * ConnCreate -- create a local connection data structure
- *
- * Returns NULL on failure, other than out-of-memory which is fatal.
- */
-static Port *
-ConnCreate(int serverFd)
-{
- Port *port;
-
- if (!(port = (Port *) calloc(1, sizeof(Port))))
- {
- ereport(LOG,
- (errcode(ERRCODE_OUT_OF_MEMORY),
- errmsg("out of memory")));
- ExitPostmaster(1);
- }
-
- if (StreamConnection(serverFd, port) != STATUS_OK)
- {
- if (port->sock != PGINVALID_SOCKET)
- StreamClose(port->sock);
- ConnFree(port);
- return NULL;
- }
-
- return port;
-}
-
-
-/*
- * ConnFree -- free a local connection data structure
- *
- * Caller has already closed the socket if any, so there's not much
- * to do here.
- */
-static void
-ConnFree(Port *port)
-{
- free(port);
-}
-
-
/*
* ClosePostmasterPorts -- close all the postmaster's open sockets
*
@@ -4122,7 +4176,7 @@ BackendStartup(Port *port)
bn = (Backend *) malloc(sizeof(Backend));
if (!bn)
{
- ereport(LOG,
+ ereport(ERROR,
(errcode(ERRCODE_OUT_OF_MEMORY),
errmsg("out of memory")));
return STATUS_ERROR;
@@ -4136,7 +4190,7 @@ BackendStartup(Port *port)
if (!RandomCancelKey(&MyCancelKey))
{
free(bn);
- ereport(LOG,
+ ereport(ERROR,
(errcode(ERRCODE_INTERNAL_ERROR),
errmsg("could not generate random cancel key")));
return STATUS_ERROR;
@@ -4340,8 +4394,8 @@ BackendInitialize(Port *port)
* Save remote_host and remote_port in port structure (after this, they
* will appear in log_line_prefix data for log messages).
*/
- port->remote_host = strdup(remote_host);
- port->remote_port = strdup(remote_port);
+ port->remote_host = pstrdup(remote_host);
+ port->remote_port = pstrdup(remote_port);
/* And now we can issue the Log_connections message, if wanted */
if (Log_connections)
@@ -4372,7 +4426,7 @@ BackendInitialize(Port *port)
ret == 0 &&
strspn(remote_host, "0123456789.") < strlen(remote_host) &&
strspn(remote_host, "0123456789ABCDEFabcdef:") < strlen(remote_host))
- port->remote_hostname = strdup(remote_host);
+ port->remote_hostname = pstrdup(remote_host);
/*
* Ready to begin client interaction. We will give up and _exit(1) after
diff --git a/src/backend/tcop/postgres.c b/src/backend/tcop/postgres.c
index 5d439f2710..5f89a4de3b 100644
--- a/src/backend/tcop/postgres.c
+++ b/src/backend/tcop/postgres.c
@@ -4147,9 +4147,9 @@ PostgresMain(const char *dbname, const char *username)
/*
* If the PostmasterContext is still around, recycle the space; we don't
* need it anymore after InitPostgres completes. Note this does not trash
- * *MyProcPort, because ConnCreate() allocated that space with malloc()
- * ... else we'd need to copy the Port data first. Also, subsidiary data
- * such as the username isn't lost either; see ProcessStartupPacket().
+ * *MyProcPort, because ConnCreate() allocated that space in the
+ * BackendStartupContext. Also, subsidiary data such as the username
+ * isn't lost either; see ProcessStartupPacket().
*/
if (PostmasterContext)
{
diff --git a/src/backend/utils/mmgr/README b/src/backend/utils/mmgr/README
index b20b9d4852..b0c3c72f8b 100644
--- a/src/backend/utils/mmgr/README
+++ b/src/backend/utils/mmgr/README
@@ -201,6 +201,12 @@ processes; so backends can't delete PostmasterContext until that's done.
ErrorContext --- the remaining top-level contexts are set up in each
backend during startup.)
+BackendStartupContext --- this is the context allocated for backend
+startup. This context is available because the PostmasterContext is
+deleted after the backend has started, but there is some information
+that needs to remain while the backend is running, for example
+information about the port.
+
CacheMemoryContext --- permanent storage for relcache, catcache, and
related modules. This will never be reset or deleted, either, so it's
not truly necessary to distinguish it from TopMemoryContext. But it
diff --git a/src/backend/utils/mmgr/mcxt.c b/src/backend/utils/mmgr/mcxt.c
index 0b00802df7..91f14efbce 100644
--- a/src/backend/utils/mmgr/mcxt.c
+++ b/src/backend/utils/mmgr/mcxt.c
@@ -141,6 +141,7 @@ MemoryContext CurrentMemoryContext = NULL;
MemoryContext TopMemoryContext = NULL;
MemoryContext ErrorContext = NULL;
MemoryContext PostmasterContext = NULL;
+MemoryContext BackendStartupContext = NULL;
MemoryContext CacheMemoryContext = NULL;
MemoryContext MessageContext = NULL;
MemoryContext TopTransactionContext = NULL;
@@ -718,9 +719,9 @@ MemoryContextStatsDetail(MemoryContext context, int max_children,
*
* We don't buffer the information about all memory contexts in a
* backend into StringInfo and log it as one message. That would
- * require the buffer to be enlarged, risking an OOM as there could
- * be a large number of memory contexts in a backend. Instead, we
- * log one message per memory context.
+ * require the buffer to be enlarged, risking an OOM as there could be
+ * a large number of memory contexts in a backend. Instead, we log
+ * one message per memory context.
*/
ereport(LOG_SERVER_ONLY,
(errhidestmt(true),
diff --git a/src/include/utils/memutils.h b/src/include/utils/memutils.h
index 21640d62a6..5d8619830d 100644
--- a/src/include/utils/memutils.h
+++ b/src/include/utils/memutils.h
@@ -58,6 +58,7 @@
extern PGDLLIMPORT MemoryContext TopMemoryContext;
extern PGDLLIMPORT MemoryContext ErrorContext;
extern PGDLLIMPORT MemoryContext PostmasterContext;
+extern PGDLLIMPORT MemoryContext BackendStartupContext;
extern PGDLLIMPORT MemoryContext CacheMemoryContext;
extern PGDLLIMPORT MemoryContext MessageContext;
extern PGDLLIMPORT MemoryContext TopTransactionContext;
--
2.34.1
0001-Check-result-of-memory-allocations-at-startup.patch (text/x-patch, 8.9K) download
From 292293e671f713bbb88c3ebf69a1bb61c4200392 Mon Sep 17 00:00:00 2001
From: Mats Kindahl <mats@timescale.com>
Date: Thu, 8 Dec 2022 10:00:23 +0100
Subject: Check result of memory allocations at startup
There are no checks for the return values of several allocations using
malloc(), calloc(), and strdup() in backend startup which means that
it is possible to get a segmentation fault when starting a backend if
the system is low on memory. It makes more sense to abort the spawning
of a backend if we're low on memory rather than tearing down the
entire server.
This commit fixes this by checking the return values of memory
allocation functions and abort with a fatal error if the memory
allocation fails.
Since the postmaster should not exit if a memory allocation failure
occurs when trying to start a backend, it will capture any errors
occuring when starting a backend and just continue running.
Fatal errors when starting the backend will still abort the postmaster,
but fatal errors in the backend will just abort starting the backend
and send an error message back to the client.
---
PATCH.md | 70 +++++++++++++++++++++++
src/backend/catalog/aclchk.c | 4 +-
src/backend/postmaster/postmaster.c | 86 ++++++++++++++++++++---------
3 files changed, 131 insertions(+), 29 deletions(-)
create mode 100644 PATCH.md
diff --git a/PATCH.md b/PATCH.md
new file mode 100644
index 0000000000..5ccd3979c8
--- /dev/null
+++ b/PATCH.md
@@ -0,0 +1,70 @@
+# Notes about the patch
+
+This contains a few notes about the patch, in particular what was
+tested manually.
+
+## Manually executed tests
+
+The following tests were executed manually to check what happens when
+starting a backend and failing the memory allocation at various
+places.
+
+### Allocating memory in postmaster process
+
+First case is when allocating memory in the postmaster process. We
+expect the connection to be denied and the postmaster to keep on
+running.
+
+When setting `bn` to null in `BackendStartup` after the `malloc` here
+
+```c
+ bn = malloc(sizeof(Backend));
+ if (!bn)
+ ereport(ERROR,
+ (errcode(ERRCODE_OUT_OF_MEMORY),
+ errmsg("out of memory")));
+```
+
+I get this result when trying to start a backend.
+
+```
+mats@fury:~/work/mkindahl/postgres$ psql
+psql: error: connection to server on socket "/tmp/.s.PGSQL.5432" failed: server closed the connection unexpectedly
+ This probably means the server terminated abnormally
+ before or while processing the request.
+```
+
+It would be nice to get a better error message, basically using the
+error message and send it to the client.
+
+### Allocating memory in backend process
+
+Second case is allocating memory in the backend and failing. When
+setting `port->remote_host` to NULL in `BackendInitialize` in this
+code:
+
+```c
+ /*
+ * Save remote_host and remote_port in port structure (after this, they
+ * will appear in log_line_prefix data for log messages).
+ */
+ port->remote_host = strdup(remote_host);
+ if (!port->remote_host)
+ ereport(FATAL,
+ (errcode(ERRCODE_OUT_OF_MEMORY),
+ errmsg("out of memory")));
+ port->remote_port = strdup(remote_port);
+ if (!port->remote_port)
+ ereport(FATAL,
+ (errcode(ERRCODE_OUT_OF_MEMORY),
+ errmsg("out of memory")));
+```
+
+The following error is printed:
+
+```
+mats@fury:~/work/mkindahl/postgres$ psql
+psql: error: connection to server on socket "/tmp/.s.PGSQL.5432" failed: FATAL: out of memory
+```
+
+This looks good. A FATAL level message will be sent to the client.
diff --git a/src/backend/catalog/aclchk.c b/src/backend/catalog/aclchk.c
index e48fc1647a..3ee1dc7568 100644
--- a/src/backend/catalog/aclchk.c
+++ b/src/backend/catalog/aclchk.c
@@ -3411,8 +3411,8 @@ pg_class_aclmask_ext(Oid table_oid, Oid roleid, AclMode mask,
result |= ACL_VACUUM;
/*
- * Check if ACL_ANALYZE is being checked and, if so, and not already set as
- * part of the result, then check if the user is a member of the
+ * Check if ACL_ANALYZE is being checked and, if so, and not already set
+ * as part of the result, then check if the user is a member of the
* pg_analyze_all_tables role, which allows ANALYZE on all relations.
*/
if (mask & ACL_ANALYZE &&
diff --git a/src/backend/postmaster/postmaster.c b/src/backend/postmaster/postmaster.c
index f459dab360..066e02530f 100644
--- a/src/backend/postmaster/postmaster.c
+++ b/src/backend/postmaster/postmaster.c
@@ -615,8 +615,8 @@ PostmasterMain(int argc, char *argv[])
*
* 1. We tell sigaction() to block all signals for the duration of the
* signal handler. This is faster than our old approach of
- * blocking/unblocking explicitly in the signal handler, and it should also
- * prevent excessive stack consumption if signals arrive quickly.
+ * blocking/unblocking explicitly in the signal handler, and it should
+ * also prevent excessive stack consumption if signals arrive quickly.
*
* 2. We do not set the SA_RESTART flag. This is because signals will be
* blocked at all times except when ServerLoop is waiting for something to
@@ -705,6 +705,10 @@ PostmasterMain(int argc, char *argv[])
case 'C':
output_config_variable = strdup(optarg);
+ if (!output_config_variable)
+ ereport(ERROR,
+ (errcode(ERRCODE_OUT_OF_MEMORY),
+ errmsg("out of memory")));
break;
case 'c':
@@ -736,6 +740,10 @@ PostmasterMain(int argc, char *argv[])
case 'D':
userDoption = strdup(optarg);
+ if (!userDoption)
+ ereport(ERROR,
+ (errcode(ERRCODE_OUT_OF_MEMORY),
+ errmsg("out of memory")));
break;
case 'd':
@@ -1783,18 +1791,35 @@ ServerLoop(void)
break;
if (FD_ISSET(ListenSocket[i], &rmask))
{
- Port *port;
+ Port *port = NULL;
- port = ConnCreate(ListenSocket[i]);
- if (port)
+ /*
+ * We wrap this in PG_TRY since if any errors occur when
+ * trying to spawn the backend, we do not want to abort
+ * the postmaster and rather report the error and continue
+ * running.
+ *
+ * Note that fatal errors will not be caught, so
+ * do not throw them unless you want to shut down the
+ * postmaster.
+ */
+ PG_TRY();
{
- BackendStartup(port);
+ port = ConnCreate(ListenSocket[i]);
+ if (port)
+ BackendStartup(port);
+ }
+ PG_END_TRY();
- /*
- * We no longer need the open socket or port structure
- * in this process
- */
- StreamClose(port->sock);
+ /*
+ * We no longer need the open socket or port structure
+ * in this process, and definitely not if we failed
+ * spawning a backend or creating the port structure.
+ */
+ if (port)
+ {
+ if (port->sock != PGINVALID_SOCKET)
+ StreamClose(port->sock);
ConnFree(port);
}
}
@@ -2561,21 +2586,15 @@ ConnCreate(int serverFd)
{
Port *port;
- if (!(port = (Port *) calloc(1, sizeof(Port))))
- {
- ereport(LOG,
+ if (!(port = calloc(1, sizeof(Port))))
+ ereport(ERROR,
(errcode(ERRCODE_OUT_OF_MEMORY),
errmsg("out of memory")));
- ExitPostmaster(1);
- }
+
+ /* Caller will deal with freeing anything that needs to be free'd */
if (StreamConnection(serverFd, port) != STATUS_OK)
- {
- if (port->sock != PGINVALID_SOCKET)
- StreamClose(port->sock);
- ConnFree(port);
return NULL;
- }
return port;
}
@@ -4054,14 +4073,12 @@ BackendStartup(Port *port)
* Create backend data structure. Better before the fork() so we can
* handle failure cleanly.
*/
- bn = (Backend *) malloc(sizeof(Backend));
+ bn = malloc(sizeof(Backend));
if (!bn)
- {
- ereport(LOG,
+ ereport(ERROR,
(errcode(ERRCODE_OUT_OF_MEMORY),
errmsg("out of memory")));
- return STATUS_ERROR;
- }
+
/*
* Compute the cancel key that will be assigned to this backend. The
@@ -4276,7 +4293,15 @@ BackendInitialize(Port *port)
* will appear in log_line_prefix data for log messages).
*/
port->remote_host = strdup(remote_host);
+ if (!port->remote_host)
+ ereport(FATAL,
+ (errcode(ERRCODE_OUT_OF_MEMORY),
+ errmsg("out of memory")));
port->remote_port = strdup(remote_port);
+ if (!port->remote_port)
+ ereport(FATAL,
+ (errcode(ERRCODE_OUT_OF_MEMORY),
+ errmsg("out of memory")));
/* And now we can issue the Log_connections message, if wanted */
if (Log_connections)
@@ -4307,7 +4332,14 @@ BackendInitialize(Port *port)
ret == 0 &&
strspn(remote_host, "0123456789.") < strlen(remote_host) &&
strspn(remote_host, "0123456789ABCDEFabcdef:") < strlen(remote_host))
+ {
port->remote_hostname = strdup(remote_host);
+ if (!port->remote_hostname)
+ ereport(FATAL,
+ (errcode(ERRCODE_OUT_OF_MEMORY),
+ errmsg("out of memory")));
+ }
+
/*
* Ready to begin client interaction. We will give up and _exit(1) after
@@ -4729,7 +4761,7 @@ retry:
*/
childinfo = malloc(sizeof(win32_deadchild_waitinfo));
if (!childinfo)
- ereport(FATAL,
+ ereport(ERROR,
(errcode(ERRCODE_OUT_OF_MEMORY),
errmsg("out of memory")));
--
2.34.1
* Re: Crash during backend start when low on memory
2022-12-14 12:58 Crash during backend start when low on memory Mats Kindahl <mats@timescale.com>
2022-12-14 13:23 ` Re: Crash during backend start when low on memory Daniel Gustafsson <daniel@yesql.se>
2022-12-14 21:14 ` Re: Crash during backend start when low on memory Mats Kindahl <mats@timescale.com>
2022-12-15 17:20 ` Re: Crash during backend start when low on memory Alvaro Herrera <alvherre@alvh.no-ip.org>
2022-12-16 08:46 ` Re: Crash during backend start when low on memory Mats Kindahl <mats@timescale.com>
2023-01-13 10:05 ` Re: Crash during backend start when low on memory Mats Kindahl <mats@timescale.com>
2023-01-13 11:53 ` Re: Crash during backend start when low on memory Alvaro Herrera <alvherre@alvh.no-ip.org>
2023-02-05 21:00 ` Re: Crash during backend start when low on memory Mats Kindahl <mats@timescale.com>
@ 2023-02-05 23:18 ` Andres Freund <andres@anarazel.de>
2023-02-06 07:37 ` Re: Crash during backend start when low on memory Mats Kindahl <mats@timescale.com>
0 siblings, 1 reply; 21+ messages in thread
From: Andres Freund @ 2023-02-05 23:18 UTC (permalink / raw)
To: Mats Kindahl <mats@timescale.com>; +Cc: Alvaro Herrera <alvherre@alvh.no-ip.org>; daniel@yesql.se; pgsql-bugs@lists.postgresql.org
Hi,
On 2023-02-05 22:00:20 +0100, Mats Kindahl wrote:
> Tom had some concerns about using PG_TRY() inside the postmaster
> ServerLoop
I don't think we gain anything in most of the places by using PG_TRY
instead of a non-throwing allocations. The relevant is cooperating
closely enough that just returning returning a failure indicator is
sufficient.
Note that that doesn't mean that palloc can't be used, see
MCXT_ALLOC_NO_OOM.
>, but I cannot see how that can cause problems since it is "just"
> a setjmp() and longjmp(). It allocates a structure on the stack that
> contains the values of the registers and the signal set; it is big, but not
> exceedingly so. If somebody could enlighten me about if there are any
> problems with this, I would be very grateful.
If you aren't careful you end up with PG_exception_stack in a backend
still pointing to that PG_TRY. Which means that an error during backend
startup would jump into postmaster code. Which would not be good.
Particularly because this is a bugfix we need to make the fix more
minimal than the patches proposed so far.
Greetings,
Andres Freund
^ permalink raw reply [nested|flat] 21+ messages in thread
* Re: Crash during backend start when low on memory
2022-12-14 12:58 Crash during backend start when low on memory Mats Kindahl <mats@timescale.com>
2022-12-14 13:23 ` Re: Crash during backend start when low on memory Daniel Gustafsson <daniel@yesql.se>
2022-12-14 21:14 ` Re: Crash during backend start when low on memory Mats Kindahl <mats@timescale.com>
2022-12-15 17:20 ` Re: Crash during backend start when low on memory Alvaro Herrera <alvherre@alvh.no-ip.org>
2022-12-16 08:46 ` Re: Crash during backend start when low on memory Mats Kindahl <mats@timescale.com>
2023-01-13 10:05 ` Re: Crash during backend start when low on memory Mats Kindahl <mats@timescale.com>
2023-01-13 11:53 ` Re: Crash during backend start when low on memory Alvaro Herrera <alvherre@alvh.no-ip.org>
2023-02-05 21:00 ` Re: Crash during backend start when low on memory Mats Kindahl <mats@timescale.com>
2023-02-05 23:18 ` Re: Crash during backend start when low on memory Andres Freund <andres@anarazel.de>
@ 2023-02-06 07:37 ` Mats Kindahl <mats@timescale.com>
2023-02-06 15:50 ` Re: Crash during backend start when low on memory Mats Kindahl <mats@timescale.com>
2023-02-06 18:41 ` Re: Crash during backend start when low on memory Andres Freund <andres@anarazel.de>
0 siblings, 2 replies; 21+ messages in thread
From: Mats Kindahl @ 2023-02-06 07:37 UTC (permalink / raw)
To: Andres Freund <andres@anarazel.de>; +Cc: Alvaro Herrera <alvherre@alvh.no-ip.org>; daniel@yesql.se; pgsql-bugs@lists.postgresql.org
On Mon, Feb 6, 2023 at 12:18 AM Andres Freund <andres@anarazel.de> wrote:
> Hi,
>
> On 2023-02-05 22:00:20 +0100, Mats Kindahl wrote:
> > Tom had some concerns about using PG_TRY() inside the postmaster
> > ServerLoop
>
> I don't think we gain anything in most of the places by using PG_TRY
> instead of a non-throwing allocations. The relevant is cooperating
> closely enough that just returning returning a failure indicator is
> sufficient.
>
> Note that that doesn't mean that palloc can't be used, see
> MCXT_ALLOC_NO_OOM.
>
Ah, yes, then both patches can be rewritten to not use PG_TRY().
>
> >, but I cannot see how that can cause problems since it is "just"
> > a setjmp() and longjmp(). It allocates a structure on the stack that
> > contains the values of the registers and the signal set; it is big, but
> not
> > exceedingly so. If somebody could enlighten me about if there are any
> > problems with this, I would be very grateful.
>
> If you aren't careful you end up with PG_exception_stack in a backend
> still pointing to that PG_TRY. Which means that an error during backend
> startup would jump into postmaster code. Which would not be good.
>
I did run into this case when "failing" some of the memory allocations
inside the backend with a debugger (see the PATCH.md).
There are, however, already cases in the backend startup that can raise an
error (for example, the code that creates the MessageContext in
PostgresMain). So, either if one of these error cases is triggered, or if
you make a mistake and add an ereport(ERROR,...) at the wrong place during
startup, it could cause this problem. In other words, wouldn't adding a
PG_TRY() in the code spawning the backend *prevent* such a problem and
hence reduce the risk of a mistake in code causing the backend jumping into
postmaster code (of course assuming that the code is written correctly).
>
> Particularly because this is a bugfix we need to make the fix more
> minimal than the patches proposed so far.
>
I can remove the PG_TRY() and use ereport(LOG, ...) + ExitPostmaster if it
feels like a safer path for incorporating a bug fix, but note that there is
still a risk that the backend will tear down the postmaster. For example,
allocating memory for a memory context can fail, or throwing another error
inside the backend startup can also fail, so I think that there will be
more work to do after this.
Best wishes,
Mats Kindahl
>
>
> Greetings,
>
> Andres Freund
>
^ permalink raw reply [nested|flat] 21+ messages in thread
* Re: Crash during backend start when low on memory
2022-12-14 12:58 Crash during backend start when low on memory Mats Kindahl <mats@timescale.com>
2022-12-14 13:23 ` Re: Crash during backend start when low on memory Daniel Gustafsson <daniel@yesql.se>
2022-12-14 21:14 ` Re: Crash during backend start when low on memory Mats Kindahl <mats@timescale.com>
2022-12-15 17:20 ` Re: Crash during backend start when low on memory Alvaro Herrera <alvherre@alvh.no-ip.org>
2022-12-16 08:46 ` Re: Crash during backend start when low on memory Mats Kindahl <mats@timescale.com>
2023-01-13 10:05 ` Re: Crash during backend start when low on memory Mats Kindahl <mats@timescale.com>
2023-01-13 11:53 ` Re: Crash during backend start when low on memory Alvaro Herrera <alvherre@alvh.no-ip.org>
2023-02-05 21:00 ` Re: Crash during backend start when low on memory Mats Kindahl <mats@timescale.com>
2023-02-05 23:18 ` Re: Crash during backend start when low on memory Andres Freund <andres@anarazel.de>
2023-02-06 07:37 ` Re: Crash during backend start when low on memory Mats Kindahl <mats@timescale.com>
@ 2023-02-06 15:50 ` Mats Kindahl <mats@timescale.com>
1 sibling, 0 replies; 21+ messages in thread
From: Mats Kindahl @ 2023-02-06 15:50 UTC (permalink / raw)
To: Andres Freund <andres@anarazel.de>; +Cc: Alvaro Herrera <alvherre@alvh.no-ip.org>; daniel@yesql.se; pgsql-bugs@lists.postgresql.org
On Mon, Feb 6, 2023 at 8:37 AM Mats Kindahl <mats@timescale.com> wrote:
>
> I can remove the PG_TRY() and use ereport(LOG, ...) + ExitPostmaster if it
> feels like a safer path for incorporating a bug fix, but note that there is
> still a risk that the backend will tear down the postmaster. For example,
> allocating memory for a memory context can fail, or throwing another error
> inside the backend startup can also fail, so I think that there will be
> more work to do after this.
>
Not ExitPostmaster, of course, but some proc_exit call for the backend and
returning status code for the postmaster.
^ permalink raw reply [nested|flat] 21+ messages in thread
* Re: Crash during backend start when low on memory
2022-12-14 12:58 Crash during backend start when low on memory Mats Kindahl <mats@timescale.com>
2022-12-14 13:23 ` Re: Crash during backend start when low on memory Daniel Gustafsson <daniel@yesql.se>
2022-12-14 21:14 ` Re: Crash during backend start when low on memory Mats Kindahl <mats@timescale.com>
2022-12-15 17:20 ` Re: Crash during backend start when low on memory Alvaro Herrera <alvherre@alvh.no-ip.org>
2022-12-16 08:46 ` Re: Crash during backend start when low on memory Mats Kindahl <mats@timescale.com>
2023-01-13 10:05 ` Re: Crash during backend start when low on memory Mats Kindahl <mats@timescale.com>
2023-01-13 11:53 ` Re: Crash during backend start when low on memory Alvaro Herrera <alvherre@alvh.no-ip.org>
2023-02-05 21:00 ` Re: Crash during backend start when low on memory Mats Kindahl <mats@timescale.com>
2023-02-05 23:18 ` Re: Crash during backend start when low on memory Andres Freund <andres@anarazel.de>
2023-02-06 07:37 ` Re: Crash during backend start when low on memory Mats Kindahl <mats@timescale.com>
@ 2023-02-06 18:41 ` Andres Freund <andres@anarazel.de>
2023-02-28 20:39 ` Re: Crash during backend start when low on memory Mats Kindahl <mats@timescale.com>
1 sibling, 1 reply; 21+ messages in thread
From: Andres Freund @ 2023-02-06 18:41 UTC (permalink / raw)
To: Mats Kindahl <mats@timescale.com>; +Cc: Alvaro Herrera <alvherre@alvh.no-ip.org>; daniel@yesql.se; pgsql-bugs@lists.postgresql.org
Hi,
On 2023-02-06 08:37:17 +0100, Mats Kindahl wrote:
> On Mon, Feb 6, 2023 at 12:18 AM Andres Freund <andres@anarazel.de> wrote:
> > >, but I cannot see how that can cause problems since it is "just"
> > > a setjmp() and longjmp(). It allocates a structure on the stack that
> > > contains the values of the registers and the signal set; it is big, but
> > not
> > > exceedingly so. If somebody could enlighten me about if there are any
> > > problems with this, I would be very grateful.
> >
> > If you aren't careful you end up with PG_exception_stack in a backend
> > still pointing to that PG_TRY. Which means that an error during backend
> > startup would jump into postmaster code. Which would not be good.
> >
>
> I did run into this case when "failing" some of the memory allocations
> inside the backend with a debugger (see the PATCH.md).
>
> There are, however, already cases in the backend startup that can raise an
> error (for example, the code that creates the MessageContext in
> PostgresMain). So, either if one of these error cases is triggered, or if
> you make a mistake and add an ereport(ERROR,...) at the wrong place during
> startup, it could cause this problem. In other words, wouldn't adding a
> PG_TRY() in the code spawning the backend *prevent* such a problem and
> hence reduce the risk of a mistake in code causing the backend jumping into
> postmaster code (of course assuming that the code is written correctly).
It's fine to add ereport(ERROR)s inside the backend startup - elog.c knows how
to deal with no error handlers being set up. We just promote the ERROR to a
FATAL. That's not at all the same as having a PG_exception_stack set up at a
point we don't want to return to.
Remember that backends are forked, so they inherit the stack that postmaster
has set up.
> > Particularly because this is a bugfix we need to make the fix more
> > minimal than the patches proposed so far.
> >
>
> I can remove the PG_TRY() and use ereport(LOG, ...) + ExitPostmaster if it
> feels like a safer path for incorporating a bug fix, but note that there is
> still a risk that the backend will tear down the postmaster. For example,
> allocating memory for a memory context can fail, or throwing another error
> inside the backend startup can also fail, so I think that there will be
> more work to do after this.
I can't really follow.
Greetings,
Andres Freund
^ permalink raw reply [nested|flat] 21+ messages in thread
* Re: Crash during backend start when low on memory
2022-12-14 12:58 Crash during backend start when low on memory Mats Kindahl <mats@timescale.com>
2022-12-14 13:23 ` Re: Crash during backend start when low on memory Daniel Gustafsson <daniel@yesql.se>
2022-12-14 21:14 ` Re: Crash during backend start when low on memory Mats Kindahl <mats@timescale.com>
2022-12-15 17:20 ` Re: Crash during backend start when low on memory Alvaro Herrera <alvherre@alvh.no-ip.org>
2022-12-16 08:46 ` Re: Crash during backend start when low on memory Mats Kindahl <mats@timescale.com>
2023-01-13 10:05 ` Re: Crash during backend start when low on memory Mats Kindahl <mats@timescale.com>
2023-01-13 11:53 ` Re: Crash during backend start when low on memory Alvaro Herrera <alvherre@alvh.no-ip.org>
2023-02-05 21:00 ` Re: Crash during backend start when low on memory Mats Kindahl <mats@timescale.com>
2023-02-05 23:18 ` Re: Crash during backend start when low on memory Andres Freund <andres@anarazel.de>
2023-02-06 07:37 ` Re: Crash during backend start when low on memory Mats Kindahl <mats@timescale.com>
2023-02-06 18:41 ` Re: Crash during backend start when low on memory Andres Freund <andres@anarazel.de>
@ 2023-02-28 20:39 ` Mats Kindahl <mats@timescale.com>
0 siblings, 0 replies; 21+ messages in thread
From: Mats Kindahl @ 2023-02-28 20:39 UTC (permalink / raw)
To: Andres Freund <andres@anarazel.de>; +Cc: Alvaro Herrera <alvherre@alvh.no-ip.org>; daniel@yesql.se; pgsql-bugs@lists.postgresql.org
On Mon, Feb 6, 2023 at 7:41 PM Andres Freund <andres@anarazel.de> wrote:
> Hi,
>
> On 2023-02-06 08:37:17 +0100, Mats Kindahl wrote:
> > On Mon, Feb 6, 2023 at 12:18 AM Andres Freund <andres@anarazel.de>
> wrote:
> > > >, but I cannot see how that can cause problems since it is "just"
> > > > a setjmp() and longjmp(). It allocates a structure on the stack that
> > > > contains the values of the registers and the signal set; it is big,
> but
> > > not
> > > > exceedingly so. If somebody could enlighten me about if there are any
> > > > problems with this, I would be very grateful.
> > >
> > > If you aren't careful you end up with PG_exception_stack in a backend
> > > still pointing to that PG_TRY. Which means that an error during backend
> > > startup would jump into postmaster code. Which would not be good.
> > >
> >
> > I did run into this case when "failing" some of the memory allocations
> > inside the backend with a debugger (see the PATCH.md).
> >
> > There are, however, already cases in the backend startup that can raise
> an
> > error (for example, the code that creates the MessageContext in
> > PostgresMain). So, either if one of these error cases is triggered, or if
> > you make a mistake and add an ereport(ERROR,...) at the wrong place
> during
> > startup, it could cause this problem. In other words, wouldn't adding a
> > PG_TRY() in the code spawning the backend *prevent* such a problem and
> > hence reduce the risk of a mistake in code causing the backend jumping
> into
> > postmaster code (of course assuming that the code is written correctly).
>
> It's fine to add ereport(ERROR)s inside the backend startup - elog.c knows
> how
> to deal with no error handlers being set up. We just promote the ERROR to a
> FATAL. That's not at all the same as having a PG_exception_stack set up at
> a
> point we don't want to return to.
>
Remember that backends are forked, so they inherit the stack that postmaster
> has set up.
>
>
> > > Particularly because this is a bugfix we need to make the fix more
> > > minimal than the patches proposed so far.
> > >
> >
> > I can remove the PG_TRY() and use ereport(LOG, ...) + ExitPostmaster if
> it
> > feels like a safer path for incorporating a bug fix, but note that there
> is
> > still a risk that the backend will tear down the postmaster. For example,
> > allocating memory for a memory context can fail, or throwing another
> error
> > inside the backend startup can also fail, so I think that there will be
> > more work to do after this.
>
> I can't really follow.
>
My apologies, I'll add my reasoning below.
However, since you wanted a patch not using PG_TRY for the bugfix I have
attached a patch that does not use it and instead uses memory context
functions and the MCXT_ALLOC_NO_OOM flag where applicable. I've tested it
manually similarly to the other patches and it passes "make check" and
"make installcheck". I have added comments to explain my reasoning for the
important allocations: what memory context it is allocated in and why.
My reasoning was that using a PG_TRY around the call to BackendStartup()
would prevent any errors generated now and in future code from killing the
postmaster, so it would be a more safe alternative. The difference in size
is not that big between the patches, but this patch does not use exception
handling.
Best wishes,
Mats Kindahl
> Greetings,
>
> Andres Freund
>
Attachments:
0001-Don-t-crash-on-out-of-memory-when-starting-backend.patch (text/x-patch, 4.0K) download
From c7ea8ad840317d729c4fc3f14b4124372e2c942c Mon Sep 17 00:00:00 2001
From: Mats Kindahl <mats@timescale.com>
Date: Thu, 16 Feb 2023 14:32:13 +0100
Subject: Don't crash on out of memory when starting backend
There are no checks for the return values of several allocations using
malloc(), calloc(), and strdup() in backend startup which means that
it is possible to get a segmentation fault when starting a backend if
the system is low on memory. It makes more sense to abort the spawning
of a backend if we're low on memory rather than tearing down the
entire server.
This commit fixes this by using memory context allocations rather than
malloc and friends.
Since the postmaster should not exit if a memory allocation failure
occurs when trying to start a backend, it will use the no-error version
of the memory context allocation functions and add checks if we're out
of memory, log an error, and just continue running.
Memory allocation errors when initializing the backend process will
generate errors instead and as a result send the error message to the
connecting client indicating that the server is out of memory and the
backend will shut down after reporting the error.
---
src/backend/postmaster/postmaster.c | 34 ++++++++++++++++++++++-------
1 file changed, 26 insertions(+), 8 deletions(-)
diff --git a/src/backend/postmaster/postmaster.c b/src/backend/postmaster/postmaster.c
index 2552327d90..794c6a4bd1 100644
--- a/src/backend/postmaster/postmaster.c
+++ b/src/backend/postmaster/postmaster.c
@@ -703,7 +703,7 @@ PostmasterMain(int argc, char *argv[])
break;
case 'C':
- output_config_variable = strdup(optarg);
+ output_config_variable = pstrdup(optarg);
break;
case 'c':
@@ -734,7 +734,7 @@ PostmasterMain(int argc, char *argv[])
}
case 'D':
- userDoption = strdup(optarg);
+ userDoption = pstrdup(optarg);
break;
case 'd':
@@ -2527,12 +2527,23 @@ ConnCreate(int serverFd)
{
Port *port;
- if (!(port = (Port *) calloc(1, sizeof(Port))))
+ /*
+ * We allocate the port memory in TopMemoryContext since the port
+ * information will be saved for the duration of the backend (as
+ * MyProcPort).
+ *
+ * Once the backend has started, we need to free the memory, or we will
+ * exhaust the memory over time.
+ */
+ port = MemoryContextAllocExtended(TopMemoryContext,
+ sizeof(Port),
+ MCXT_ALLOC_ZERO | MCXT_ALLOC_NO_OOM);
+ if (port == NULL)
{
ereport(LOG,
(errcode(ERRCODE_OUT_OF_MEMORY),
errmsg("out of memory")));
- ExitPostmaster(1);
+ return NULL;
}
if (StreamConnection(serverFd, port) != STATUS_OK)
@@ -2556,7 +2567,7 @@ ConnCreate(int serverFd)
static void
ConnFree(Port *port)
{
- free(port);
+ pfree(port);
}
@@ -4339,9 +4350,16 @@ BackendInitialize(Port *port)
/*
* Save remote_host and remote_port in port structure (after this, they
* will appear in log_line_prefix data for log messages).
+ *
+ * We allocate the port structure in TopMemoryContext since it will remain
+ * for the duration of the backend (in MyProcPort). It is not necessary to
+ * check the returned value since MemoryContextStrdup() will error out if
+ * it cannot allocate memory. This is fine since we are running in the
+ * backend and it will just report the error to the connected client and
+ * shut down the backend.
*/
- port->remote_host = strdup(remote_host);
- port->remote_port = strdup(remote_port);
+ port->remote_host = MemoryContextStrdup(TopMemoryContext, remote_host);
+ port->remote_port = MemoryContextStrdup(TopMemoryContext, remote_port);
/* And now we can issue the Log_connections message, if wanted */
if (Log_connections)
@@ -4372,7 +4390,7 @@ BackendInitialize(Port *port)
ret == 0 &&
strspn(remote_host, "0123456789.") < strlen(remote_host) &&
strspn(remote_host, "0123456789ABCDEFabcdef:") < strlen(remote_host))
- port->remote_hostname = strdup(remote_host);
+ port->remote_hostname = MemoryContextStrdup(TopMemoryContext, remote_host);
/*
* Ready to begin client interaction. We will give up and _exit(1) after
--
2.34.1
end of thread, other threads:[~2023-02-28 20:39 UTC | newest]
Thread overview: 21+ messages (download: mbox mbox.gz follow: Atom feed)
-- links below jump to the message on this page --
2022-12-14 12:58 Crash during backend start when low on memory Mats Kindahl <mats@timescale.com>
2022-12-14 13:23 ` Daniel Gustafsson <daniel@yesql.se>
2022-12-14 15:54 ` Tom Lane <tgl@sss.pgh.pa.us>
2022-12-14 21:21 ` Mats Kindahl <mats@timescale.com>
2022-12-14 21:14 ` Mats Kindahl <mats@timescale.com>
2022-12-15 17:20 ` Alvaro Herrera <alvherre@alvh.no-ip.org>
2022-12-16 08:46 ` Mats Kindahl <mats@timescale.com>
2023-01-13 10:05 ` Mats Kindahl <mats@timescale.com>
2023-01-13 11:53 ` Alvaro Herrera <alvherre@alvh.no-ip.org>
2023-01-13 15:01 ` Tom Lane <tgl@sss.pgh.pa.us>
2023-01-16 20:52 ` Mats Kindahl <mats@timescale.com>
2023-01-16 20:58 ` Tom Lane <tgl@sss.pgh.pa.us>
2023-01-16 21:10 ` Mats Kindahl <mats@timescale.com>
2023-01-16 21:49 ` Tom Lane <tgl@sss.pgh.pa.us>
2023-01-16 10:09 ` Mats Kindahl <mats@timescale.com>
2023-02-05 21:00 ` Mats Kindahl <mats@timescale.com>
2023-02-05 23:18 ` Andres Freund <andres@anarazel.de>
2023-02-06 07:37 ` Mats Kindahl <mats@timescale.com>
2023-02-06 15:50 ` Mats Kindahl <mats@timescale.com>
2023-02-06 18:41 ` Andres Freund <andres@anarazel.de>
2023-02-28 20:39 ` Mats Kindahl <mats@timescale.com>
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox