At Fri, 8 Sep 2023 08:02:57 +0000, "Hayato Kuroda (Fujitsu)"
<[email protected]> 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
