Re: Add lasterrno setting for dir_existsfile()

On Sat, Aug 13, 2022 at 4:34 AM Bruce Momjian <bruce(at)momjian(dot)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/