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

From: Michael Paquier <michael(at)paquier(dot)xyz>
To: Robert Haas <robertmhaas(at)gmail(dot)com>
Cc: Kyotaro Horiguchi <horikyota(dot)ntt(at)gmail(dot)com>, kuroda(dot)hayato(at)fujitsu(dot)com, shlok(dot)kyal(dot)oss(at)gmail(dot)com, pgsql-hackers(at)lists(dot)postgresql(dot)org
Subject: Re: pg_ctl start may return 0 even if the postmaster has been already started on Windows
Date: 2024-01-09 00:40:23
Message-ID: ZZyV99g76Q3-YwEn@paquier.xyz
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
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

In response to

Responses

Browse pgsql-hackers by date

  From Date Subject
Next Message Dian Fay 2024-01-09 00:51:58 Re: add function argument names to regex* functions.
Previous Message Michael Paquier 2024-01-09 00:14:42 Re: Add support for __attribute__((returns_nonnull))