On 06/02/2015 11:55 PM, Amit Kapila wrote:
On Tue, Jun 2, 2015 at 10:26 PM, Andrew Dunstan <and...@dunslane.net
<mailto:and...@dunslane.net>> wrote:
On 05/15/2015 02:21 AM, Amit Kapila wrote:
Find the patch which gets rid of rmtree usage. I have made it as
a separate function because the same code is used from
create_tablespace_directories() as well. I thought of
extending the
same API for using it from destroy_tablespace_directories() as
well,
but due to special handling (especially for ENOENT) in that
function,
I left it as of now.
Well, it seems to me the new function is being altogether way too
trusting about the nature of what it's being asked to remove. In
the first place, the S_ISDIR/rmdir branch should only be for
Windows, and secondly in the other branch we should be checking
that S_ISLNK is true. It would actually be nice if we could test
for a junction point on Windows, but that seems to be a bit
difficult.
I think during recovery for tablespace related operations, it is
quite possible to have a directory instead of symlink in some
special cases (see function TablespaceCreateDbspace() and comments
in destroy_tablespace_directories() { ..Try to remove the symlink..}).
Also this new function is being called from
create_tablespace_directories()
which uses the code as written in new function, so it doesn't make much
sense to change it Windows and non-Windows specific code.
Looking at it again, this might be not as bad as I thought, but I do
think we should probably call the function something other than
rmsymlink(). That seems too generic, since it also tries to remove
directories - albeit that this will fail if the directory isn't empty.
And I still think we should add a test for S_ISLNK in the second branch.
As it stands the function could try to unlink anything that's not a
directory. That might be safe-ish in the context it's used in for the
tablespace code, but it's far from safe enough for a function that's in
src/common
Given that the function raises an error on failure, I think it will
otherwise be OK as is.
cheers
andrew
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
