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

On Thu, Sep 07, 2023 at 07:07:36AM +0000, Hayato Kuroda (Fujitsu) wrote:

# Problem

The "pg_ctl start" command returns 0 (succeeded) even if the cluster has
already been started. This occurs on Windows environment, and when the command
is executed just after postmaster starts.

Not failing on `pg_ctl start` if the command is run on a data folder
that has already been started previously by a different command with a
postmaster still alive feels like cheating, because pg_ctl is lying
about its result. If pg_ctl wants to start a cluster but is not able
to do it, either because the postmaster failed at startup or because
the cluster has already started, it should report a failure. Now, I
also recall that the processes spawned by pg_ctl on Windows make the
status handling rather tricky to reason about..
--
Michael

Dear Michael,

Thank you for replying!

Not failing on `pg_ctl start` if the command is run on a data folder
that has already been started previously by a different command with a
postmaster still alive feels like cheating, because pg_ctl is lying
about its result. If pg_ctl wants to start a cluster but is not able
to do it, either because the postmaster failed at startup or because
the cluster has already started, it should report a failure.

I have a same feelings as you. Users may use the return code in their batch file
and they may decide what to do based on the wrong status. Reporting the status
more accurately is nice.

My first idea is that to move the checking part to above, but this may not handle
the case the postmaster is still alive (now sure this is real issue). Do we have to
add a new indicator which ensures the identity of processes for windows?
Please tell me how you feel.

Now, I
also recall that the processes spawned by pg_ctl on Windows make the
status handling rather tricky to reason about..

Did you say about the below comment? Currently I have no idea to make
codes more proper, sorry.

```
* On Windows, we may be checking the postmaster's parent shell, but
* that's fine for this purpose.
```

Best Regards,
Hayato Kuroda
FUJITSU LIMITED

At Thu, 7 Sep 2023 10:53:41 +0000, "Hayato Kuroda (Fujitsu)" <kuroda.hayato@fujitsu.com> wrote in

My first idea is that to move the checking part to above, but this may not handle
the case the postmaster is still alive (now sure this is real issue). Do we have to
add a new indicator which ensures the identity of processes for windows?
Please tell me how you feel.

It doesn't seem to work as expected. We still lose the relationship
between the PID file and the launched postmaster.

Now, I
also recall that the processes spawned by pg_ctl on Windows make the
status handling rather tricky to reason about..

Did you say about the below comment? Currently I have no idea to make
codes more proper, sorry.

```
* On Windows, we may be checking the postmaster's parent shell, but
* that's fine for this purpose.
```

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.

regards.

--
Kyotaro Horiguchi
NTT Open Source Software Center

At Fri, 08 Sep 2023 14:17:16 +0900 (JST), Kyotaro Horiguchi <horikyota.ntt@gmail.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.

Cleaned it up a bit.

regards.

--
Kyotaro Horiguchi
NTT Open Source Software Center

Dear Hoiguchi-san,

Thank you for making the patch!

It doesn't seem to work as expected. We still lose the relationship
between the PID file and the launched postmaster.

Yes, I did not expect that the relationship can be kept.
Conceptually +1 for your approach.

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.

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).

2.
I think the argument should be pid_t.

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.

```
#ifdef _MSC_VER
typedef int pid_t;
#endif
```

Best Regards,
Hayato Kuroda
FUJITSU LIMITED

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

Dear Horiguchi-san,

I have tested your patch on my CI, but several test could not patch with error:
"pg_ctl: launcher shell executed multiple processes".

I added the thread to next CF entry, so let's see the how cfbot says.

[1]: https://commitfest.postgresql.org/45/4573/

Best Regards,
Hayato Kuroda
FUJITSU LIMITED

Dear Horiguchi-san,

I added the thread to next CF entry, so let's see the how cfbot says.

At least there are several compiler warnings. E.g.,

* pgwin32_find_postmaster_pid() has "return;", but IIUC it should be "exit(1)"
* When DWORD is printed, "%lx" should be used.
* The variable "flags" seems not needed.

Here is a patch which suppresses warnings, whereas test would fail...
You can use it if acceptable.

Best Regards,
Hayato Kuroda
FUJITSU LIMITED

At Tue, 19 Sep 2023 13:48:55 +0000, "Hayato Kuroda (Fujitsu)" <kuroda.hayato@fujitsu.com> wrote in

Dear Horiguchi-san,

I added the thread to next CF entry, so let's see the how cfbot says.

At least there are several compiler warnings. E.g.,

* pgwin32_find_postmaster_pid() has "return;", but IIUC it should be "exit(1)"
* When DWORD is printed, "%lx" should be used.
* The variable "flags" seems not needed.

Yeah, I thought that they all have been fixed but.. you are right in
every respect.

Here is a patch which suppresses warnings, whereas test would fail...
You can use it if acceptable.

