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

Alexander Lakhin <exclusion@gmail.com> writes:
> 18.12.2019 19:03, Tom Lane wrote:
>> Even if fsync_fname_ext() happens to be the only place that does that
>> today (which I doubt) it's going to bite us on the rear regularly in
>> the future, because there's no way to enforce it, and most developers
>> aren't going to notice the problem in testing.

> Regarding fsync_fname_ext(), I thought that it's OK to avoid performing
> a call that is known to fail. And even if someone will try to open() a
> directory, he will need to add the same check for EACCES, otherwise the
> code will fail on Windows. In fact, I see the same pattern in
> pre_sync_fname, fsync_fname in file_utils.c.

My point is that it's totally unreasonable to expect callers to always
know, in advance of accessing the filesystem, whether the pathname they
are going to access is a directory or not.  The point of calling a
filesystem access function is to find out that kind of information.
Yes, there are many cases where we can expect that PG can predict
this, but an open() replacement has no business making such a sweeping
assumption about its use-cases.

>> Is there a way to get the original kernel error code?
>> It'd be a lot better if we could be sure that the condition
>> is STATUS_DELETE_PENDING before looping.

> Unfortunately no, there is no known way  to get the kernel error code.

Bleah.  I did a little bit of googling and couldn't find anything
to contradict that, but it's sure annoying.

> So I see three ways now:
> 1) Revert the pgwin32_open change and choose the previously rejected
> approach: Unlink postmaster.pid using rename operation (adopt the
> solution from https://stackoverflow.com/questions/3764072/).
> 2) Add a check for directory in pgwin32_open, but I think then we're
> getting close to checking there just for postmaster.pid.
> 3) Find out how often we get ERROR_ACCESS_DENIED during all regression
> tests and if such error count is near zero, then add the check for isdir
> in fsync_fname_ext and friends, and get done.

I'm not terribly thrilled with (1) because I do not think that
postmaster.pid accesses are the only place where we have this issue.
And (3) is not only not a forward-looking solution, but it imagines
that the speed of the regression tests is the only thing we need to
worry about here.  We generally don't assume that the regression
tests are an accurate model of the performance users will see.

So that leaves us with (2), it seems.

			regards, tom lane