Re: Add lasterrno setting for dir_existsfile()

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(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.

I have applied the attached patch to master to explain why we don't set
lasterrno.

--
Bruce Momjian <bruce(at)momjian(dot)us> https://momjian.us
EDB https://enterprisedb.com

Only you can decide what is important to you.