public inbox for [email protected]
help / color / mirror / Atom feed[PATCH v14 2/7] Don't proc_exit() in startup's SIGTERM handler if forked by system().
16+ messages / 5 participants
[nested] [flat]
* [PATCH v14 2/7] 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; 16+ 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
--IS0zKkzwUGydFO0o
Content-Type: text/x-diff; charset=us-ascii
Content-Disposition: attachment;
filename="v14-0003-introduce-routine-for-checking-mutually-exclusiv.patch"
^ permalink raw reply [nested|flat] 16+ messages in thread
* [PATCH v15 2/7] 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; 16+ 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
--k+w/mQv8wyuph6w0
Content-Type: text/x-diff; charset=us-ascii
Content-Disposition: attachment;
filename="v15-0003-introduce-routine-for-checking-mutually-exclusiv.patch"
^ permalink raw reply [nested|flat] 16+ messages in thread
* [PATCH v6 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; 16+ 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 | 20 +++++++++++++++++++-
src/backend/storage/ipc/ipc.c | 3 +++
src/backend/storage/lmgr/proc.c | 2 ++
3 files changed, 24 insertions(+), 1 deletion(-)
diff --git a/src/backend/postmaster/startup.c b/src/backend/postmaster/startup.c
index efc2580536..de2b56c2fa 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,23 @@ 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
+ {
+ const char msg[] = "StartupProcShutdownHandler() called in child process";
+ int rc pg_attribute_unused();
+
+ rc = write(STDERR_FILENO, msg, sizeof(msg));
+ _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..e3da0622d7 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(MyProcPid == (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(MyProcPid == (int) getpid()); /* not safe if forked by system(), etc. */
auxproc = &AuxiliaryProcs[proctype];
--
2.25.1
--wac7ysb48OaltWcw--
^ permalink raw reply [nested|flat] 16+ messages in thread
* [PATCH v7 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; 16+ 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 | 20 +++++++++++++++++++-
src/backend/storage/ipc/ipc.c | 3 +++
src/backend/storage/lmgr/proc.c | 2 ++
3 files changed, 24 insertions(+), 1 deletion(-)
diff --git a/src/backend/postmaster/startup.c b/src/backend/postmaster/startup.c
index efc2580536..bace915881 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,23 @@ 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
+ {
+ const char msg[] = "StartupProcShutdownHandler() called in child process\n";
+ int rc pg_attribute_unused();
+
+ rc = write(STDERR_FILENO, msg, sizeof(msg));
+ _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..e3da0622d7 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(MyProcPid == (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(MyProcPid == (int) getpid()); /* not safe if forked by system(), etc. */
auxproc = &AuxiliaryProcs[proctype];
--
2.25.1
--u3/rZRmxL6MmkK24--
^ permalink raw reply [nested|flat] 16+ messages in thread
* [PATCH v8 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; 16+ 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 | 20 +++++++++++++++++++-
src/backend/storage/ipc/ipc.c | 3 +++
src/backend/storage/lmgr/proc.c | 2 ++
3 files changed, 24 insertions(+), 1 deletion(-)
diff --git a/src/backend/postmaster/startup.c b/src/backend/postmaster/startup.c
index efc2580536..bace915881 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,23 @@ 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
+ {
+ const char msg[] = "StartupProcShutdownHandler() called in child process\n";
+ int rc pg_attribute_unused();
+
+ rc = write(STDERR_FILENO, msg, sizeof(msg));
+ _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];
--
2.25.1
--FCuugMFkClbJLl1L--
^ permalink raw reply [nested|flat] 16+ messages in thread
* [PATCH v9 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; 16+ 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 ++
3 files changed, 21 insertions(+), 1 deletion(-)
diff --git a/src/backend/postmaster/startup.c b/src/backend/postmaster/startup.c
index efc2580536..261f992357 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("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];
--
2.25.1
--IS0zKkzwUGydFO0o--
^ permalink raw reply [nested|flat] 16+ messages in thread
* [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; 16+ 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] 16+ messages in thread
* [PATCH v13 2/7] 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; 16+ 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
--5vNYLRcllDrimb99
Content-Type: text/x-diff; charset=us-ascii
Content-Disposition: attachment;
filename="v13-0003-introduce-routine-for-checking-mutually-exclusiv.patch"
^ permalink raw reply [nested|flat] 16+ messages in thread
* [PATCH v11 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; 16+ 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 checks in 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 | 4 ++++
src/backend/storage/lmgr/proc.c | 8 ++++++++
src/backend/utils/error/elog.c | 28 ++++++++++++++++++++++++++++
src/include/utils/elog.h | 6 ++++++
5 files changed, 62 insertions(+), 1 deletion(-)
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..6591b5d6a8 100644
--- a/src/backend/storage/ipc/ipc.c
+++ b/src/backend/storage/ipc/ipc.c
@@ -103,6 +103,10 @@ static int on_proc_exit_index,
void
proc_exit(int code)
{
+ /* not safe if forked by system(), etc. */
+ if (MyProcPid != (int) getpid())
+ elog(PANIC, "proc_exit() called in child process");
+
/* 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 5b663a2997..e9e445bb21 100644
--- a/src/backend/storage/lmgr/proc.c
+++ b/src/backend/storage/lmgr/proc.c
@@ -806,6 +806,10 @@ ProcKill(int code, Datum arg)
Assert(MyProc != NULL);
+ /* not safe if forked by system(), etc. */
+ if (MyProc->pid != (int) getpid())
+ elog(PANIC, "ProcKill() called in child process");
+
/* Make sure we're out of the sync rep lists */
SyncRepCleanupAtProcExit();
@@ -926,6 +930,10 @@ AuxiliaryProcKill(int code, Datum arg)
Assert(proctype >= 0 && proctype < NUM_AUXILIARY_PROCS);
+ /* not safe if forked by system(), etc. */
+ if (MyProc->pid != (int) getpid())
+ elog(PANIC, "AuxiliaryProcKill() called in child process");
+
auxproc = &AuxiliaryProcs[proctype];
Assert(MyProc == auxproc);
diff --git a/src/backend/utils/error/elog.c b/src/backend/utils/error/elog.c
index 8e1f3e8521..eeb238331e 100644
--- a/src/backend/utils/error/elog.c
+++ b/src/backend/utils/error/elog.c
@@ -3733,6 +3733,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 *str)
+{
+ int nwritten = 0;
+ int ntotal = strlen(str);
+
+ while (nwritten < ntotal)
+ {
+ int rc;
+
+ rc = write(STDERR_FILENO, str + 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..0292e88b4f 100644
--- a/src/include/utils/elog.h
+++ b/src/include/utils/elog.h
@@ -536,4 +536,10 @@ extern void write_jsonlog(ErrorData *edata);
*/
extern void write_stderr(const char *fmt,...) pg_attribute_printf(1, 2);
+/*
+ * Write a message to STDERR using only async-signal-safe functions. This can
+ * be used to safely emit a message from a signal handler.
+ */
+extern void write_stderr_signal_safe(const char *fmt);
+
#endif /* ELOG_H */
--
2.25.1
--jRHKVT23PllUwdXP--
^ permalink raw reply [nested|flat] 16+ messages in thread
* Enhance pg_dump multi-threaded streaming (WAS: Re: filesystem full during vacuum - space recovery issues)
@ 2024-07-18 23:08 Thomas Simpson <[email protected]>
0 siblings, 2 replies; 16+ messages in thread
From: Thomas Simpson @ 2024-07-18 23:08 UTC (permalink / raw)
To: [email protected]; [email protected]
[Added cross post to [email protected] - background is
multi-TB database needs recovered via pgdumpall & reload, thoughts on
ways to make pg_dump scale to multi-thread to expedite loading to a new
cluster. Straight dump to a file is impractical as the dump will be
>200TB; hackers may be a better home for the discussion than current
admin list]
Hi Ron
On 18-Jul-2024 18:41, Ron Johnson wrote:
> Multi-threaded writing to the same giant text file won't work too
> well, when all the data for one table needs to be together.
>
> Just temporarily add another disk for backups.
>
For clarity, I'm not proposing multi threaded writing to one file; the
proposal is a new special mode which specifically makes multiple output
streams across *network sockets* to a listener which is listening on the
other side. The goal is avoiding any files at all and only using
multiple network streams to gain multi-threaded processing with some
co-ordination to keep things organized and consistent.
This would really be specifically for the use-case of dump/reload
upgrade or recreate rather than everyday use. And particularly for very
large databases.
Looking at pg_dump.c it's doing the baseline organization but the
extension would be adding the required coordination with the
destination. So, for a huge table (I have many) these would go in
different streams but if there is a dependency (FK relations etc) the
checkpoint needs to ensure those are met before proceeding. Worst case
scenario it would end up using only 1 thread but it would be very
unusual to have a database where every table depends on another table
all the way down.
In theory at least, some gains should be achieved for typical databases
where a degree of parallelism is possible.
Thanks
Tom
> On Thu, Jul 18, 2024 at 4:55 PM Thomas Simpson <[email protected]> wrote:
>
>
> On 18-Jul-2024 16:32, Ron Johnson wrote:
>> On Thu, Jul 18, 2024 at 3:01 PM Thomas Simpson
>> <[email protected]> wrote:
>> [snip]
>>
>> [BTW, v9.6 which I know is old but this server is stuck there]
>>
>> [snip]
>>
>> I know I'm stuck with the slow rebuild at this point.
>> However, I doubt I am the only person in the world that needs
>> to dump and reload a large database. My thought is this is a
>> weak point for PostgreSQL so it makes sense to consider ways
>> to improve the dump reload process, especially as it's the
>> last-resort upgrade path recommended in the upgrade guide and
>> the general fail-safe route to get out of trouble.
>>
>> No database does fast single-threaded backups.
>
> Agreed. My thought is that is should be possible for a 'new
> dumpall' to be multi-threaded.
>
> Something like :
>
> * Set number of threads on 'source' (perhaps by querying a
> listening destination for how many threads it is prepared to
> accept via a control port)
>
> * Select each database in turn
>
> * Organize the tables which do not have references themselves
>
> * Send each table separately in each thread (or queue them until a
> thread is available) ('Stage 1')
>
> * Rendezvous stage 1 completion (pause sending, wait until
> feedback from destination confirming all completed) so we have a
> known consistent state that is safe to proceed to subsequent tables
>
> * Work through tables that do refer to the previously sent in the
> same way (since the tables they reference exist and have their
> data) ('Stage 2')
>
> * Repeat progressively until all tables are done ('Stage 3', 4
> etc. as necessary)
>
> The current dumpall is essentially doing this table organization
> currently [minus stage checkpoints/multi-thread] otherwise the
> dump/load would not work. It may even be doing a lot of this for
> 'directory' mode? The change here is organizing n threads to
> process them concurrently where possible and coordinating the
> pipes so they only send data which can be accepted.
>
> The destination would need to have a multi-thread listen and
> co-ordinate with the sender on some control channel so feed back
> completion of each stage.
>
> Something like a destination host and control channel port to
> establish the pipes and create additional netcat pipes on
> incremental ports above the control port for each thread used.
>
> Dumpall seems like it could be a reasonable start point since it
> is already doing the complicated bits of serializing the dump data
> so it can be consistently loaded.
>
> Probably not really an admin question at this point, more a
> feature enhancement.
>
> Is there anything fundamentally wrong that someone with more
> intimate knowledge of dumpall could point out?
>
> Thanks
>
> Tom
>
>
^ permalink raw reply [nested|flat] 16+ messages in thread
* Re: Enhance pg_dump multi-threaded streaming (WAS: Re: filesystem full during vacuum - space recovery issues)
@ 2024-07-19 12:21 Ron Johnson <[email protected]>
parent: Thomas Simpson <[email protected]>
1 sibling, 0 replies; 16+ messages in thread
From: Ron Johnson @ 2024-07-19 12:21 UTC (permalink / raw)
To: Pgsql-admin <[email protected]>
200TB... how do you currently back up your database?
On Fri, Jul 19, 2024 at 5:08 AM Thomas Simpson <[email protected]> wrote:
> [Added cross post to [email protected] - background is
> multi-TB database needs recovered via pgdumpall & reload, thoughts on ways
> to make pg_dump scale to multi-thread to expedite loading to a new
> cluster. Straight dump to a file is impractical as the dump will be
> >200TB; hackers may be a better home for the discussion than current admin
> list]
>
> Hi Ron
>
> On 18-Jul-2024 18:41, Ron Johnson wrote:
>
> Multi-threaded writing to the same giant text file won't work too well,
> when all the data for one table needs to be together.
>
> Just temporarily add another disk for backups.
>
> For clarity, I'm not proposing multi threaded writing to one file; the
> proposal is a new special mode which specifically makes multiple output
> streams across *network sockets* to a listener which is listening on the
> other side. The goal is avoiding any files at all and only using multiple
> network streams to gain multi-threaded processing with some co-ordination
> to keep things organized and consistent.
>
> This would really be specifically for the use-case of dump/reload upgrade
> or recreate rather than everyday use. And particularly for very large
> databases.
>
> Looking at pg_dump.c it's doing the baseline organization but the
> extension would be adding the required coordination with the destination.
> So, for a huge table (I have many) these would go in different streams but
> if there is a dependency (FK relations etc) the checkpoint needs to ensure
> those are met before proceeding. Worst case scenario it would end up using
> only 1 thread but it would be very unusual to have a database where every
> table depends on another table all the way down.
>
> In theory at least, some gains should be achieved for typical databases
> where a degree of parallelism is possible.
> Thanks
>
> Tom
>
>
>
> On Thu, Jul 18, 2024 at 4:55 PM Thomas Simpson <[email protected]> wrote:
>
>>
>> On 18-Jul-2024 16:32, Ron Johnson wrote:
>>
>> On Thu, Jul 18, 2024 at 3:01 PM Thomas Simpson <[email protected]> wrote:
>> [snip]
>>
>>> [BTW, v9.6 which I know is old but this server is stuck there]
>>>
>> [snip]
>>
>>> I know I'm stuck with the slow rebuild at this point. However, I doubt
>>> I am the only person in the world that needs to dump and reload a large
>>> database. My thought is this is a weak point for PostgreSQL so it makes
>>> sense to consider ways to improve the dump reload process, especially as
>>> it's the last-resort upgrade path recommended in the upgrade guide and the
>>> general fail-safe route to get out of trouble.
>>>
>> No database does fast single-threaded backups.
>>
>> Agreed. My thought is that is should be possible for a 'new dumpall' to
>> be multi-threaded.
>>
>> Something like :
>>
>> * Set number of threads on 'source' (perhaps by querying a listening
>> destination for how many threads it is prepared to accept via a control
>> port)
>>
>> * Select each database in turn
>>
>> * Organize the tables which do not have references themselves
>>
>> * Send each table separately in each thread (or queue them until a thread
>> is available) ('Stage 1')
>>
>> * Rendezvous stage 1 completion (pause sending, wait until feedback from
>> destination confirming all completed) so we have a known consistent state
>> that is safe to proceed to subsequent tables
>>
>> * Work through tables that do refer to the previously sent in the same
>> way (since the tables they reference exist and have their data) ('Stage 2')
>>
>> * Repeat progressively until all tables are done ('Stage 3', 4 etc. as
>> necessary)
>>
>> The current dumpall is essentially doing this table organization
>> currently [minus stage checkpoints/multi-thread] otherwise the dump/load
>> would not work. It may even be doing a lot of this for 'directory' mode?
>> The change here is organizing n threads to process them concurrently where
>> possible and coordinating the pipes so they only send data which can be
>> accepted.
>>
>> The destination would need to have a multi-thread listen and co-ordinate
>> with the sender on some control channel so feed back completion of each
>> stage.
>>
>> Something like a destination host and control channel port to establish
>> the pipes and create additional netcat pipes on incremental ports above the
>> control port for each thread used.
>>
>> Dumpall seems like it could be a reasonable start point since it is
>> already doing the complicated bits of serializing the dump data so it can
>> be consistently loaded.
>>
>> Probably not really an admin question at this point, more a feature
>> enhancement.
>>
>> Is there anything fundamentally wrong that someone with more intimate
>> knowledge of dumpall could point out?
>>
>> Thanks
>>
>> Tom
>>
>>
>>
^ permalink raw reply [nested|flat] 16+ messages in thread
* Re: Enhance pg_dump multi-threaded streaming (WAS: Re: filesystem full during vacuum - space recovery issues)
@ 2024-07-19 13:26 Scott Ribe <[email protected]>
parent: Thomas Simpson <[email protected]>
1 sibling, 1 reply; 16+ messages in thread
From: Scott Ribe @ 2024-07-19 13:26 UTC (permalink / raw)
To: Thomas Simpson <[email protected]>; +Cc: Pgsql-admin <[email protected]>; [email protected]
Do you actually have 100G networking between the nodes? Because if not, a single CPU should be able to saturate 10G.
Likewise the receiving end would need disk capable of keeping up. Which brings up the question, why not write to disk, but directly to the destination rather than write locally then copy?
Do you require dump-reload because of suspected corruption? That's a tough one. But if not, if the goal is just to get up and running on a new server, why not pg_basebackup, streaming replica, promote? That depends on the level of data modification activity being low enough that pg_basebackup can keep up with WAL as it's generated and apply it faster than new WAL comes in, but given that your server is currently keeping up with writing that much WAL and flushing that many changes, seems likely it would keep up as long as the network connection is fast enough. Anyway, in that scenario, you don't need to care how long pg_basebackup takes.
If you do need a dump/reload because of suspected corruption, the only thing I can think of is something like doing it a table at a time--partitioning would help here, if practical.
^ permalink raw reply [nested|flat] 16+ messages in thread
* Re: Enhance pg_dump multi-threaded streaming (WAS: Re: filesystem full during vacuum - space recovery issues)
@ 2024-07-19 13:46 Thomas Simpson <[email protected]>
parent: Scott Ribe <[email protected]>
0 siblings, 2 replies; 16+ messages in thread
From: Thomas Simpson @ 2024-07-19 13:46 UTC (permalink / raw)
To: Scott Ribe <[email protected]>; [email protected]
Hi Scott,
I realize some of the background was snipped on what I sent to the
hacker list, I'll try to fill in the details.
Short background is very large database ran out of space during vacuum
full taking down the server. There is a replica which was applying the
WALs and so it too ran out of space. On restart after clearing some
space, the database came back up but left over the in-progress rebuild
files. I've cleared that replica and am using it as my rebuild target
just now.
Trying to identify the 'orphan' files and move them away always led to
the database spotting the supposedly unused files having gone and
refusing to start, so I had no successful way to clean up and get space
back.
Last resort after discussion is pg_dumpall & reload. I'm doing this via
a network pipe (netcat) as I do not have the vast amount of storage
necessary for the dump file to be stored (in any format).
On 19-Jul-2024 09:26, Scott Ribe wrote:
> Do you actually have 100G networking between the nodes? Because if not, a single CPU should be able to saturate 10G.
Servers connect via 10G WAN; sending is not the issue, it's application
of the incoming stream on the destination which is bottlenecked.
>
> Likewise the receiving end would need disk capable of keeping up. Which brings up the question, why not write to disk, but directly to the destination rather than write locally then copy?
In this case, it's not a local write, it's piped via netcat.
> Do you require dump-reload because of suspected corruption? That's a tough one. But if not, if the goal is just to get up and running on a new server, why not pg_basebackup, streaming replica, promote? That depends on the level of data modification activity being low enough that pg_basebackup can keep up with WAL as it's generated and apply it faster than new WAL comes in, but given that your server is currently keeping up with writing that much WAL and flushing that many changes, seems likely it would keep up as long as the network connection is fast enough. Anyway, in that scenario, you don't need to care how long pg_basebackup takes.
>
> If you do need a dump/reload because of suspected corruption, the only thing I can think of is something like doing it a table at a time--partitioning would help here, if practical.
The basebackup is, to the best of my understanding, essentially just
copying the database files. Since the failed vacuum has left extra
files, my expectation is these too would be copied, leaving me in the
same position I started in. If I'm wrong, please tell me as that would
be vastly quicker - it is how I originally set up the replica and it
took only a few hours on the 10G link.
The inability to get a clean start if I move any files out the way leads
me to be concerned for some underlying corruption/issue and the
recommendation earlier in the discussion was opt for dump/reload as the
fail-safe.
Resigned to my fate, my thoughts were to see if there is a way to
improve the dump-reload approach for the future. Since dump-reload is
the ultimate upgrade suggestion in the documentation, it seems
worthwhile to see if there is a way to improve the performance of that
especially as very large databases like mine are a thing with
PostgreSQL. From a quick review of pg_dump.c (I'm no expert on it
obviously), it feels like it's already doing most of what needs done and
the addition is some sort of multi-thread coordination with a restore
client to ensure each thread can successfully complete each task it has
before accepting more work. I realize that's actually difficult to
implement.
Thanks
Tom
^ permalink raw reply [nested|flat] 16+ messages in thread
* Re: Enhance pg_dump multi-threaded streaming (WAS: Re: filesystem full during vacuum - space recovery issues)
@ 2024-07-19 19:34 Scott Ribe <[email protected]>
parent: Thomas Simpson <[email protected]>
1 sibling, 1 reply; 16+ messages in thread
From: Scott Ribe @ 2024-07-19 19:34 UTC (permalink / raw)
To: Thomas Simpson <[email protected]>; +Cc: Pgsql-admin <[email protected]>
> On Jul 19, 2024, at 7:46 AM, Thomas Simpson <[email protected]> wrote:
>
> I realize some of the background was snipped on what I sent to the hacker list, I'll try to fill in the details.
I was gone from my computer for a day and lost track of the thread.
Perhaps logical replication could help you out here?
^ permalink raw reply [nested|flat] 16+ messages in thread
* Re: Enhance pg_dump multi-threaded streaming (WAS: Re: filesystem full during vacuum - space recovery issues)
@ 2024-07-19 20:23 Thomas Simpson <[email protected]>
parent: Scott Ribe <[email protected]>
0 siblings, 0 replies; 16+ messages in thread
From: Thomas Simpson @ 2024-07-19 20:23 UTC (permalink / raw)
To: Scott Ribe <[email protected]>; +Cc: [email protected]
Hi Scott
On 19-Jul-2024 15:34, Scott Ribe wrote:
>> On Jul 19, 2024, at 7:46 AM, Thomas Simpson<[email protected]> wrote:
>>
>> I realize some of the background was snipped on what I sent to the hacker list, I'll try to fill in the details.
> I was gone from my computer for a day and lost track of the thread.
>
> Perhaps logical replication could help you out here?
I'm not sure - perhaps, but at this point, I've got that dump/reload
running and provided it completes ok (in about 20 days time at current
rate), I'll be fine with this.
The database itself is essentially an archive of data so is no longer
being added to at this point, so it's an annoyance for the rebuild time
rather than a disaster.
[But incidentally, I am working on an even larger project which is
likely to make this one seem small, so improvement around large
databases is important to me.]
However, my thought is around how to avoid this issue in the future and
to improve the experience for others faced with the dump-reload which is
always the fall-back upgrade suggestion between versions.
Getting parallelism should be possible and the current pg_dump does that
for directory mode from what I can see - making multiple threads etc.
according to parallel.c in pg_dump, it even looks like most of where my
thought process was going is actually already there.
The extension should be adding synchronization/checkpointing between the
generating dump and the receiving reload to ensure objects are not
processed until all their requirements are already present in the new
database. This is all based around routing via network streams instead
of the filesystem as currently happens.
Perhaps this is already in place since the restore can be done in
parallel, so must need to implement that ordering already? If someone
with a good understanding of dump is able to comment or even give
suggestions, I'm not against making an attempt to implement something as
a first attempt.
I see Tom Lane from git blame did a bunch of work around the parallel
dump back in 2020 - perhaps he could make suggestions either via private
direct email or the list ?
Thanks
Tom
^ permalink raw reply [nested|flat] 16+ messages in thread
* Re: Enhance pg_dump multi-threaded streaming (WAS: Re: filesystem full during vacuum - space recovery issues)
@ 2024-07-22 15:50 Andrew Dunstan <[email protected]>
parent: Thomas Simpson <[email protected]>
1 sibling, 0 replies; 16+ messages in thread
From: Andrew Dunstan @ 2024-07-22 15:50 UTC (permalink / raw)
To: Thomas Simpson <[email protected]>; Scott Ribe <[email protected]>; [email protected]
On 2024-07-19 Fr 9:46 AM, Thomas Simpson wrote:
>
> Hi Scott,
>
> I realize some of the background was snipped on what I sent to the
> hacker list, I'll try to fill in the details.
>
> Short background is very large database ran out of space during vacuum
> full taking down the server. There is a replica which was applying
> the WALs and so it too ran out of space. On restart after clearing
> some space, the database came back up but left over the in-progress
> rebuild files. I've cleared that replica and am using it as my
> rebuild target just now.
>
> Trying to identify the 'orphan' files and move them away always led to
> the database spotting the supposedly unused files having gone and
> refusing to start, so I had no successful way to clean up and get
> space back.
>
> Last resort after discussion is pg_dumpall & reload. I'm doing this
> via a network pipe (netcat) as I do not have the vast amount of
> storage necessary for the dump file to be stored (in any format).
>
> On 19-Jul-2024 09:26, Scott Ribe wrote:
>> Do you actually have 100G networking between the nodes? Because if not, a single CPU should be able to saturate 10G.
> Servers connect via 10G WAN; sending is not the issue, it's
> application of the incoming stream on the destination which is
> bottlenecked.
>> Likewise the receiving end would need disk capable of keeping up. Which brings up the question, why not write to disk, but directly to the destination rather than write locally then copy?
> In this case, it's not a local write, it's piped via netcat.
>> Do you require dump-reload because of suspected corruption? That's a tough one. But if not, if the goal is just to get up and running on a new server, why not pg_basebackup, streaming replica, promote? That depends on the level of data modification activity being low enough that pg_basebackup can keep up with WAL as it's generated and apply it faster than new WAL comes in, but given that your server is currently keeping up with writing that much WAL and flushing that many changes, seems likely it would keep up as long as the network connection is fast enough. Anyway, in that scenario, you don't need to care how long pg_basebackup takes.
>>
>> If you do need a dump/reload because of suspected corruption, the only thing I can think of is something like doing it a table at a time--partitioning would help here, if practical.
>
> The basebackup is, to the best of my understanding, essentially just
> copying the database files. Since the failed vacuum has left extra
> files, my expectation is these too would be copied, leaving me in the
> same position I started in. If I'm wrong, please tell me as that
> would be vastly quicker - it is how I originally set up the replica
> and it took only a few hours on the 10G link.
>
> The inability to get a clean start if I move any files out the way
> leads me to be concerned for some underlying corruption/issue and the
> recommendation earlier in the discussion was opt for dump/reload as
> the fail-safe.
>
> Resigned to my fate, my thoughts were to see if there is a way to
> improve the dump-reload approach for the future. Since dump-reload is
> the ultimate upgrade suggestion in the documentation, it seems
> worthwhile to see if there is a way to improve the performance of that
> especially as very large databases like mine are a thing with
> PostgreSQL. From a quick review of pg_dump.c (I'm no expert on it
> obviously), it feels like it's already doing most of what needs done
> and the addition is some sort of multi-thread coordination with a
> restore client to ensure each thread can successfully complete each
> task it has before accepting more work. I realize that's actually
> difficult to implement.
>
>
There is a plan for a non-text mode for pg_dumpall. I have started work
on it, and hope to have a WIP patch in a month or so. It's not my
intention to parallelize it for the first cut, but it could definitely
be parallelizable in future. However, it will require writing to disk
somewhere, albeit that the data will be compressed. It's well nigh
impossible to parallelize text format dumps.
Restoration of custom and directory format dumps has long been
parallelized. Parallel dumps require directory format, and so will
non-text pg_dumpall.
cheers
andrew
--
Andrew Dunstan
EDB: https://www.enterprisedb.com
^ permalink raw reply [nested|flat] 16+ messages in thread
end of thread, other threads:[~2024-07-22 15:50 UTC | newest]
Thread overview: 16+ messages (download: mbox mbox.gz follow: Atom feed)
-- links below jump to the message on this page --
2023-02-14 17:44 [PATCH v13 2/7] Don't proc_exit() in startup's SIGTERM handler if forked by system(). Nathan Bossart <[email protected]>
2023-02-14 17:44 [PATCH v6 2/2] Don't proc_exit() in startup's SIGTERM handler if forked by system(). Nathan Bossart <[email protected]>
2023-02-14 17:44 [PATCH v8 2/2] Don't proc_exit() in startup's SIGTERM handler if forked by system(). Nathan Bossart <[email protected]>
2023-02-14 17:44 [PATCH v9 2/2] Don't proc_exit() in startup's SIGTERM handler if forked by system(). Nathan Bossart <[email protected]>
2023-02-14 17:44 [PATCH v15 2/7] Don't proc_exit() in startup's SIGTERM handler if forked by system(). Nathan Bossart <[email protected]>
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]>
2023-02-14 17:44 [PATCH v11 2/2] Don't proc_exit() in startup's SIGTERM handler if forked by system(). Nathan Bossart <[email protected]>
2023-02-14 17:44 [PATCH v14 2/7] Don't proc_exit() in startup's SIGTERM handler if forked by system(). Nathan Bossart <[email protected]>
2023-02-14 17:44 [PATCH v7 2/2] Don't proc_exit() in startup's SIGTERM handler if forked by system(). Nathan Bossart <[email protected]>
2024-07-18 23:08 Enhance pg_dump multi-threaded streaming (WAS: Re: filesystem full during vacuum - space recovery issues) Thomas Simpson <[email protected]>
2024-07-19 12:21 ` Re: Enhance pg_dump multi-threaded streaming (WAS: Re: filesystem full during vacuum - space recovery issues) Ron Johnson <[email protected]>
2024-07-19 13:26 ` Re: Enhance pg_dump multi-threaded streaming (WAS: Re: filesystem full during vacuum - space recovery issues) Scott Ribe <[email protected]>
2024-07-19 13:46 ` Re: Enhance pg_dump multi-threaded streaming (WAS: Re: filesystem full during vacuum - space recovery issues) Thomas Simpson <[email protected]>
2024-07-19 19:34 ` Re: Enhance pg_dump multi-threaded streaming (WAS: Re: filesystem full during vacuum - space recovery issues) Scott Ribe <[email protected]>
2024-07-19 20:23 ` Re: Enhance pg_dump multi-threaded streaming (WAS: Re: filesystem full during vacuum - space recovery issues) Thomas Simpson <[email protected]>
2024-07-22 15:50 ` Re: Enhance pg_dump multi-threaded streaming (WAS: Re: filesystem full during vacuum - space recovery issues) Andrew Dunstan <[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