I was able to see the trouble in the CI environment, but not
locally. I'll delve deeper into this. Thanks you for bringing it to my
attention.

regards.

--
Kyotaro Horiguchi
NTT Open Source Software Center

At Wed, 20 Sep 2023 14:18:41 +0900 (JST), Kyotaro Horiguchi <horikyota.ntt@gmail.com> wrote in

I was able to see the trouble in the CI environment, but not
locally. I'll delve deeper into this. Thanks you for bringing it to my
attention.

I found two instances with multiple child processes.

# child-pid / parent-pid / given-pid : exec name
process parent PID child PID target PID exec file
shell 1228 6472 1228 cmd.exe
child 5184 1228 1228 cmd.exe
child 6956 1228 1228 postgres.exe

launcher shell executed multiple processes

process parent PID child PID target PID exec file
shell 4296 5880 4296 cmd.exe
child 5156 4296 4296 agent.exe
child 5640 4296 4296 postgres.exe

launcher shell executed multiple processes

It looks like the environment has autorun setups for cmd.exe. There's
another known issue related to auto-launching chcp at
startup. Ideally, we would avoid such behavior in the
postmaster-launcher shell. I think we should add "/D" flag to cmd.exe
command line, perhaps in a separate patch.

Even after making that change, I still see something being launched from the launcher cmd.exe...

process parent PID child PID target PID exec file
shell 2784 6668 2784 cmd.exe
child 6140 2784 2784 MicrosoftEdgeUpdate.exe
child 6260 2784 2784 postgres.exe

launcher shell executed multiple processes

I'm not sure what triggers this; perhaps some other kind of hooks? If
we cannot avoid this behavior, we'll have to verify the executable
file name. It should be fine, given that the file name is constant,
but I'm not fully convinced that this is the ideal solution.

Another issue is.. that I haven't been able to cause the false
positive of pg_ctl start.. Do you have a concise reproducer of the
issue?

regards.

--
Kyotaro Horiguchi
NTT Open Source Software Center

Dear Horiguchi-san,

Thank you for making a patch! They can pass ci.
I'm still not sure what should be, but I can respond a part.

Another issue is.. that I haven't been able to cause the false
positive of pg_ctl start.. Do you have a concise reproducer of the
issue?

I found a short sleep in pg_ctl/t/001_start_stop.pl. This was introduced in
6bcce2580 to ensure waiting more than 2 seconds. I've tested on my CI and
found that removing the sleep can trigger the failure. Also, I confirmed your patch
fixes the problem. PSA the small patch for cfbot. 0001 and 0002 were not changed.

Best Regards,
Hayato Kuroda
FUJITSU LIMITED

On Fri, 6 Oct 2023 at 11:38, Hayato Kuroda (Fujitsu)
<kuroda.hayato@fujitsu.com> wrote:

Dear Horiguchi-san,

Thank you for making a patch! They can pass ci.
I'm still not sure what should be, but I can respond a part.

Another issue is.. that I haven't been able to cause the false
positive of pg_ctl start.. Do you have a concise reproducer of the
issue?

I found a short sleep in pg_ctl/t/001_start_stop.pl. This was introduced in
6bcce2580 to ensure waiting more than 2 seconds. I've tested on my CI and
found that removing the sleep can trigger the failure. Also, I confirmed your patch
fixes the problem. PSA the small patch for cfbot. 0001 and 0002 were not changed.

I have tested the patches on my windows setup.
I am trying to start two postgres servers with an interval of 5 secs.

with HEAD (when same server is started after an interval of 5 secs):
D:\project\pg\bin>pg_ctl -D ../data -l data2.log start
pg_ctl: another server might be running; trying to start server anyway
waiting for server to start.... stopped waiting
pg_ctl: could not start server
Examine the log output.

with Patch:(when same server is started after an interval of 5 secs)
D:\project\pg_dev\bin>pg_ctl -D ../data -l data2.log start
pg_ctl: another server might be running; trying to start server anyway
waiting for server to start....pg_ctl: launcher shell died

The output message after patch is different from the HEAD. I felt that
with patch as well we should get the message "pg_ctl: could not start
server".
Is this message change intentional?

Thanks,
Shlok Kumar Kyal

Thank you for testing this!

At Fri, 6 Oct 2023 12:28:32 +0530, Shlok Kyal <shlok.kyal.oss@gmail.com> wrote i> D:\project\pg_dev\bin>pg_ctl -D ../data -l data2.log start

pg_ctl: another server might be running; trying to start server anyway
waiting for server to start....pg_ctl: launcher shell died

The output message after patch is different from the HEAD. I felt that
with patch as well we should get the message "pg_ctl: could not start
server".
Is this message change intentional?

