19.12.2019 4:39, Kyotaro Horiguchi wrote:
> At Wed, 18 Dec 2019 17:09:15 -0500, Tom Lane <tgl@sss.pgh.pa.us> wrote in
>> Alexander Lakhin <exclusion@gmail.com> writes:
>>> 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.
> Agreed for (2).
>
> I'm not sure about the point of the discussion that stat()'ing is
> messy. If it's about performance, does stat()ing only when CreateFile
> returned ERROR_ACCESS_DENIED helps? Anyway we wait for a moment in
> that case.
Please look at the patch that implements (2). It makes `vcregress
recoverycheck` pass (and my demo restart test still passes too).
As open(dir) is getting a little more expensive than before, maybe it's
still worthwhile to patch fsync*(..., isdir,...). I can prepare such a
patch if so.
Best regards,
Alexander