Date: Tue, 31 Oct 2023 12:00:38 -0400
From: Bruce Momjian <bruce@momjian.us>
To: Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com>
Cc: Wei Sun <936739278@qq.com>,
	pgsql-hackers <pgsql-hackers@postgresql.org>
Subject: Re: Add lasterrno setting for dir_existsfile()
Message-ID: <ZUEkpjraVnmaFSOY@momjian.us>
References: <tencent_1276C08F98579CC19D8A4488C848A8411806@qq.com>
 <YvbSidQSzscEErFW@momjian.us>
 <YvbcbLub4Ph54/Mi@momjian.us>
 <CALj2ACX_5Je9+qYLYwKCuDQqvJS3uwTOKL5tAYkvN75A+xKGzQ@mail.gmail.com>
MIME-Version: 1.0
Content-Type: multipart/mixed; boundary="26Bj5Xcg0cbDBh2g"
Content-Disposition: inline
In-Reply-To: <CALj2ACX_5Je9+qYLYwKCuDQqvJS3uwTOKL5tAYkvN75A+xKGzQ@mail.gmail.com>
Archived-At: <https://www.postgresql.org/message-id/ZUEkpjraVnmaFSOY%40momjian.us>
Precedence: bulk


--26Bj5Xcg0cbDBh2g
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline

On Sat, Aug 13, 2022 at 02:46:24PM +0530, Bharath Rupireddy wrote:
> On Sat, Aug 13, 2022 at 4:34 AM Bruce Momjian <bruce@momjian.us> wrote:
> >
> > On Fri, Aug 12, 2022 at 06:22:01PM -0400, Bruce Momjian wrote:
> > > 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.
> > >
> > > 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.
> >
> > Thinking some more, how would you know to check lasterrno since exists
> > and not exists are both valid outputs?
> 
> I agree with Bruce here, ENOENT isn't a failure for open because it
> says that file doesn't exist.
> 
> If we have the policy like every syscall failure must be captured in
> lasterrno and be reported by the callers accordingly, then the patch
> (of course, with the change that doesn't set lasterrno when errno is
> ENOENT) proposed makes sense to me. Right now, the callers of
> existsfile() aren't caring for the errno though. Every other open()
> syscall failure in walmethods.c is captured in lasterrno.
> 
> Otherwise, adding a comment in dir_existsfile() on why aren't
> capturing lasterrno might help and avoid future discussions around
> this.

I have applied the attached patch to master to explain why we don't set
lasterrno.

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

  Only you can decide what is important to you.

--26Bj5Xcg0cbDBh2g
Content-Type: text/x-diff; charset=us-ascii
Content-Disposition: attachment; filename="lasterro.diff"

diff --git a/src/bin/pg_basebackup/walmethods.c b/src/bin/pg_basebackup/walmethods.c
index 2de11ce9b1..33cb85b849 100644
--- a/src/bin/pg_basebackup/walmethods.c
+++ b/src/bin/pg_basebackup/walmethods.c
@@ -594,6 +594,11 @@ dir_existsfile(WalWriteMethod *wwmethod, const char *pathname)
 
 	fd = open(tmppath, O_RDONLY | PG_BINARY, 0);
 	if (fd < 0)
+
+		/*
+		 * Skip setting dir_data->lasterrno here because we are only checking
+		 * for existence.
+		 */
 		return false;
 	close(fd);
 	return true;

--26Bj5Xcg0cbDBh2g--