Re: [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.

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(at)paquier(dot)xyz>:

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

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(at)paquier(dot)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

> --
> Michael
>

On Fri, May 18, 2018 at 02:42:08PM +0800, Paul Guo wrote:
> 2018-05-17 21:18 GMT+08:00 Michael Paquier <michael(at)paquier(dot)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(at)paquier(dot)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