At Fri, 8 Sep 2023 08:02:57 +0000, "Hayato Kuroda (Fujitsu)" <kuroda.hay...@fujitsu.com> wrote in > > > Ditching cmd.exe seems like a big hassle. So, on the flip side, I > > > tried to identify the postmaster PID using the shell's PID, and it > > > seem to work. The APIs used are avaiable from XP/2003 onwards. > > According to 495ed0ef2, Windows 10 seems the minimal requirement for using > the postgres. So the approach seems OK. > > Followings are my comment, but I can say only cosmetic ones because I do not > have > windows machine which can run postgres.
Thank you for the comment! > 1. > Forward declaration seems missing. In the pg_ctl.c, the static function seems > to > be declared even if there is only one caller (c.f., GetPrivilegesToDelete). Agreed. > 2. > I think the argument should be pid_t. Yeah, I didn't linger on that detail earlier. But revisiting it, I coucur it is best suited since it is a local function in pg_ctl.c. I've now positioned it at the end of a WIN32 section defining other win32-specific functions. Hence, a forward declaration became necessary:p > 3. > I'm not sure the return type of the function should be pid_t or not. According > to the document, DWORD corrresponds to the pid_t. In win32_port.h, the pid_t > is > defiend as int (_MSC_VER seems to be defined when the VisualStduio is used). > It > is harmless, but I perfer to match the interface between caller/callee. IIUC > we > can add just a cast. For the reason previously stated, I've adjusted the type for both the parameter and the return value to pid_t. start_postmaster() already assumed that pid_t is wider than DWORD. I noticed that PID 0 is valid on Windows. However, it is consistently the PID for the system idle process, so it can't be associated with cmd.exe or postgres. I've added a comment noting that premise. Also I did away with an unused variable. For the CreateToolhelp32Snapshot function, I changed the second parameter to 0 from shell_pid, since it is not used when using TH32CS_SNAPPROCESS. I changed the comparison operator for pid_t from > to !=, ensuring correct behavior even with negative values. regards. -- Kyotaro Horiguchi NTT Open Source Software Center
diff --git a/src/bin/pg_ctl/pg_ctl.c b/src/bin/pg_ctl/pg_ctl.c index 807d7023a9..68ab83779c 100644 --- a/src/bin/pg_ctl/pg_ctl.c +++ b/src/bin/pg_ctl/pg_ctl.c @@ -132,6 +132,7 @@ static void adjust_data_dir(void); #ifdef WIN32 #include <versionhelpers.h> +#include <tlhelp32.h> static bool pgwin32_IsInstalled(SC_HANDLE); static char *pgwin32_CommandLine(bool); static void pgwin32_doRegister(void); @@ -142,6 +143,7 @@ static void WINAPI pgwin32_ServiceMain(DWORD, LPTSTR *); static void pgwin32_doRunAsService(void); static int CreateRestrictedProcess(char *cmd, PROCESS_INFORMATION *processInfo, bool as_service); static PTOKEN_PRIVILEGES GetPrivilegesToDelete(HANDLE hToken); +static pid_t pgwin32_find_postmaster_pid(pid_t shell_pid); #endif static pid_t get_pgpid(bool is_status_request); @@ -609,7 +611,11 @@ wait_for_postmaster_start(pid_t pm_pid, bool do_checkpoint) /* File is complete enough for us, parse it */ pid_t pmpid; time_t pmstart; - +#ifndef WIN32 + pid_t wait_pid = pm_pid; +#else + pid_t wait_pid = pgwin32_find_postmaster_pid(pm_pid); +#endif /* * Make sanity checks. If it's for the wrong PID, or the recorded * start time is before pg_ctl started, then either we are looking @@ -619,14 +625,8 @@ wait_for_postmaster_start(pid_t pm_pid, bool do_checkpoint) */ pmpid = atol(optlines[LOCK_FILE_LINE_PID - 1]); pmstart = atol(optlines[LOCK_FILE_LINE_START_TIME - 1]); - if (pmstart >= start_time - 2 && -#ifndef WIN32 - pmpid == pm_pid -#else - /* Windows can only reject standalone-backend PIDs */ - pmpid > 0 -#endif - ) + + if (pmstart >= start_time - 2 && pmpid == wait_pid) { /* * OK, seems to be a valid pidfile from our child. Check the @@ -1950,6 +1950,90 @@ GetPrivilegesToDelete(HANDLE hToken) return tokenPrivs; } + +/* + * Find the PID of the launched postmaster. + * + * On Windows, the cmd.exe doesn't support the exec command. As a result, we + * don't directly get the postmaster's PID. This function identifies the PID of + * the postmaster started by the child cmd.exe. + * + * Returns the postmaster's PID. If the shell is alive but the postmaster is + * missing, returns 0. Otherwise terminates this command with an error. + * + * This function uses PID 0 as an invalid value, assuming the system idle + * process occupies it and it won't be a PID for a shell or postmaster. + */ +pid_t +pgwin32_find_postmaster_pid(pid_t shell_pid) +{ + HANDLE hSnapshot; + DWORD flags; + PROCESSENTRY32 ppe; + pid_t pm_pid = 0; /* abusing 0 as an invalid value */ + bool shell_exists = false; + bool multiple_children = false; + DWORD last_error; + + /* create a process snapshot */ + hSnapshot = CreateToolhelp32Snapshot(TH32CS_SNAPPROCESS, 0); + if (hSnapshot == INVALID_HANDLE_VALUE) + { + write_stderr(_("%s: CreateToolhelp32Snapshot failed\n"), + progname); + return; + } + + /* start iterating on the snapshot */ + ppe.dwSize = sizeof(PROCESSENTRY32); + if (!Process32First(hSnapshot, &ppe)) + { + write_stderr(_("%s: Process32First failed: errcode=%08x\n"), + progname, GetLastError()); + exit(1); + } + + /* iterate over the snapshot */ + do + { + if (ppe.th32ProcessID == shell_pid) + shell_exists = true; + else if (ppe.th32ParentProcessID == shell_pid) + { + if (pm_pid != 0) + multiple_children = true; + pm_pid = ppe.th32ProcessID; + } + } + while (Process32Next(hSnapshot, &ppe)); + + /* avoid multiple calls primary for clarity, not out of necessity */ + last_error = GetLastError(); + if (last_error != ERROR_NO_MORE_FILES) + { + write_stderr(_("%s: Process32Next failed: errcode=%08x\n"), + progname, last_error); + exit(1); + } + CloseHandle(hSnapshot); + + /* assuming the launching shell executes a single process */ + if (multiple_children) + { + write_stderr(_("%s: launcher shell executed multiple processes\n"), + progname); + exit(1); + } + + /* check if the process is still alive */ + if (!shell_exists) + { + write_stderr(_("%s: launcher shell died\n"), progname); + exit(1); + } + + return pm_pid; +} #endif /* WIN32 */ static void