Date: Fri, 12 Aug 2022 18:22:01 -0400
From: Bruce Momjian <bruce@momjian.us>
To: Wei Sun <936739278@qq.com>
Cc: pgsql-hackers <pgsql-hackers@postgresql.org>
Subject: Re: Add lasterrno setting for dir_existsfile()
Message-ID: <YvbSidQSzscEErFW@momjian.us>
References: <tencent_1276C08F98579CC19D8A4488C848A8411806@qq.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <tencent_1276C08F98579CC19D8A4488C848A8411806@qq.com>
Archived-At: 
 <https://www.postgresql.org/message-id/YvbSidQSzscEErFW%40momjian.us>
Precedence: bulk

On Mon, Jan 10, 2022 at 12:19:28AM +0800, Wei Sun wrote:
> Hi,
> 
> Some time ago,the following patch clean up error handling in pg_basebackup's
> walmethods.c.
> https://github.com/postgres/postgres/commit/248c3a9
> 
> This patch keep the error state in the DirectoryMethodData struct,
> in most functions, the lasterrno is set correctly, but in function
> dir_existsfile(), 
> the lasterrno is not set when the file fails to open.
> 
> If this is a correction omission, I think this patch can fix this.
> 
> Cheers

> diff --git a/src/bin/pg_basebackup/walmethods.c b/src/bin/pg_basebackup/walmethods.c
> index f74bd13..35cf5a8 100644
> --- a/src/bin/pg_basebackup/walmethods.c
> +++ b/src/bin/pg_basebackup/walmethods.c
> @@ -580,7 +580,10 @@ dir_existsfile(const char *pathname)
>  
>  	fd = open(tmppath, O_RDONLY | PG_BINARY, 0);
>  	if (fd < 0)
> +	{
> +		dir_data->lasterrno = errno;
>  		return false;
> +	}
>  	close(fd);
>  	return true;
>  }

Looking at this, the function is used to check if something exists, and
return a boolean. I am not sure it is helpful to also return a ENOENT in
the lasterrno status field.  It might be useful to set lasterrno if the
open fails and it is _not_ ENOENT.

-- 
  Bruce Momjian  <bruce@momjian.us>        https://momjian.us
  EDB                                      https://enterprisedb.com

  Indecision is a decision.  Inaction is an action.  Mark Batterson