Date: Sun, 8 Sep 2024 09:53:55 -0700
From: Noah Misch <noah@leadboat.com>
To: Alexander Lakhin <exclusion@gmail.com>
Cc: pgsql-hackers <pgsql-hackers@postgresql.org>
Subject: Re: Yet another way for pg_ctl stop to fail on Windows
Message-ID: <20240908165355.93.nmisch@google.com>
References: <ba498e06-d22a-5bfb-389b-031f8db62d7d@gmail.com>
 <20240907181143.11.nmisch@google.com>
 <da4f9ec1-a272-bd2d-4ca1-6eb58d6dc79a@gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=iso-8859-1
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <da4f9ec1-a272-bd2d-4ca1-6eb58d6dc79a@gmail.com>
User-Agent: Mutt/2.2.12 (2023-09-09)
Archived-At: <https://www.postgresql.org/message-id/20240908165355.93.nmisch%40google.com>
Precedence: bulk

On Sun, Sep 08, 2024 at 06:00:00PM +0300, Alexander Lakhin wrote:
> 07.09.2024 21:11, Noah Misch wrote:

> > > Noah, what do you think of handling this error in line with handling of
> > > ERROR_BROKEN_PIPE and ERROR_BAD_PIPE (which was done in 0ea1f2a3a)?
> > > 
> > > I tried the following change:
> > >          switch (GetLastError())
> > >          {
> > >                  case ERROR_BROKEN_PIPE:
> > >                  case ERROR_BAD_PIPE:
> > > +               case ERROR_PIPE_BUSY:
> > > and saw no issues.
> > That would be a strict improvement over returning EINVAL like we do today.  We
> > do use PIPE_UNLIMITED_INSTANCES, so I expect the causes of ERROR_PIPE_BUSY are
> > process exit and ENOMEM-like situations.  While that change is the best thing
> > if the process is exiting, it could silently drop the signal in ENOMEM-like
> > situations.  Consider the following alternative.  If sig==0, just return 0
> > like you propose, because the process isn't completely gone.  Otherwise, sleep
> > and retry the signal, like pgwin32_open_handle() retries after certain errors.
> > What do you think of that?

> I agree with your approach. It looks like Microsoft recommends to loop on
> ERROR_PIPE_BUSY: [1] (they say "Calling CallNamedPipe is equivalent to
> calling the CreateFile ..." at [2]).

I see Microsoft suggests WaitNamedPipeA() as opposed to just polling.
WaitNamedPipeA() should be more responsive.  Given how rare this has been, it
likely doesn't matter whether we use WaitNamedPipeA() or polling.  I'd lean
toward whichever makes the code simpler, probably polling.

> So if we aim to not only fix "pg_ctl stop", but to make pgkill() robust,
> it's the way to go, IMHO. I'm not sure about an infinite loop they show,
> I'd vote for a loop with the same characteristics as in
> pgwin32_open_handle().

I agree with bounding the total time of each kill(), like
pgwin32_open_handle() does for open().

> [1] https://learn.microsoft.com/en-us/windows/win32/ipc/named-pipe-client
> [2] https://learn.microsoft.com/en-us/windows/win32/api/winbase/nf-winbase-callnamedpipea