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.
> 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.
> 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 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?
s/Veryfying/Verifying/.
Perhaps 0002 should make more efforts in documenting things like
th32ProcessID and th32ParentProcessID.
--
Michael