Partly no, partly yes. My focus was on verifying the accuracy of
identifying the actual postmaster PID on Windows. The current patch
provides a detailed description of the events, primarily because I
lack a comprehensive understanding of both the behavior of Windows
APIs and the associated processes. Given that context, the messages
essentially serve debugging purposes.

I agree with your suggestion. Ultimately, if there's a possibility
for this to be committed, the message will be consolidated to "could
not start server".

regards.

--
Kyotaro Horiguchi
NTT Open Source Software Center

Dear Horiguchi-san, Shlok,

At Fri, 6 Oct 2023 12:28:32 +0530, Shlok Kyal <shlok.kyal.oss@gmail.com> wrote
i> D:\project\pg_dev\bin>pg_ctl -D ../data -l data2.log start

pg_ctl: another server might be running; trying to start server anyway
waiting for server to start....pg_ctl: launcher shell died

The output message after patch is different from the HEAD. I felt that
with patch as well we should get the message "pg_ctl: could not start
server".
Is this message change intentional?

Partly no, partly yes. My focus was on verifying the accuracy of
identifying the actual postmaster PID on Windows. The current patch
provides a detailed description of the events, primarily because I
lack a comprehensive understanding of both the behavior of Windows
APIs and the associated processes. Given that context, the messages
essentially serve debugging purposes.

I agree with your suggestion. Ultimately, if there's a possibility
for this to be committed, the message will be consolidated to "could
not start server".

Based on the suggestion, I tried to update the patch.
A new argument is_valid is added for reporting callee. Also, reporting formats
are adjusted based on other functions. How do you think?

Best Regards,
Hayato Kuroda
FUJITSU LIMITED

At Mon, 23 Oct 2023 08:57:19 +0000, "Hayato Kuroda (Fujitsu)" <kuroda.hayato@fujitsu.com> wrote in

I agree with your suggestion. Ultimately, if there's a possibility
for this to be committed, the message will be consolidated to "could
not start server".

Based on the suggestion, I tried to update the patch.
A new argument is_valid is added for reporting callee. Also, reporting formats
are adjusted based on other functions. How do you think?

An equivalent check is already done shortly afterward in the calling
function. Therefore, we can simply remove the code path for "launcher
shell died", and it will work the same way. Please find the attached.

Other error cases will fit to "shouldn't occur under normal
conditions" errors.

There is a possibility that the launcher shell terminates while
postmaster is running. Even in such a case, the server continue
working without any problems. I contemplated accomodating this case
but the effort required seemed disproportionate to the possibility.

regards.

--
Kyotaro Horiguchi
NTT Open Source Software Center

Dear Horiguchi-san,

Thanks for updates! I was quite not sure the Windows env, but I can post comments.
(We need reviews by windows-friendly developers...)

Other error cases will fit to "shouldn't occur under normal
conditions" errors.

Formatting of messages for write_stderr() seem different from others. In v3,
I slightly modified for readability like below. I wanted to let you know just in case
because you did not say anything about these changes...

```
+	/* create a process snapshot */
+	hSnapshot = CreateToolhelp32Snapshot(TH32CS_SNAPPROCESS, 0);
+	if (hSnapshot == INVALID_HANDLE_VALUE)
+	{
+		write_stderr(_("%s: could not create a snapshot: error code %lu\n"),
+					 progname, (unsigned long) GetLastError());
+		exit(1);
+	}
+
+	/* start iterating on the snapshot */
+	ppe.dwSize = sizeof(PROCESSENTRY32);
+	if (!Process32First(hSnapshot, &ppe))
+	{
+		write_stderr(_("%s: cound not retrieve information about the process: error code %lu\n"),
+					 progname, GetLastError());
+		exit(1);
+	}
+
```

Best Regards,
Hayato Kuroda
FUJITSU LIMITED

At Tue, 24 Oct 2023 07:37:22 +0000, "Hayato Kuroda (Fujitsu)" <kuroda.hayato@fujitsu.com> wrote in

Dear Horiguchi-san,

Thanks for updates! I was quite not sure the Windows env, but I can post comments.
(We need reviews by windows-friendly developers...)

Indeed, I haven't managed to successfully build using Meson on
Windows...

Formatting of messages for write_stderr() seem different from others. In v3,
I slightly modified for readability like below. I wanted to let you know just in case
because you did not say anything about these changes...

Ah. Sorry, I was lazy about the messages because I didn't regard this
to be at that stage yet.

In the attached, fixed the existing two messages, and adjusted one
message to display an error code, all in the consistent format.

Thanks!

--
Kyotaro Horiguchi
NTT Open Source Software Center

On Tue, Oct 24, 2023 at 4:28 AM Kyotaro Horiguchi
<horikyota.ntt@gmail.com> wrote:

In the attached, fixed the existing two messages, and adjusted one
message to display an error code, all in the consistent format.

Hi,

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.

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. 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.

--
Robert Haas
EDB: http://www.enterprisedb.com

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