Subject: Re: Add lasterrno setting for dir_existsfile()

From: Bruce Momjian <[email-masked]>
Date: Tue, 31 Oct 2023 12:00:38 -0400

On Sat, Aug 13, 2022 at 02:46:24PM +0530, Bharath Rupireddy wrote:
> On Sat, Aug 13, 2022 at 4:34 AM Bruce Momjian <[email-masked]> 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  <[email-masked]>        https://momjian.us
  EDB                                      https://enterprisedb.com

  Only you can decide what is important to you.
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;