On Thu, Jun 4, 2015 at 1:52 AM, Andrew Dunstan <and...@dunslane.net> wrote:
> > 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: >> >> 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 > > Okay, as we both seem to agree that it can be mostly used in tablespace symlinks context, so I have changed the name to remove_tablespace_symlink() and moved the function to tablespace.c. S_ISLINK check is used for non-windows code, so not sure adding it here makes any real difference now that we have made it specific to tablespace and we might need to write small port specific code if we want to add S_ISLINK check. > Given that the function raises an error on failure, I think it will > otherwise be OK as is. > > Please find an updated patch attached with this mail. With Regards, Amit Kapila. EnterpriseDB: http://www.enterprisedb.com
