Re: BUG #16161: pg_ctl stop fails sometimes (on Windows)

At Thu, 19 Dec 2019 15:09:45 -0500, Tom Lane <tgl@sss.pgh.pa.us> wrote in 
> Alexander Lakhin <exclusion@gmail.com> writes:
> > Maybe we should change the condition to 'if (stat(fileName, &st) != 0 &&
> > (err = GetLastError()) == ERROR_ACCESS_DENIED)' to avoid unnecessary
> > sleep with a loop iteration...
> 
> Well, we have to loop back on file-not-found too ...

Agreed. But no need for a sleep in the case and even no need to loop
back when we are opening an existing file, if CreateFile would return
ERROR_ACCESS_DENIED on opening an existing file.

> > It seems that the check for ERROR_DELETE_PENDING was added to
> > pgwin32_safestat() blindly, the issue wasn't reproduced at that time:
> > https://www.postgresql.org/message-id/CAB7nPqRJV6trFta-Qzgi6j2feuYR2ZC%2BKHvWdHnbpDG2scTrxw%40mail.gmail.com
> 
> Hmm, makes one wonder whether that's actually live code.

Even if it is actually dead code, it seems reasonable as it stands
since it is intending to read status of an existing file and the
caller is assumed not to be knowing of ERROR_ACCESS_DENIED. In our
case, pgwin32_open should be conscious of the state so I think calling
pgwin32_safestat does not fit. (Or pgwin32_open and pgwin32_safestat
are on the same level of API.)

Maybe we could just loop back without extra stat'ing. With the
following change, fopen(pidfile, 'r') immediately returns if the file
is being deleted. Postmaster waits for the file gone (it would be
already gone at the first try in most cases). The sleep on
ERROR_ACCESS_DENEID moves from pg_ctl to postmaster in that case,
but I think it doesn't matter.

while (CreateFile())
{
  ...
  if (err == ERROR_ACCESS_DENIED)
  {
    if ((fileFlags & O_CREAT) == 0)
      <retun with ENOENT>

    pg_usleep(...);
    loops++;
    continue;

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center