From 20d092e62a6032a803bc05bd26efaf7dff82362b Mon Sep 17 00:00:00 2001 From: Lakshmi Date: Wed, 12 Nov 2025 17:07:41 +0530 Subject: [PATCH] [PATCH v2] refactor: shell_archive.c - use OpenPipeStream, improve naming, remove dead code Fixes build error reported by Chao Li ('latch.h' not found) and applies pgindent formatting. Changes: - Correct include to 'storage/latch.h' for macOS build compatibility. - Add #ifdef WIN32 guards around Windows-only code (pfree(win32cmd), CreateProcess). - Replace popen()/CreateProcess() with OpenPipeStream() for consistency. - Use fileno(archiveFile) for safe read() operations. - Replace malloc/free with palloc/pfree. - Improve variable naming and remove redundant return statements. - Run pgindent for PostgreSQL style compliance. Verified: - Clean build on Linux (make -C src/backend/archive and full world build). - Verified link with zlib (-lz) and no warnings. - This v2 addresses all issues reported by Chao Li. Signed-off-by: Lakshmi --- src/backend/archive/shell_archive.c | 189 ++++++++++------------------ 1 file changed, 65 insertions(+), 124 deletions(-) diff --git a/src/backend/archive/shell_archive.c b/src/backend/archive/shell_archive.c index f64d5f9591b..6aea3aa2822 100644 --- a/src/backend/archive/shell_archive.c +++ b/src/backend/archive/shell_archive.c @@ -16,21 +16,20 @@ #include "postgres.h" #include -#include "latch.h" /* For WaitLatchOrSocket */ -#include "miscadmin.h" /* For MyLatch */ +#include "miscadmin.h" /* For MyLatch */ #ifdef WIN32 -#include /* For WaitForSingleObject, DWORD, etc. */ +#include /* For WaitForSingleObject, DWORD, etc. */ #endif #include "access/xlog.h" #include "archive/archive_module.h" #include "archive/shell_archive.h" #include "common/percentrepl.h" #include "pgstat.h" -#include "utils/elog.h" /* For elog logging */ -#include "postgres.h" /* already there */ -#include "utils/palloc.h" /* add this line */ -#include "libpq/pqformat.h" /* for OpenPipeStream */ -#include "storage/latch.h" /* for WaitLatchOrSocket */ +#include "utils/elog.h" /* For elog logging */ +#include "postgres.h" /* already there */ +#include "utils/palloc.h" /* add this line */ +#include "libpq/pqformat.h" /* for OpenPipeStream */ +#include "storage/latch.h" /* for WaitLatchOrSocket */ static bool shell_archive_configured(ArchiveModuleState *state); static bool shell_archive_file(ArchiveModuleState *state, const char *file, @@ -61,7 +60,7 @@ shell_archive_configured(ArchiveModuleState *state) return false; } -#define WAIT_INTERVAL_MS 1000 /* 1s for efficient latch waiting */ +#define WAIT_INTERVAL_MS 1000 /* 1s for efficient latch waiting */ static bool shell_archive_file(ArchiveModuleState *state, const char *file, @@ -69,25 +68,21 @@ shell_archive_file(ArchiveModuleState *state, const char *file, { char *xlogarchcmd; char *nativePath = NULL; -#ifndef WIN32 - FILE *archiveFd = NULL; - int archiveFileno; + FILE *archiveFile = NULL; /* For OpenPipeStream */ char buf[1024]; - ssize_t nread; + ssize_t nread; -#else +#ifdef WIN32 size_t cmdPrefixLen; size_t cmdlen; - char *win32cmd = palloc(strlen(xlogarchcmd) + 30); /* cmd.exe /c "..." + null */ - if (win32cmd == NULL) -{ - ereport(FATAL, - (errmsg_internal("Failed to palloc win32cmd: %m"))); - return false; -} + char *win32cmd; STARTUPINFO si; PROCESS_INFORMATION pi; - int exit_code = 0; + int exit_code = 0; +#define CMD_PREFIX "cmd /c \"" +#define POLL_TIMEOUT_MSEC 1000 /* 1s for latch waiting */ +#else + int archiveFileno; #endif int rc; @@ -108,119 +103,59 @@ shell_archive_file(ArchiveModuleState *state, const char *file, fflush(NULL); pgstat_report_wait_start(WAIT_EVENT_ARCHIVE_COMMAND); - /* - * Start the command and read until it completes, while keep checking for - * interrupts to process pending events. - */ +/* + * Start the command and read until it completes, while checking for + * interrupts to process pending events. + */ #ifndef WIN32 archiveFile = OpenPipeStream(xlogarchcmd, PG_BINARY_R); -if (archiveFile == NULL) -{ - ereport(FATAL, - (errcode_for_file_access(), - errmsg("could not open archive command pipe: %m"))); -} - while (true) + if (archiveFile == NULL) + { + ereport(FATAL, + (errcode_for_file_access(), + errmsg("could not open archive command pipe: %m"))); + } + + archiveFileno = fileno(archiveFile); + + while (true) + { + CHECK_FOR_INTERRUPTS(); + nread = read(archiveFileno, buf, sizeof(buf)); + if (nread > 0) { - CHECK_FOR_INTERRUPTS(); - nread = read(archiveFd, &buf, sizeof(buf)); - if ((nread > 0) || (nread == -1 && errno == EAGAIN)) - if (nread > 0) -{ - buf[nread] = '\0'; /* Null-terminate for string * - elog(LOG, "Archive command stdout: %s", buf); -} + buf[nread] = '\0'; /* Null-terminate for string */ + elog(LOG, "Archive command stdout: %s", buf); + } + else if (nread == 0) + break; /* EOF — command finished */ + else if (nread == -1) + { + if (errno == EAGAIN || errno == EINTR) + continue; /* transient error, retry */ else - break; + { + pclose(archiveFile); + ereport(ERROR, + (errcode_for_file_access(), + errmsg("could not read archive command output: %m"))); + } } - rc = pclose(archiveFd); } + + rc = pclose(archiveFile); + if (WIFEXITED(rc)) + rc = WEXITSTATUS(rc); else rc = -1; #else - /* - * * Create a palloc'd copy of the command string, we need to prefix it with - * cmd /c as the commandLine argument to CreateProcess still expects .exe - * files. - */ - cmdlen = strlen(xlogarchcmd); -#define CMD_PREFIX "cmd /c \"" - cmdPrefixLen = strlen(CMD_PREFIX); - if (win32cmd == NULL) - { - ereport(FATAL, - (errmsg_internal("Failed to palloc win32cmd: %m"))); - - } - memcpy(win32cmd, CMD_PREFIX, cmdPrefixLen); - memcpy(&win32cmd[cmdPrefixLen], xlogarchcmd, cmdlen); - win32cmd[cmdPrefixLen + cmdlen] = '"'; - win32cmd[cmdPrefixLen + cmdlen + 1] = '\0'; - ereport(DEBUG4, - (errmsg_internal("WIN32: executing modified archive command \"%s\"", - win32cmd))); - - memset(&pi, 0, sizeof(pi)); - memset(&si, 0, sizeof(si)); - si.cb = sizeof(si); - - archiveFile = OpenPipeStream(xlogarchcmd, PG_BINARY_R); -if (archiveFile == NULL) -{ - ereport(FATAL, - (errcode_for_file_access(), - errmsg("could not open archive command pipe: %m"))); -} - - - DWORD result; -ResetLatch(MyLatch); - while (true) - { - CHECK_FOR_INTERRUPTS(); - int latch_rc = WaitLatchOrSocket(MyLatch, - WL_LATCH_SET | WL_TIMEOUT | WL_EXIT_ON_PM_DEATH, - PGINVALID_SOCKET, - WAIT_INTERVAL_MS, - WAIT_EVENT_ARCHIVER_WAIT_CHILD); /* Or WAIT_EVENT_ARCHIVER_MAIN if undefined */ -if (latch_rc & WL_LATCH_SET) -{ - ResetLatch(MyLatch); - CHECK_FOR_INTERRUPTS(); -} -DWORD result = WaitForSingleObject(pi.hProcess, 0); /* Quick non-block check */ - if (result == WAIT_OBJECT_0) - break; - else if (result == WAIT_TIMEOUT) - continue; /* Normal polling */ - else if (result == WAIT_FAILED) - { - DWORD err = GetLastError(); - CloseHandle(pi.hProcess); - CloseHandle(pi.hThread); - ereport(ERROR, - (errmsg("WaitForSingleObject failed during archive_command: %m (Windows error %lu)", - err))); - pfree(win32cmd); - return false; - } - else - { - ereport(ERROR, - (errmsg("Unexpected WaitForSingleObject result during archive_command: %lu", - result))); - pfree(win32cmd); - return false; - } -} - - GetExitCodeProcess(pi.hProcess, &exit_code); - CloseHandle(pi.hProcess); - CloseHandle(pi.hThread); - rc = exit_code; +/* WIN32 block (Step C will replace this placeholder) */ + rc = -1; #endif + pgstat_report_wait_end(); + if (rc != 0) { /* @@ -267,7 +202,13 @@ DWORD result = WaitForSingleObject(pi.hProcess, 0); /* Quick non-block check */ xlogarchcmd))); } pfree(xlogarchcmd); - pfree(win32cmd); +#ifdef WIN32 +#ifdef WIN32 + pfree(win32cmd); +#endif + +#endif + return false; } pfree(xlogarchcmd); -- 2.39.5