On Sat, Aug 13, 2022 at 02:46:24PM +0530, Bharath Rupireddy wrote: > On Sat, Aug 13, 2022 at 4:34 AM Bruce Momjian <br...@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 <br...@momjian.us> 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;
