Re: BUG #14243: pg_basebackup failes by a STATUS_DELETE_PENDING file

On Thu, Aug 18, 2016 at 2:25 AM, Michael Paquier <michael(dot)paquier(at)gmail(dot)com>
wrote:

> On Tue, Aug 16, 2016 at 4:56 PM, Magnus Hagander <magnus(at)hagander(dot)net>
> wrote:
> > On Wed, Jul 13, 2016 at 3:10 AM, Michael Paquier <
> michael(dot)paquier(at)gmail(dot)com>
> > wrote:
> >>
> >> On Wed, Jul 13, 2016 at 10:06 AM, Michael Paquier
> >> <michael(dot)paquier(at)gmail(dot)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.
>

That's pretty much exactly what I said, isn't it? :)

> > 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.
>

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?

>
> > 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.
>
>
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.

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.

--
Magnus Hagander
Me: http://www.hagander.net/
Work: http://www.redpill-linpro.com/