Re: pg_ctl start may return 0 even if the postmaster has been already started on Windows
Michael Paquier <michael@paquier.xyz>
From: Michael Paquier <michael@paquier.xyz>
To: Robert Haas <robertmhaas@gmail.com>
Cc: Kyotaro Horiguchi <horikyota.ntt@gmail.com>, kuroda.hayato@fujitsu.com, shlok.kyal.oss@gmail.com, pgsql-hackers@lists.postgresql.org
Date: 2024-01-09T00:40:23Z
Lists: pgsql-hackers
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
Commits
-
Re-enable autoruns for cmd.exe on Windows
- 74b8e6a69802 18.0 landed
-
Re-enable autoruns for for cmd.exe on Windows
- fa1a63dffcd7 17.0 landed
-
pg_regress: Disable autoruns for cmd.exe on Windows
- e50a52b2b448 12.18 landed
- 2a0eb80ae4ad 13.14 landed
- 4a7475e9a332 14.11 landed
- 7e7d827f57b9 15.6 landed
- 506c77f9b2e5 16.2 landed
- b83747a8a65b 17.0 landed
-
pg_ctl: Disable autoruns for cmd.exe on Windows
- 9e70e6564fc8 12.18 landed
- c8aab699edd6 13.14 landed
- 4c6944223ef4 14.11 landed
- 33d1be06aee0 15.6 landed
- 714bfb7813ed 16.2 landed
- 9886744a361b 17.0 landed