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

From: Kyotaro Horiguchi <horikyota(dot)ntt(at)gmail(dot)com>
To: kuroda(dot)hayato(at)fujitsu(dot)com
Cc: michael(at)paquier(dot)xyz, 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: 2023-09-13 06:52:39
Message-ID: 20230913.155239.174815272687539360.horikyota.ntt@gmail.com
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: pgsql-hackers

At Fri, 8 Sep 2023 08:02:57 +0000, "Hayato Kuroda (Fujitsu)" <kuroda(dot)hayato(at)fujitsu(dot)com> 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

Attachment Content-Type Size
pg_ctl_waits_for_postmasters_pid_on_windows_2.patch text/x-patch 4.0 KB

In response to

Responses

Browse pgsql-hackers by date

  From Date Subject
Next Message Michael Paquier 2023-09-13 07:14:56 Re: persist logical slots to disk during shutdown checkpoint
Previous Message Junwang Zhao 2023-09-13 06:46:30 [dynahash] do not refill the hashkey after hash_search