Date: Sat, 14 Nov 2020 19:11:12 -0600
From: Justin Pryzby <pryzby@telsasoft.com>
To: Alexander Lakhin <exclusion@gmail.com>
Cc: pgsql-hackers <pgsql-hackers@postgresql.org>
Subject: Re: More time spending with "delete pending"
Message-ID: <20201115011112.GW30691@telsasoft.com>
References: <c3427edf-d7c0-ff57-90f6-b5de3bb62709@gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <c3427edf-d7c0-ff57-90f6-b5de3bb62709@gmail.com>
User-Agent: Mutt/1.9.4 (2018-02-28)
Precedence: bulk

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;
>  	}