Thread: Add lasterrno setting for dir_existsfile()
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
Attachment
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
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? -- Bruce Momjian <bruce@momjian.us> https://momjian.us EDB https://enterprisedb.com Indecision is a decision. Inaction is an action. Mark Batterson
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. -- Bharath Rupireddy RDS Open Source Databases: https://aws.amazon.com/rds/postgresql/
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.