Re: More time spending with "delete pending" - Mailing list pgsql-hackers

On Sat, Nov 14, 2020 at 01:00:00PM +0300, Alexander Lakhin wrote:
> As noted in [1], a sensible solution would be putting the same "retry on
> ERROR_ACCESS_DENIED" action in a wrapper for stat().
> And bed90759f brought in master the _pgstat64() function, where such
> error handling should be placed.
> So please look at the patch (based on the previous one made to fix
> bug#16161), that makes the attached test pass.

Your patch introduces a "loops", but doesn't use it to escape the loop.

> diff --git a/src/backend/utils/adt/genfile.c b/src/backend/utils/adt/genfile.c
> index d34182a7b0..922df49125 100644
> --- a/src/backend/utils/adt/genfile.c
> +++ b/src/backend/utils/adt/genfile.c
> @@ -28,6 +28,7 @@
>  #include "funcapi.h"
>  #include "mb/pg_wchar.h"
>  #include "miscadmin.h"
> +#include "port.h"
>  #include "postmaster/syslogger.h"
>  #include "storage/fd.h"
>  #include "utils/acl.h"
> diff --git a/src/port/win32stat.c b/src/port/win32stat.c
> index 4351aa4d08..c2b55c7fa6 100644
> --- a/src/port/win32stat.c
> +++ b/src/port/win32stat.c
> @@ -170,6 +170,7 @@ _pgstat64(const char *name, struct stat *buf)
>      SECURITY_ATTRIBUTES sa;
>      HANDLE        hFile;
>      int            ret;
> +    int            loops = 0;
>  #if _WIN32_WINNT < 0x0600
>      IO_STATUS_BLOCK ioStatus;
>      FILE_STANDARD_INFORMATION standardInfo;
> @@ -182,31 +183,60 @@ _pgstat64(const char *name, struct stat *buf)
>          errno = EINVAL;
>          return -1;
>      }
> -
>      /* fast not-exists check */
>      if (GetFileAttributes(name) == INVALID_FILE_ATTRIBUTES)
>      {
> -        _dosmaperr(GetLastError());
> -        return -1;
> +        DWORD        err = GetLastError();
> +
> +        if (err != ERROR_ACCESS_DENIED) {
> +            _dosmaperr(err);
> +            return -1;
> +        }
>      }
>  
>      /* get a file handle as lightweight as we can */
>      sa.nLength = sizeof(SECURITY_ATTRIBUTES);
>      sa.bInheritHandle = TRUE;
>      sa.lpSecurityDescriptor = NULL;
> -    hFile = CreateFile(name,
> -                       GENERIC_READ,
> -                       (FILE_SHARE_READ | FILE_SHARE_WRITE | FILE_SHARE_DELETE),
> -                       &sa,
> -                       OPEN_EXISTING,
> -                       (FILE_FLAG_NO_BUFFERING | FILE_FLAG_BACKUP_SEMANTICS |
> -                        FILE_FLAG_OVERLAPPED),
> -                       NULL);
> -    if (hFile == INVALID_HANDLE_VALUE)
> +    while ((hFile = CreateFile(name,
> +                               GENERIC_READ,
> +                               (FILE_SHARE_READ | FILE_SHARE_WRITE | FILE_SHARE_DELETE),
> +                               &sa,
> +                               OPEN_EXISTING,
> +                               (FILE_FLAG_NO_BUFFERING | FILE_FLAG_BACKUP_SEMANTICS |
> +                               FILE_FLAG_OVERLAPPED),
> +                               NULL)) == INVALID_HANDLE_VALUE)
>      {
>          DWORD        err = GetLastError();
>  
> -        CloseHandle(hFile);
> +        /*
> +         * ERROR_ACCESS_DENIED is returned if the file is deleted but not yet
> +         * gone (Windows NT status code is STATUS_DELETE_PENDING).  In that
> +         * case we want to wait a bit and try again, giving up after 1 second
> +         * (since this condition should never persist very long).  However,
> +         * there are other commonly-hit cases that return ERROR_ACCESS_DENIED,
> +         * so care is needed.  In particular that happens if we try to open a
> +         * directory, or of course if there's an actual file-permissions
> +         * problem.  To distinguish these cases, try a stat().  In the
> +         * delete-pending case, it will either also get STATUS_DELETE_PENDING,
> +         * or it will see the file as gone and fail with ENOENT.  In other
> +         * cases it will usually succeed.  The only somewhat-likely case where
> +         * this coding will uselessly wait is if there's a permissions problem
> +         * with a containing directory, which we hope will never happen in any
> +         * performance-critical code paths.
> +         */
> +        if (err == ERROR_ACCESS_DENIED)
> +        {
> +            struct microsoft_native_stat st;
> +
> +            if (microsoft_native_stat(name, &st) != 0)
> +            {
> +                pg_usleep(100000);
> +                loops++;
> +                continue;
> +            }
> +        }
> +
>          _dosmaperr(err);
>          return -1;
>      }