[PATCH] Use access() to check file existence in GetNewRelFileNode().
Previous code uses BasicOpenFile() + close().
access() should be faster than BasicOpenFile()+close() and access()
should be more correct since BasicOpenFile() could fail for various
cases (e.g. due to file permission, etc) even the file exists.
access() is supported on Linux/Unix. I do not have a Windows dev
environment, but MSDN tells me that access() is supported on Windows also
and there have been access() call in the workspace, so I assume there is no
portability issue.
Thanks.
Attachments:
0001-Use-access-to-check-file-existence-in-GetNewRelFileN.patchapplication/octet-stream; name=0001-Use-access-to-check-file-existence-in-GetNewRelFileN.patchDownload
From 924ccdcea246eb7c5ebbe7909ce5618fe723a372 Mon Sep 17 00:00:00 2001
From: Paul Guo <paulguo@gmail.com>
Date: Thu, 17 May 2018 11:24:37 +0800
Subject: [PATCH] Use access() to check file existence in GetNewRelFileNode().
Previous code use BasicOpenFile() + close().
access() should be faster than BasicOpenFile()+close() and access()
should be more correct since BasicOpenFile() could fail for various
cases (e.g. due to file permission, etc) even the file exists.
---
src/backend/catalog/catalog.c | 5 +----
1 file changed, 1 insertion(+), 4 deletions(-)
diff --git a/src/backend/catalog/catalog.c b/src/backend/catalog/catalog.c
index 2292deb703..e516055a2b 100644
--- a/src/backend/catalog/catalog.c
+++ b/src/backend/catalog/catalog.c
@@ -397,7 +397,6 @@ GetNewRelFileNode(Oid reltablespace, Relation pg_class, char relpersistence)
{
RelFileNodeBackend rnode;
char *rpath;
- int fd;
bool collides;
BackendId backend;
@@ -445,12 +444,10 @@ GetNewRelFileNode(Oid reltablespace, Relation pg_class, char relpersistence)
/* Check for existing file of same name */
rpath = relpath(rnode, MAIN_FORKNUM);
- fd = BasicOpenFile(rpath, O_RDONLY | PG_BINARY);
- if (fd >= 0)
+ if (access(rpath, F_OK) == 0)
{
/* definite collision */
- close(fd);
collides = true;
}
else
--
2.15.1 (Apple Git-101)
On Thu, May 17, 2018 at 04:09:27PM +0800, Paul Guo wrote:
Previous code uses BasicOpenFile() + close().
access() should be faster than BasicOpenFile()+close() and access()
should be more correct since BasicOpenFile() could fail for various
cases (e.g. due to file permission, etc) even the file exists.
Failing because of file permissions would be correct. There have been
cases in the past, particularly on Windows, where anti-virus softwares
wildly scan files, causing EACCES on various points of the data folder.
access() is supported on Linux/Unix. I do not have a Windows dev
environment, but MSDN tells me that access() is supported on Windows also
and there have been access() call in the workspace, so I assume there is no
portability issue.
Yes, access() is spread already in the core code.
- fd = BasicOpenFile(rpath, O_RDONLY | PG_BINARY);
- if (fd >= 0)
+ if (access(rpath, F_OK) == 0)What you are looking for here is R_OK, no?
--
Michael
F_OK seems to be better than R_OK because we want to check file existence
(not read permission) before creating the relation file with the path
later.
2018-05-17 17:09 GMT+08:00 Michael Paquier <michael@paquier.xyz>:
Show quoted text
On Thu, May 17, 2018 at 04:09:27PM +0800, Paul Guo wrote:
Previous code uses BasicOpenFile() + close().
access() should be faster than BasicOpenFile()+close() and access()
should be more correct since BasicOpenFile() could fail for various
cases (e.g. due to file permission, etc) even the file exists.Failing because of file permissions would be correct. There have been
cases in the past, particularly on Windows, where anti-virus softwares
wildly scan files, causing EACCES on various points of the data folder.access() is supported on Linux/Unix. I do not have a Windows dev
environment, but MSDN tells me that access() is supported on Windows also
and there have been access() call in the workspace, so I assume there isno
portability issue.
Yes, access() is spread already in the core code.
- fd = BasicOpenFile(rpath, O_RDONLY | PG_BINARY);
- if (fd >= 0) + if (access(rpath, F_OK) == 0)What you are looking for here is R_OK, no?
--
Michael
On Thu, May 17, 2018 at 05:23:28PM +0800, Paul Guo wrote:
F_OK seems to be better than R_OK because we want to check file existence
(not read permission) before creating the relation file with the path
later.
Please do not top-post, this breaks the discussion logic of the thread.
Perhaps Tom remembers why things have been done this way in 721e537. At
the end, on second-thoughts, the current coding looks a bit
over-engineered as there is no check for any error codes other than
ENOENT so using only F_OK would be OK. Note that access cannot complain
about EPERM at least on Linux, so you'd want to actually modify this
comment block.
You should also add this patch to the next commit fest, development of
v11 is done and it is a stabilization period now, so no new patches are
merged. Here is where you can register the patch:
https://commitfest.postgresql.org/18/
--
Michael
2018-05-17 21:18 GMT+08:00 Michael Paquier <michael@paquier.xyz>:
On Thu, May 17, 2018 at 05:23:28PM +0800, Paul Guo wrote:
F_OK seems to be better than R_OK because we want to check file existence
(not read permission) before creating the relation file with the path
later.Please do not top-post, this breaks the discussion logic of the thread.
Perhaps Tom remembers why things have been done this way in 721e537. At
the end, on second-thoughts, the current coding looks a bit
over-engineered as there is no check for any error codes other than
ENOENT so using only F_OK would be OK. Note that access cannot complain
about EPERM at least on Linux, so you'd want to actually modify this
comment block.
Thanks. Agreed. Attached is the new patch.
You should also add this patch to the next commit fest, development of
v11 is done and it is a stabilization period now, so no new patches are
merged. Here is where you can register the patch:
https://commitfest.postgresql.org/18/
Submitted.
-Paul
Show quoted text
--
Michael
Attachments:
0001-Use-access-to-check-file-existence-in-GetNewRelFileN.v2.patchapplication/octet-stream; name=0001-Use-access-to-check-file-existence-in-GetNewRelFileN.v2.patchDownload
From 817cfd196dc055b663412a149a6e3554b21c0a90 Mon Sep 17 00:00:00 2001
From: Paul Guo <paulguo@gmail.com>
Date: Thu, 17 May 2018 11:24:37 +0800
Subject: [PATCH] Use access() to check file existence in GetNewRelFileNode().
Previous code use BasicOpenFile() + close().
access() should be faster than BasicOpenFile()+close() and access()
should be more correct since BasicOpenFile() could fail for various
cases (e.g. due to file permission, etc) even the file exists.
---
src/backend/catalog/catalog.c | 13 +++----------
1 file changed, 3 insertions(+), 10 deletions(-)
diff --git a/src/backend/catalog/catalog.c b/src/backend/catalog/catalog.c
index 2292deb703..9f119a031f 100644
--- a/src/backend/catalog/catalog.c
+++ b/src/backend/catalog/catalog.c
@@ -397,7 +397,6 @@ GetNewRelFileNode(Oid reltablespace, Relation pg_class, char relpersistence)
{
RelFileNodeBackend rnode;
char *rpath;
- int fd;
bool collides;
BackendId backend;
@@ -445,24 +444,18 @@ GetNewRelFileNode(Oid reltablespace, Relation pg_class, char relpersistence)
/* Check for existing file of same name */
rpath = relpath(rnode, MAIN_FORKNUM);
- fd = BasicOpenFile(rpath, O_RDONLY | PG_BINARY);
- if (fd >= 0)
+ if (access(rpath, F_OK) == 0)
{
/* definite collision */
- close(fd);
collides = true;
}
else
{
/*
* Here we have a little bit of a dilemma: if errno is something
- * other than ENOENT, should we declare a collision and loop? In
- * particular one might think this advisable for, say, EPERM.
- * However there really shouldn't be any unreadable files in a
- * tablespace directory, and if the EPERM is actually complaining
- * that we can't read the directory itself, we'd be in an infinite
- * loop. In practice it seems best to go ahead regardless of the
+ * other than ENOENT, should we declare a collision and loop?
+ * In practice it seems best to go ahead regardless of the
* errno. If there is a colliding file we will get an smgr
* failure when we attempt to create the new relation file.
*/
--
2.15.1 (Apple Git-101)
On Fri, May 18, 2018 at 02:42:08PM +0800, Paul Guo wrote:
2018-05-17 21:18 GMT+08:00 Michael Paquier <michael@paquier.xyz>:
You should also add this patch to the next commit fest, development of
v11 is done and it is a stabilization period now, so no new patches are
merged. Here is where you can register the patch:
https://commitfest.postgresql.org/18/Submitted.
Thanks.
--
Michael
On 18.05.18 16:04, Michael Paquier wrote:
On Fri, May 18, 2018 at 02:42:08PM +0800, Paul Guo wrote:
2018-05-17 21:18 GMT+08:00 Michael Paquier <michael@paquier.xyz>:
You should also add this patch to the next commit fest, development of
v11 is done and it is a stabilization period now, so no new patches are
merged. Here is where you can register the patch:
https://commitfest.postgresql.org/18/Submitted.
This patch looks sensible to me. We also use access() elsewhere in the
backend to test for file existence.
--
Peter Eisentraut http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On Fri, Jul 06, 2018 at 10:52:14PM +0200, Peter Eisentraut wrote:
This patch looks sensible to me. We also use access() elsewhere in the
backend to test for file existence.
Yes, the patch made sense when I looked at it, and it still does, so
committed.
--
Michael