public inbox for [email protected]
help / color / mirror / Atom feed[PATCH v10 2/2] Don't proc_exit() in startup's SIGTERM handler if forked by system().
5+ messages / 4 participants
[nested] [flat]
* [PATCH v10 2/2] Don't proc_exit() in startup's SIGTERM handler if forked by system().
@ 2023-02-14 17:44 Nathan Bossart <[email protected]>
0 siblings, 0 replies; 5+ messages in thread
From: Nathan Bossart @ 2023-02-14 17:44 UTC (permalink / raw)
Instead, emit a message to STDERR and _exit() in this case. This
change also adds assertions to proc_exit(), ProcKill(), and
AuxiliaryProcKill() to verify that these functions are not called
by a process forked by system(), etc.
---
src/backend/postmaster/startup.c | 17 ++++++++++++++++-
src/backend/storage/ipc/ipc.c | 3 +++
src/backend/storage/lmgr/proc.c | 2 ++
src/backend/utils/error/elog.c | 28 ++++++++++++++++++++++++++++
src/include/utils/elog.h | 6 +-----
5 files changed, 50 insertions(+), 6 deletions(-)
diff --git a/src/backend/postmaster/startup.c b/src/backend/postmaster/startup.c
index efc2580536..0e7de26bc2 100644
--- a/src/backend/postmaster/startup.c
+++ b/src/backend/postmaster/startup.c
@@ -19,6 +19,8 @@
*/
#include "postgres.h"
+#include <unistd.h>
+
#include "access/xlog.h"
#include "access/xlogrecovery.h"
#include "access/xlogutils.h"
@@ -121,7 +123,20 @@ StartupProcShutdownHandler(SIGNAL_ARGS)
int save_errno = errno;
if (in_restore_command)
- proc_exit(1);
+ {
+ /*
+ * If we are in a child process (e.g., forked by system() in
+ * RestoreArchivedFile()), we don't want to call any exit callbacks.
+ * The parent will take care of that.
+ */
+ if (MyProcPid == (int) getpid())
+ proc_exit(1);
+ else
+ {
+ write_stderr_signal_safe("StartupProcShutdownHandler() called in child process\n");
+ _exit(1);
+ }
+ }
else
shutdown_requested = true;
WakeupRecovery();
diff --git a/src/backend/storage/ipc/ipc.c b/src/backend/storage/ipc/ipc.c
index 1904d21795..d5097dc008 100644
--- a/src/backend/storage/ipc/ipc.c
+++ b/src/backend/storage/ipc/ipc.c
@@ -103,6 +103,9 @@ static int on_proc_exit_index,
void
proc_exit(int code)
{
+ /* proc_exit() is not safe in forked processes from system(), etc. */
+ Assert(MyProcPid == (int) getpid());
+
/* Clean up everything that must be cleaned up */
proc_exit_prepare(code);
diff --git a/src/backend/storage/lmgr/proc.c b/src/backend/storage/lmgr/proc.c
index 22b4278610..b9e2c3aafe 100644
--- a/src/backend/storage/lmgr/proc.c
+++ b/src/backend/storage/lmgr/proc.c
@@ -805,6 +805,7 @@ ProcKill(int code, Datum arg)
dlist_head *procgloballist;
Assert(MyProc != NULL);
+ Assert(MyProc->pid == (int) getpid()); /* not safe if forked by system(), etc. */
/* Make sure we're out of the sync rep lists */
SyncRepCleanupAtProcExit();
@@ -925,6 +926,7 @@ AuxiliaryProcKill(int code, Datum arg)
PGPROC *proc;
Assert(proctype >= 0 && proctype < NUM_AUXILIARY_PROCS);
+ Assert(MyProc->pid == (int) getpid()); /* not safe if forked by system(), etc. */
auxproc = &AuxiliaryProcs[proctype];
diff --git a/src/backend/utils/error/elog.c b/src/backend/utils/error/elog.c
index 5898100acb..f9925c8d8e 100644
--- a/src/backend/utils/error/elog.c
+++ b/src/backend/utils/error/elog.c
@@ -3730,6 +3730,34 @@ write_stderr(const char *fmt,...)
}
+/*
+ * Write a message to STDERR using only async-signal-safe functions. This can
+ * be used to safely emit a message from a signal handler.
+ *
+ * TODO: It is likely possible to safely do a limited amount of string
+ * interpolation (e.g., %s and %d), but that is not presently supported.
+ */
+void
+write_stderr_signal_safe(const char *fmt)
+{
+ int nwritten = 0;
+ int ntotal = strlen(fmt);
+
+ while (nwritten < ntotal)
+ {
+ int rc;
+
+ rc = write(STDERR_FILENO, fmt + nwritten, ntotal - nwritten);
+
+ /* Just give up on error. There isn't much else we can do. */
+ if (rc == -1)
+ return;
+
+ nwritten += rc;
+ }
+}
+
+
/*
* Adjust the level of a recovery-related message per trace_recovery_messages.
*
diff --git a/src/include/utils/elog.h b/src/include/utils/elog.h
index 4a9562fdaa..20f1b54c6a 100644
--- a/src/include/utils/elog.h
+++ b/src/include/utils/elog.h
@@ -529,11 +529,7 @@ extern void write_pipe_chunks(char *data, int len, int dest);
extern void write_csvlog(ErrorData *edata);
extern void write_jsonlog(ErrorData *edata);
-/*
- * Write errors to stderr (or by equal means when stderr is
- * not available). Used before ereport/elog can be used
- * safely (memory context, GUC load etc)
- */
extern void write_stderr(const char *fmt,...) pg_attribute_printf(1, 2);
+extern void write_stderr_signal_safe(const char *fmt);
#endif /* ELOG_H */
--
2.25.1
--Q68bSM7Ycu6FN28Q--
^ permalink raw reply [nested|flat] 5+ messages in thread
* Re: Remove useless casts to (char *)
@ 2025-02-23 14:23 Peter Eisentraut <[email protected]>
0 siblings, 1 reply; 5+ messages in thread
From: Peter Eisentraut @ 2025-02-23 14:23 UTC (permalink / raw)
To: Dagfinn Ilmari Mannsåker <[email protected]>; +Cc: pgsql-hackers
I have committed the rest of this with the adjustments you suggested.
On 10.02.25 18:44, Dagfinn Ilmari Mannsåker wrote:
>> Here is a new patch set rebased over that.
>
> I had a more thorough read-through this time (as well as applying and
> building it), and it does make the code a lot more readable.
>
> I noticed you in some places added extra parens around remaining casts
> with offset additions, e.g.
>
> - XLogRegisterData((char *) old_key_tuple->t_data + SizeofHeapTupleHeader,
> + XLogRegisterData(((char *) old_key_tuple->t_data) + SizeofHeapTupleHeader,
> old_key_tuple->t_len - SizeofHeapTupleHeader);
>
> But not in others:
>
> - memcpy((char *) tuple->t_data + SizeofHeapTupleHeader,
> - (char *) data,
> - datalen);
> + memcpy((char *) tuple->t_data + SizeofHeapTupleHeader, data, datalen);
>
>
> I don't have a particularly strong opinion either way (maybe -0.2 on the
> extra parens), but I mainly think we should keep it consistent, and not
> change it gratuitously.
>
> Greppig indicates to me that the paren-less version is more common:
>
> $ git grep -P '\(\w+\s*\**\) [\w>-]+ \+ \w+' | wc -l
> 283
> $ git grep -P '\(\(\w+\s*\**\) [\w>-]+\) \+ \w+' | wc -l
> 96
>
> So I think we should leave them as they are.
>
> - ilmari
>
>
^ permalink raw reply [nested|flat] 5+ messages in thread
* Re: Remove useless casts to (char *)
@ 2025-03-26 15:01 Vladlen Popolitov <[email protected]>
parent: Peter Eisentraut <[email protected]>
0 siblings, 1 reply; 5+ messages in thread
From: Vladlen Popolitov @ 2025-03-26 15:01 UTC (permalink / raw)
To: Peter Eisentraut <[email protected]>; +Cc: Dagfinn Ilmari Mannsåker <[email protected]>; pgsql-hackers
Peter Eisentraut писал(а) 2025-02-23 21:23:
> I have committed the rest of this with the adjustments you suggested.
>
>
> On 10.02.25 18:44, Dagfinn Ilmari Mannsåker wrote:
>>> Here is a new patch set rebased over that.
Hi
I mentioned this patch in my message
https://www.postgresql.org/message-id/f28f3b45ec84bf9dc99fe129023a2d6b%40postgrespro.ru
Starting from it queries with Parallel Seq Scan (probably with other
parallel executor nodes)
hang under the debugger in Linux and MacOs.
--
Best regards,
Vladlen Popolitov.
^ permalink raw reply [nested|flat] 5+ messages in thread
* Re: Remove useless casts to (char *)
@ 2025-03-26 22:20 Michael Paquier <[email protected]>
parent: Vladlen Popolitov <[email protected]>
0 siblings, 1 reply; 5+ messages in thread
From: Michael Paquier @ 2025-03-26 22:20 UTC (permalink / raw)
To: Vladlen Popolitov <[email protected]>; +Cc: Peter Eisentraut <[email protected]>; Dagfinn Ilmari Mannsåker <[email protected]>; pgsql-hackers
On Wed, Mar 26, 2025 at 10:01:47PM +0700, Vladlen Popolitov wrote:
> I mentioned this patch in my message https://www.postgresql.org/message-id/f28f3b45ec84bf9dc99fe129023a2d6b%40postgrespro.ru
> Starting from it queries with Parallel Seq Scan (probably with other
> parallel executor nodes)
> hang under the debugger in Linux and MacOs.
Uh. How could a simple cast impact a parallel seqscan? That seems
unrelated to me.
--
Michael
Attachments:
[application/pgp-signature] signature.asc (833B, ../../[email protected]/2-signature.asc)
download
^ permalink raw reply [nested|flat] 5+ messages in thread
* Re: Remove useless casts to (char *)
@ 2025-03-27 06:46 Vladlen Popolitov <[email protected]>
parent: Michael Paquier <[email protected]>
0 siblings, 0 replies; 5+ messages in thread
From: Vladlen Popolitov @ 2025-03-27 06:46 UTC (permalink / raw)
To: Michael Paquier <[email protected]>; +Cc: Peter Eisentraut <[email protected]>; Dagfinn Ilmari Mannsåker <[email protected]>; pgsql-hackers
Michael Paquier писал(а) 2025-03-27 05:20:
> On Wed, Mar 26, 2025 at 10:01:47PM +0700, Vladlen Popolitov wrote:
>> I mentioned this patch in my message
>> https://www.postgresql.org/message-id/f28f3b45ec84bf9dc99fe129023a2d6b%40postgrespro.ru
>> Starting from it queries with Parallel Seq Scan (probably with other
>> parallel executor nodes)
>> hang under the debugger in Linux and MacOs.
>
> Uh. How could a simple cast impact a parallel seqscan? That seems
> unrelated to me.
> --
> Michael
Hi Michel,
I changed the debugging extension in VScode and this issue disappeared.
I am sorry for disturbing and taking your time.
--
Best regards,
Vladlen Popolitov.
^ permalink raw reply [nested|flat] 5+ messages in thread
end of thread, other threads:[~2025-03-27 06:46 UTC | newest]
Thread overview: 5+ messages (download: mbox mbox.gz follow: Atom feed)
-- links below jump to the message on this page --
2023-02-14 17:44 [PATCH v10 2/2] Don't proc_exit() in startup's SIGTERM handler if forked by system(). Nathan Bossart <[email protected]>
2025-02-23 14:23 Re: Remove useless casts to (char *) Peter Eisentraut <[email protected]>
2025-03-26 15:01 ` Re: Remove useless casts to (char *) Vladlen Popolitov <[email protected]>
2025-03-26 22:20 ` Re: Remove useless casts to (char *) Michael Paquier <[email protected]>
2025-03-27 06:46 ` Re: Remove useless casts to (char *) Vladlen Popolitov <[email protected]>
This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox