Re: pg_ctl start may return 0 even if the postmaster has been already started on Windows

Thanks for restarting this thread.

At Tue, 9 Jan 2024 09:40:23 +0900, Michael Paquier <michael@paquier.xyz> wrote in 
> On Fri, Jan 05, 2024 at 02:58:55PM -0500, Robert Haas wrote:
> > I'm not a Windows expert, but my guess is that 0001 is a very good
> > idea. I hope someone who is a Windows expert will comment on that.
> 
> I am +1 on 0001.  It is just something we've never anticipated when
> these wrappers around cmd in pg_ctl were written.

Thanks for committing it.

> > 0002 seems problematic to me. One potential issue is that it would
> > break if someone renamed postgres.exe to something else -- although
> > that's probably not really a serious problem.
> 
> We do a find_other_exec_or_die() on "postgres" with what could be a
> custom execution path.  So we're actually sure that the binary will be
> there in the start path, no?  I don't like much the hardcoded
> dependency to .exe here.

The patch doesn't care of the path for postgres.exe. If you are referring to the code you cited below, it's for another reason. I'll describe that there.

> > A bigger issue is that
> > it seems like it would break if someone used pg_ctl to start several
> > instances in different data directories on the same machine. If I'm
> > understanding correctly, that currently mostly works, and this would
> > break it.
> 
> Not having the guarantee that a single shell_pid is associated to a
> single postgres.exe would be a problem.  Now the patch includes this
> code:
> +		if (ppe.th32ParentProcessID == shell_pid &&
> +			strcmp("postgres.exe", ppe.szExeFile) == 0)
> +		{
> +			if (pm_pid != ppe.th32ProcessID && pm_pid != 0)
> +				multiple_children = true;
> +			pm_pid = ppe.th32ProcessID;
> +		}
> 
> Which is basically giving this guarantee?  multiple_children should
> never happen once the autorun part is removed.  Is that right?

The patch indeed ensures the relationship between the parent
pg_ctl.exe and postgres.exe.  However, for some reason, in my Windows
11 environment with the /D option specified, I observed that another
cmd.exe is spawned as the second child process of the parent
cmd.exe. This is why there is a need to verify the executable file
name. I have no idea how the second cmd.exe is being spawned.

> +        * The launcher shell might start other cmd.exe instances or programs
> +        * besides postgres.exe. Veryfying the program file name is essential.
> 
> With the autorun part of cmd.exe removed, what's still relevant here?

Yes, if the strcmp() is commented out, multiple_children sometimes
becomes true..

> s/Veryfying/Verifying/.

Oops!

> Perhaps 0002 should make more efforts in documenting things like
> th32ProcessID and th32ParentProcessID.

Is it correct to understand that you are requesting changes as follows?

--- a/src/bin/pg_ctl/pg_ctl.c
+++ b/src/bin/pg_ctl/pg_ctl.c
@@ -1995,11 +1995,14 @@ pgwin32_find_postmaster_pid(pid_t shell_pid)
 	 *
 	 * Check for duplicate processes to ensure reliability.
 	 *
- 	 * The launcher shell might start other cmd.exe instances or programs
-	 * besides postgres.exe. Verifying the program file name is essential.
-	 *
-	 * The launcher shell process isn't checked in this function.  It will be
-	 * checked by the caller.
+	 * The ppe entry to be examined is identified by th32ParentProcessID, which
+	 * should correspond to the cmd.exe process that executes the postgres.exe
+	 * binary. Additionally, th32ProcessID in the same entry should be the PID
+	 * of the launched postgres.exe. However, even though we have launched the
+	 * parent cmd.exe with the /D option specified, it is sometimes observed
+	 * that another cmd.exe is launched for unknown reasons. Therefore, it is
+	 * crucial to verify the program file name to avoid returning the wrong
+	 * PID.
 	 */


regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center