On Thu, Aug 18, 2016 at 7:07 PM, Magnus Hagander <magnus@hagander.net> wrote:
> On Thu, Aug 18, 2016 at 2:25 AM, Michael Paquier <michael.paquier@gmail.com>
> wrote:
>> On Tue, Aug 16, 2016 at 4:56 PM, Magnus Hagander <magnus@hagander.net>
>> wrote:
>> > On Wed, Jul 13, 2016 at 3:10 AM, Michael Paquier
>> > <michael.paquier@gmail.com>
>> > wrote:
> That's pretty much exactly what I said, isn't it? :)
:p
>> lstat() needs a similar treatment, see sendTablespace that calls it.
>> port.h needs also to have its comments updated.
>
> port/win32.h has:
> /*
> * Supplement to <sys/stat.h>.
> */
> #define lstat(path, sb) stat((path), (sb))
>
> So I don't think we should need that, no?
Right, this mapping may be caused by the fact that WIN32 uses junction
points. Is that right?
>> I think that your patch is definitely an improvement, and that we may
>> have better backpatch it: any extension relying on those calls is
>> going to fail similarly, so this gives an escape path. To be honest, I
>> have not thought of GetLastError() and check that with
>> STATUS_DELETE_PENDING, but that's definitely the right call to ignore
>> a file that has this status instead of failing with EACCESS.
>>
>
> So, thinking more about that.
>
> AllocateFile() calls fopen(). That one is only documented to set errno, and
> might not always set the value used by GetLastError().
>
> Perhaps we should make AllocateFile() on win32 specifically check for
> EACCESS, and in case we get that error then we do a GetFileAttributesEx() on
> the same file and see if we get STATUS_DELETE_PENDING or not. If we get
> STATUS_DELETE_PENDING we rewrite EACCESS to ENOENT and return, if we get
> anything else we just let it through.
That's a good idea.
> The race then is if the file "expires", is deleted *and* we create a new
> file in between. In that case, we would still return EACCESS and not open
> the new file. But that seems like a very narrow case.
You mean a file with the same name that gets re-created? Nah, it's not
worth worrying about that. The original complaint here was about a
relfilenode that a checkpoint has removed after it got likely
truncated by a backend and those are unique.
--
Michael