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(). 12+ messages / 3 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; 12+ 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] 12+ 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; 12+ 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] 12+ 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; 12+ 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] 12+ 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; 12+ 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] 12+ 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; 12+ 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] 12+ 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; 12+ 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] 12+ 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; 12+ 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] 12+ 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; 12+ 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] 12+ 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; 12+ 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] 12+ messages in thread
* Use streaming read API in pgstattuple. @ 2024-11-25 18:17 Kirill Reshke <[email protected]> 0 siblings, 1 reply; 12+ messages in thread From: Kirill Reshke @ 2024-11-25 18:17 UTC (permalink / raw) To: PostgreSQL Hackers <[email protected]> Hi! While reviewing other threads implementing stream API for various core subsystems, I spotted that pgstattuple could also benefit from that. So, PFA. Notice refactoring around pgstat_hash_page and changes in pgstat_page signature. This is needed because pgstat_hash_tuple uses _hash_getbuf_with_strategy rather than ReadBufferExtended, which is OK, but makes adapting the streaming API impossible. So, I changed this place. Old codes should behave exactly the same way as new ones. I eliminated the additional check that _hash_getbuf_with_strategy performed, which was blkno == P_NEW. However, since the streaming API already delivers the right buffer, I think it's okay. This change behaves sanely on my by-hand testing. -- Best regards, Kirill Reshke Attachments: [application/octet-stream] v1-0001-Use-stream-read-interface-for-pgstattuple-routine.patch (4.9K, ../../CALdSSPjPGRXHsVhiuDdSNWKrCfJio1mKrjnoOVDmgSN-PM8pCg@mail.gmail.com/2-v1-0001-Use-stream-read-interface-for-pgstattuple-routine.patch) download | inline diff: From 5685f97cf59311922129ba6cd124e85c624846c5 Mon Sep 17 00:00:00 2001 From: reshke kirill <[email protected]> Date: Mon, 25 Nov 2024 18:03:44 +0000 Subject: [PATCH v1] Use stream read interface for pgstattuple routines. Patch implements new streaming read API for pgstattuple contrib extension. This is not perfomance speedup patch. This patch enables this extension to benefit from stream API in the future without introducing any performance improvements. --- contrib/pgstattuple/pgstattuple.c | 55 ++++++++++++++++++------------- 1 file changed, 33 insertions(+), 22 deletions(-) diff --git a/contrib/pgstattuple/pgstattuple.c b/contrib/pgstattuple/pgstattuple.c index 48cb8f59c4f..cea3aa83619 100644 --- a/contrib/pgstattuple/pgstattuple.c +++ b/contrib/pgstattuple/pgstattuple.c @@ -61,22 +61,18 @@ typedef struct pgstattuple_type uint64 free_space; /* free/reusable space in bytes */ } pgstattuple_type; -typedef void (*pgstat_page) (pgstattuple_type *, Relation, BlockNumber, - BufferAccessStrategy); +typedef void (*pgstat_page) (pgstattuple_type *, Relation, Buffer); static Datum build_pgstattuple_type(pgstattuple_type *stat, FunctionCallInfo fcinfo); static Datum pgstat_relation(Relation rel, FunctionCallInfo fcinfo); static Datum pgstat_heap(Relation rel, FunctionCallInfo fcinfo); static void pgstat_btree_page(pgstattuple_type *stat, - Relation rel, BlockNumber blkno, - BufferAccessStrategy bstrategy); + Relation rel, Buffer buf); static void pgstat_hash_page(pgstattuple_type *stat, - Relation rel, BlockNumber blkno, - BufferAccessStrategy bstrategy); + Relation rel, Buffer buf); static void pgstat_gist_page(pgstattuple_type *stat, - Relation rel, BlockNumber blkno, - BufferAccessStrategy bstrategy); + Relation rel, Buffer buf); static Datum pgstat_index(Relation rel, BlockNumber start, pgstat_page pagefn, FunctionCallInfo fcinfo); static void pgstat_index_page(pgstattuple_type *stat, Page page, @@ -405,13 +401,10 @@ pgstat_heap(Relation rel, FunctionCallInfo fcinfo) * pgstat_btree_page -- check tuples in a btree page */ static void -pgstat_btree_page(pgstattuple_type *stat, Relation rel, BlockNumber blkno, - BufferAccessStrategy bstrategy) +pgstat_btree_page(pgstattuple_type *stat, Relation rel, Buffer buf) { - Buffer buf; Page page; - buf = ReadBufferExtended(rel, MAIN_FORKNUM, blkno, RBM_NORMAL, bstrategy); LockBuffer(buf, BT_READ); page = BufferGetPage(buf); @@ -449,13 +442,15 @@ pgstat_btree_page(pgstattuple_type *stat, Relation rel, BlockNumber blkno, * pgstat_hash_page -- check tuples in a hash page */ static void -pgstat_hash_page(pgstattuple_type *stat, Relation rel, BlockNumber blkno, - BufferAccessStrategy bstrategy) +pgstat_hash_page(pgstattuple_type *stat, Relation rel, Buffer buf) { - Buffer buf; Page page; - buf = _hash_getbuf_with_strategy(rel, blkno, HASH_READ, 0, bstrategy); + LockBuffer(buf, HASH_READ); + + /* ref count and lock type are correct */ + + _hash_checkpage(rel, buf, 0); page = BufferGetPage(buf); if (PageGetSpecialSize(page) == MAXALIGN(sizeof(HashPageOpaqueData))) @@ -491,13 +486,10 @@ pgstat_hash_page(pgstattuple_type *stat, Relation rel, BlockNumber blkno, * pgstat_gist_page -- check tuples in a gist page */ static void -pgstat_gist_page(pgstattuple_type *stat, Relation rel, BlockNumber blkno, - BufferAccessStrategy bstrategy) +pgstat_gist_page(pgstattuple_type *stat, Relation rel, Buffer buf) { - Buffer buf; Page page; - buf = ReadBufferExtended(rel, MAIN_FORKNUM, blkno, RBM_NORMAL, bstrategy); LockBuffer(buf, GIST_SHARE); gistcheckpage(rel, buf); page = BufferGetPage(buf); @@ -526,10 +518,22 @@ pgstat_index(Relation rel, BlockNumber start, pgstat_page pagefn, BlockNumber blkno; BufferAccessStrategy bstrategy; pgstattuple_type stat = {0}; + Buffer buf; + BlockRangeReadStreamPrivate p; + ReadStream *stream = NULL; /* prepare access strategy for this index */ bstrategy = GetAccessStrategy(BAS_BULKREAD); + p.current_blocknum = start; + stream = read_stream_begin_relation(READ_STREAM_FULL, + bstrategy, + rel, + MAIN_FORKNUM, + block_range_read_stream_cb, + &p, + 0); + blkno = start; for (;;) { @@ -546,14 +550,21 @@ pgstat_index(Relation rel, BlockNumber start, pgstat_page pagefn, break; } - for (; blkno < nblocks; blkno++) + while(BufferIsValid(buf = read_stream_next_buffer(stream, NULL))) { CHECK_FOR_INTERRUPTS(); - pagefn(&stat, rel, blkno, bstrategy); + pagefn(&stat, rel, buf); } + /* + * After reaching the end we have to reset stream to use it again. + * Extra restart in case of just one iteration does not cost us much. + */ + read_stream_reset(stream); } + read_stream_end(stream); + relation_close(rel, AccessShareLock); return build_pgstattuple_type(&stat, fcinfo); -- 2.34.1 ^ permalink raw reply [nested|flat] 12+ messages in thread
* Re: Use streaming read API in pgstattuple. @ 2024-11-26 10:39 Nazir Bilal Yavuz <[email protected]> parent: Kirill Reshke <[email protected]> 0 siblings, 1 reply; 12+ messages in thread From: Nazir Bilal Yavuz @ 2024-11-26 10:39 UTC (permalink / raw) To: Kirill Reshke <[email protected]>; +Cc: PostgreSQL Hackers <[email protected]> Hi, Thank you for working on this! On Mon, 25 Nov 2024 at 21:17, Kirill Reshke <[email protected]> wrote: > While reviewing other threads implementing stream API for various core > subsystems, I spotted that pgstattuple could also benefit from that. > So, PFA. > > Notice refactoring around pgstat_hash_page and changes in pgstat_page > signature. This is needed because pgstat_hash_tuple uses > _hash_getbuf_with_strategy rather than ReadBufferExtended, which is > OK, but makes adapting the streaming API impossible. So, I changed > this place. Old codes should behave exactly the same way as new ones. > I eliminated the additional check that _hash_getbuf_with_strategy > performed, which was blkno == P_NEW. However, since the streaming API > already delivers the right buffer, I think it's okay. I agree that this looks okay. > This change behaves sanely on my by-hand testing. I encountered some problems while playing with your patch. This query [1] fails when the patch is applied although CI passes, it wasn't failing before. I looked at the code and found that: === 1 blkno = start; for (;;) { /* Get the current relation length */ LockRelationForExtension(rel, ExclusiveLock); nblocks = RelationGetNumberOfBlocks(rel); UnlockRelationForExtension(rel, ExclusiveLock); I think you need to set p.last_exclusive to nblocks here. === 2 - for (; blkno < nblocks; blkno++) + while(BufferIsValid(buf = read_stream_next_buffer(stream, NULL))) { CHECK_FOR_INTERRUPTS(); - pagefn(&stat, rel, blkno, bstrategy); + pagefn(&stat, rel, buf); } blkno doesn't get increased now and this causes an infinite loop. === Also, since this problem couldn't be found by the CI, you may want to add a test for the pgstat_index function. [1] CREATE EXTENSION pgstattuple; create table test (a int primary key, b int[]); create index test_hashidx on test using hash (b); select pgstattuple(oid) from pg_class where relname = 'test_hashidx'; -- Regards, Nazir Bilal Yavuz Microsoft ^ permalink raw reply [nested|flat] 12+ messages in thread
* Re: Use streaming read API in pgstattuple. @ 2024-11-28 06:13 Kirill Reshke <[email protected]> parent: Nazir Bilal Yavuz <[email protected]> 0 siblings, 0 replies; 12+ messages in thread From: Kirill Reshke @ 2024-11-28 06:13 UTC (permalink / raw) To: Nazir Bilal Yavuz <[email protected]>; +Cc: PostgreSQL Hackers <[email protected]> On Tue, 26 Nov 2024 at 15:39, Nazir Bilal Yavuz <[email protected]> wrote: > > Hi, > > Thank you for working on this! > > On Mon, 25 Nov 2024 at 21:17, Kirill Reshke <[email protected]> wrote: > > While reviewing other threads implementing stream API for various core > > subsystems, I spotted that pgstattuple could also benefit from that. > > So, PFA. > > > > Notice refactoring around pgstat_hash_page and changes in pgstat_page > > signature. This is needed because pgstat_hash_tuple uses > > _hash_getbuf_with_strategy rather than ReadBufferExtended, which is > > OK, but makes adapting the streaming API impossible. So, I changed > > this place. Old codes should behave exactly the same way as new ones. > > I eliminated the additional check that _hash_getbuf_with_strategy > > performed, which was blkno == P_NEW. However, since the streaming API > > already delivers the right buffer, I think it's okay. > > I agree that this looks okay. > > > This change behaves sanely on my by-hand testing. > > I encountered some problems while playing with your patch. This query > [1] fails when the patch is applied although CI passes, it wasn't > failing before. I looked at the code and found that: > > === 1 > > blkno = start; > for (;;) > { > /* Get the current relation length */ > LockRelationForExtension(rel, ExclusiveLock); > nblocks = RelationGetNumberOfBlocks(rel); > UnlockRelationForExtension(rel, ExclusiveLock); > > I think you need to set p.last_exclusive to nblocks here. > > === 2 > > - for (; blkno < nblocks; blkno++) > + while(BufferIsValid(buf = read_stream_next_buffer(stream, NULL))) > { > CHECK_FOR_INTERRUPTS(); > > - pagefn(&stat, rel, blkno, bstrategy); > + pagefn(&stat, rel, buf); > } > > blkno doesn't get increased now and this causes an infinite loop. > > === > > Also, since this problem couldn't be found by the CI, you may want to > add a test for the pgstat_index function. > > [1] CREATE EXTENSION pgstattuple; create table test (a int primary > key, b int[]); create index test_hashidx on test using hash (b); > select pgstattuple(oid) from pg_class where relname = 'test_hashidx'; > > -- > Regards, > Nazir Bilal Yavuz > Microsoft Hello! Thank you for taking a peek. Your review comments have been corrected. Since my changes were wrong, I honestly don't know why this worked in version 1. By a miracle. As for CI, i rechecked v1: ``` db2=# select * from pgstathashindex('test_hashidx'); version | bucket_pages | overflow_pages | bitmap_pages | unused_pages | live_items | dead_items | free_percent ---------+--------------+----------------+--------------+--------------+------------+------------+-------------- 4 | 4 | 0 | 1 | 0 | 0 | 0 | 100 (1 row) db2=# select * from pgstattuple('test_hashidx'); ERROR: could not read blocks 6..16 in file "base/16454/16473": read only 0 of 90112 bytes ``` In v2 this behaves correctly. Should we add `pgstattuple(...)` after `pgstat*type*index(..)` everywhere in pgstattuple regression tests? P.S. I want to mention something that I forgot in my initial email: I reused codes from here https://commitfest.postgresql.org/50/5327/ with some adaptation changes. So, Andrey is the author of this patch too. -- Best regards, Kirill Reshke Attachments: [application/octet-stream] v2-0001-Use-stream-read-interface-for-pgstattuple-routine.patch (5.3K, ../../CALdSSPgvSe2gEpBzs9q1h1BHLG0oewyS2+i5M_2cjaQOtfV4hw@mail.gmail.com/2-v2-0001-Use-stream-read-interface-for-pgstattuple-routine.patch) download | inline diff: From 72dbd541e8efdbaa7340df871823da71f3c6e0fa Mon Sep 17 00:00:00 2001 From: reshke kirill <[email protected]> Date: Mon, 25 Nov 2024 18:03:44 +0000 Subject: [PATCH v2] Use stream read interface for pgstattuple routines. Patch implements new streaming read API for pgstattuple contrib extension. This is not perfomance speedup patch. This patch enables this extension to benefit from stream API in the future without introducing any performance improvements. --- contrib/pgstattuple/pgstattuple.c | 64 +++++++++++++++++++------------ 1 file changed, 39 insertions(+), 25 deletions(-) diff --git a/contrib/pgstattuple/pgstattuple.c b/contrib/pgstattuple/pgstattuple.c index 48cb8f59c4f..79b0157b9c0 100644 --- a/contrib/pgstattuple/pgstattuple.c +++ b/contrib/pgstattuple/pgstattuple.c @@ -61,22 +61,18 @@ typedef struct pgstattuple_type uint64 free_space; /* free/reusable space in bytes */ } pgstattuple_type; -typedef void (*pgstat_page) (pgstattuple_type *, Relation, BlockNumber, - BufferAccessStrategy); +typedef void (*pgstat_page) (pgstattuple_type *, Relation, Buffer); static Datum build_pgstattuple_type(pgstattuple_type *stat, FunctionCallInfo fcinfo); static Datum pgstat_relation(Relation rel, FunctionCallInfo fcinfo); static Datum pgstat_heap(Relation rel, FunctionCallInfo fcinfo); static void pgstat_btree_page(pgstattuple_type *stat, - Relation rel, BlockNumber blkno, - BufferAccessStrategy bstrategy); + Relation rel, Buffer buf); static void pgstat_hash_page(pgstattuple_type *stat, - Relation rel, BlockNumber blkno, - BufferAccessStrategy bstrategy); + Relation rel, Buffer buf); static void pgstat_gist_page(pgstattuple_type *stat, - Relation rel, BlockNumber blkno, - BufferAccessStrategy bstrategy); + Relation rel, Buffer buf); static Datum pgstat_index(Relation rel, BlockNumber start, pgstat_page pagefn, FunctionCallInfo fcinfo); static void pgstat_index_page(pgstattuple_type *stat, Page page, @@ -405,13 +401,10 @@ pgstat_heap(Relation rel, FunctionCallInfo fcinfo) * pgstat_btree_page -- check tuples in a btree page */ static void -pgstat_btree_page(pgstattuple_type *stat, Relation rel, BlockNumber blkno, - BufferAccessStrategy bstrategy) +pgstat_btree_page(pgstattuple_type *stat, Relation rel, Buffer buf) { - Buffer buf; Page page; - buf = ReadBufferExtended(rel, MAIN_FORKNUM, blkno, RBM_NORMAL, bstrategy); LockBuffer(buf, BT_READ); page = BufferGetPage(buf); @@ -449,13 +442,15 @@ pgstat_btree_page(pgstattuple_type *stat, Relation rel, BlockNumber blkno, * pgstat_hash_page -- check tuples in a hash page */ static void -pgstat_hash_page(pgstattuple_type *stat, Relation rel, BlockNumber blkno, - BufferAccessStrategy bstrategy) +pgstat_hash_page(pgstattuple_type *stat, Relation rel, Buffer buf) { - Buffer buf; Page page; - buf = _hash_getbuf_with_strategy(rel, blkno, HASH_READ, 0, bstrategy); + LockBuffer(buf, HASH_READ); + + /* ref count and lock type are correct */ + + _hash_checkpage(rel, buf, 0); page = BufferGetPage(buf); if (PageGetSpecialSize(page) == MAXALIGN(sizeof(HashPageOpaqueData))) @@ -491,13 +486,10 @@ pgstat_hash_page(pgstattuple_type *stat, Relation rel, BlockNumber blkno, * pgstat_gist_page -- check tuples in a gist page */ static void -pgstat_gist_page(pgstattuple_type *stat, Relation rel, BlockNumber blkno, - BufferAccessStrategy bstrategy) +pgstat_gist_page(pgstattuple_type *stat, Relation rel, Buffer buf) { - Buffer buf; Page page; - buf = ReadBufferExtended(rel, MAIN_FORKNUM, blkno, RBM_NORMAL, bstrategy); LockBuffer(buf, GIST_SHARE); gistcheckpage(rel, buf); page = BufferGetPage(buf); @@ -523,14 +515,24 @@ pgstat_index(Relation rel, BlockNumber start, pgstat_page pagefn, FunctionCallInfo fcinfo) { BlockNumber nblocks; - BlockNumber blkno; BufferAccessStrategy bstrategy; pgstattuple_type stat = {0}; + Buffer buf; + BlockRangeReadStreamPrivate p; + ReadStream *stream = NULL; /* prepare access strategy for this index */ bstrategy = GetAccessStrategy(BAS_BULKREAD); - blkno = start; + p.current_blocknum = start; + stream = read_stream_begin_relation(READ_STREAM_FULL, + bstrategy, + rel, + MAIN_FORKNUM, + block_range_read_stream_cb, + &p, + 0); + for (;;) { /* Get the current relation length */ @@ -539,21 +541,33 @@ pgstat_index(Relation rel, BlockNumber start, pgstat_page pagefn, UnlockRelationForExtension(rel, ExclusiveLock); /* Quit if we've scanned the whole relation */ - if (blkno >= nblocks) + if (p.current_blocknum >= nblocks) { stat.table_len = (uint64) nblocks * BLCKSZ; break; } - for (; blkno < nblocks; blkno++) + p.last_exclusive = nblocks; + + while (BufferIsValid(buf = read_stream_next_buffer(stream, NULL))) { CHECK_FOR_INTERRUPTS(); - pagefn(&stat, rel, blkno, bstrategy); + pagefn(&stat, rel, buf); } + + Assert(read_stream_next_buffer(stream, NULL) == InvalidBuffer); + + /* + * After reaching the end we have to reset stream to use it again. + * Extra restart in case of just one iteration does not cost us much. + */ + read_stream_reset(stream); } + read_stream_end(stream); + relation_close(rel, AccessShareLock); return build_pgstattuple_type(&stat, fcinfo); -- 2.34.1 ^ permalink raw reply [nested|flat] 12+ messages in thread
end of thread, other threads:[~2024-11-28 06:13 UTC | newest] Thread overview: 12+ messages (download: mbox mbox.gz follow: Atom feed) -- links below jump to the message on this page -- 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 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 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 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 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 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 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 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 v7 2/2] Don't proc_exit() in startup's SIGTERM handler if forked by system(). Nathan Bossart <[email protected]> 2024-11-25 18:17 Use streaming read API in pgstattuple. Kirill Reshke <[email protected]> 2024-11-26 10:39 ` Re: Use streaming read API in pgstattuple. Nazir Bilal Yavuz <[email protected]> 2024-11-28 06:13 ` Re: Use streaming read API in pgstattuple. Kirill Reshke <[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