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
Attachments:
add_lasterrno_setting.patchapplication/octet-stream; charset=ISO-8859-1; name=add_lasterrno_setting.patchDownload
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;
}
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/248c3a9This 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/248c3a9This 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/248c3a9This 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/248c3a9This 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.
Attachments:
lasterro.difftext/x-diff; charset=us-asciiDownload
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;