From 5dbab2ccf0d74313dbc2cbaeb418346de8cc2f48 Mon Sep 17 00:00:00 2001 From: Jelte Fennema-Nio Date: Sun, 23 Feb 2025 16:52:29 +0100 Subject: [PATCH v8 1/3] Adds a helper for places that shell out to system() We need to call system() in a few places, and to do so safely we need some pre/post-fork logic. This encapsulates that logic into a single System helper function. The main reason to do this is that in a follow up commit we want to add even more logic pre and post-fork. --- src/backend/access/transam/xlogarchive.c | 29 ++-------------- src/backend/archive/shell_archive.c | 5 +-- src/backend/postmaster/startup.c | 1 + src/backend/storage/file/fd.c | 42 ++++++++++++++++++++++++ src/include/storage/fd.h | 1 + 5 files changed, 48 insertions(+), 30 deletions(-) diff --git a/src/backend/access/transam/xlogarchive.c b/src/backend/access/transam/xlogarchive.c index 1ef1713c91a..c7640ec5025 100644 --- a/src/backend/access/transam/xlogarchive.c +++ b/src/backend/access/transam/xlogarchive.c @@ -158,27 +158,8 @@ RestoreArchivedFile(char *path, const char *xlogfname, (errmsg_internal("executing restore command \"%s\"", xlogRestoreCmd))); - fflush(NULL); - pgstat_report_wait_start(WAIT_EVENT_RESTORE_COMMAND); - - /* - * PreRestoreCommand() informs the SIGTERM handler for the startup process - * that it should proc_exit() right away. This is done for the duration - * of the system() call because there isn't a good way to break out while - * it is executing. Since we might call proc_exit() in a signal handler, - * it is best to put any additional logic before or after the - * PreRestoreCommand()/PostRestoreCommand() section. - */ - PreRestoreCommand(); - - /* - * Copy xlog from archival storage to XLOGDIR - */ - rc = system(xlogRestoreCmd); - - PostRestoreCommand(); - - pgstat_report_wait_end(); + /* Copy xlog from archival storage to XLOGDIR */ + rc = pg_system(xlogRestoreCmd, WAIT_EVENT_RESTORE_COMMAND); pfree(xlogRestoreCmd); if (rc == 0) @@ -325,11 +306,7 @@ ExecuteRecoveryCommand(const char *command, const char *commandName, /* * execute the constructed command */ - fflush(NULL); - pgstat_report_wait_start(wait_event_info); - rc = system(xlogRecoveryCmd); - pgstat_report_wait_end(); - + rc = pg_system(xlogRecoveryCmd, wait_event_info); pfree(xlogRecoveryCmd); if (rc != 0) diff --git a/src/backend/archive/shell_archive.c b/src/backend/archive/shell_archive.c index 828723afe47..64c2c6aa760 100644 --- a/src/backend/archive/shell_archive.c +++ b/src/backend/archive/shell_archive.c @@ -75,10 +75,7 @@ shell_archive_file(ArchiveModuleState *state, const char *file, (errmsg_internal("executing archive command \"%s\"", xlogarchcmd))); - fflush(NULL); - pgstat_report_wait_start(WAIT_EVENT_ARCHIVE_COMMAND); - rc = system(xlogarchcmd); - pgstat_report_wait_end(); + rc = pg_system(xlogarchcmd, WAIT_EVENT_ARCHIVE_COMMAND); if (rc != 0) { diff --git a/src/backend/postmaster/startup.c b/src/backend/postmaster/startup.c index 7149a67fcbc..eb79fda1fd9 100644 --- a/src/backend/postmaster/startup.c +++ b/src/backend/postmaster/startup.c @@ -33,6 +33,7 @@ #include "utils/guc.h" #include "utils/memutils.h" #include "utils/timeout.h" +#include "utils/wait_event_types.h" #ifndef USE_POSTMASTER_DEATH_SIGNAL diff --git a/src/backend/storage/file/fd.c b/src/backend/storage/file/fd.c index 0e8299dd556..718d8079ad7 100644 --- a/src/backend/storage/file/fd.c +++ b/src/backend/storage/file/fd.c @@ -2734,6 +2734,48 @@ OpenTransientFilePerm(const char *fileName, int fileFlags, mode_t fileMode) return -1; /* failure */ } +/* + * A custom wrapper around the system() function that calls the necessary + * functions pre/post-fork. + * + * If WAIT_EVENT_RESTORE_COMMAND is passed as the wait_event_info, it will also + * call the necessary PreRestoreCommand/PostRerstoreCommand functions. It's + * unfortunate that we have to do couple the behaviour of this function so + * tighlty to the restore command logic, but it's the only way to make sure + * that we don't have additional logic inbetween the PreRestoreCommand and + * PostRestoreCommand calls. + */ +int +pg_system(const char *command, uint32 wait_event_info) +{ + int rc; + + fflush(NULL); + pgstat_report_wait_start(wait_event_info); + + if (wait_event_info == WAIT_EVENT_RESTORE_COMMAND) + { + /* + * PreRestoreCommand() informs the SIGTERM handler for the startup + * process that it should proc_exit() right away. This is done for + * the duration of the system() call because there isn't a good way to + * break out while it is executing. Since we might call proc_exit() + * in a signal handler, it is best to put any additional logic before + * or after the PreRestoreCommand()/PostRestoreCommand() section. + */ + PreRestoreCommand(); + } + + rc = system(command); + + if (wait_event_info == WAIT_EVENT_RESTORE_COMMAND) + PostRestoreCommand(); + + pgstat_report_wait_end(); + return rc; +} + + /* * Routines that want to initiate a pipe stream should use OpenPipeStream * rather than plain popen(). This lets fd.c deal with freeing FDs if diff --git a/src/include/storage/fd.h b/src/include/storage/fd.h index b77d8e5e30e..2d445674a1a 100644 --- a/src/include/storage/fd.h +++ b/src/include/storage/fd.h @@ -187,6 +187,7 @@ extern int pg_fsync_writethrough(int fd); extern int pg_fdatasync(int fd); extern bool pg_file_exists(const char *name); extern void pg_flush_data(int fd, off_t offset, off_t nbytes); +extern int pg_system(const char *command, uint32 wait_event_info); extern int pg_truncate(const char *path, off_t length); extern void fsync_fname(const char *fname, bool isdir); extern int fsync_fname_ext(const char *fname, bool isdir, bool ignore_perm, int elevel); base-commit: f09088a01d3372fdfe5da12dd0b2e24989f0caa6 -- 2.43.0