public inbox for [email protected]  

help / color / mirror / Atom feed

From: Bruce Momjian <[email protected]>
To: Bharath Rupireddy <[email protected]>
Cc: Wei Sun <[email protected]>
Cc: pgsql-hackers <[email protected]>
Subject: Re: Add lasterrno setting for dir_existsfile()
Date: Tue, 31 Oct 2023 12:00:38 -0400
Message-ID: <[email protected]> (raw)
In-Reply-To: <CALj2ACX_5Je9+qYLYwKCuDQqvJS3uwTOKL5tAYkvN75A+xKGzQ@mail.gmail.com>
References: <[email protected]>
	<[email protected]>
	<YvbcbLub4Ph54/[email protected]>
	<CALj2ACX_5Je9+qYLYwKCuDQqvJS3uwTOKL5tAYkvN75A+xKGzQ@mail.gmail.com>

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 protected]> 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 protected]>        https://momjian.us
  EDB                                      https://enterprisedb.com

  Only you can decide what is important to you.


Attachments:

  [text/x-diff] lasterro.diff (504B, ../[email protected]/2-lasterro.diff)
  download | inline 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;

view thread (2+ messages)

reply

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Reply to all the recipients using the --to and --cc options:
  reply via email

  To: [email protected]
  Cc: [email protected], [email protected], [email protected]
  Subject: Re: Add lasterrno setting for dir_existsfile()
  In-Reply-To: <[email protected]>

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox