Обсуждение: Add lasterrno setting for dir_existsfile()

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.