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:
>>
>> On Wed, Jul 13, 2016 at 10:06 AM, Michael Paquier
>> <michael.paquier@gmail.com> wrote:
>> > Checking directly for STATUS_DELETE_PENDING would be the way to go,
>> > likely with tweaks in basebackup.c. but like Takatsuka-san, I cannot
>> > find anything around that would allow us to check for that.
>> >
>> > [1]:
>> >
http://stackoverflow.com/questions/6680491/why-does-windows-return-error-access-denied-when-i-try-to-open-a-delete-pended-f
>>
>> Actually we may want to tweak stat() to not complain for a file with
>> such a state.
>>
>
> That will still leave is with a race condition won't it? It'll just be much
> smaller? The file could go into STATUS_DELETE_PENDING between the call to
> stat() and the call to sendFile()/allocateFile().
That would only reduce the window. If the file is marked as ready for
deletion after checking with it for stat() and scanned afterwards
we're out, we'd still get a similar error afterwards.
> Maybe that's an acceptable difference? If not, we also need to teach
> AllocateFile() about the error.
>
> If we're happy with just stat, it looks like we will have to add it to
> pgwin32_safestat().
lstat() needs a similar treatment, see sendTablespace that calls it.
port.h needs also to have its comments updated.
> I don' t have a working window sbox for testing ATM (yeah yeah, it's on my
> magic todo), but can someone confirm if calling stat() on such a deleted
> flie sets the errorcode so it can be checked with GetLastError() *as well*
> as setting errno? IIRC those functions do, but it needs to be checked. If
> so, the attached might work? If not, then we need an extra call to
> GetFileAttributesEx(), or rearrange those calls in a way to trap errors
> there.
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.
--
Michael