On Thu, Jun 4, 2015 at 10:14 AM, Amit Kapila <amit.kapil...@gmail.com> wrote:
> 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. > > Sorry, forgot to attach the patch in previous mail, now attaching. Thanks Bruce for reminding me offlist. With Regards, Amit Kapila. EnterpriseDB: http://www.enterprisedb.com
remove_only_symlinks_during_recovery_v2.patch
Description: Binary data
-- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
