On Fri, Jun 5, 2015 at 7:29 AM, Andrew Dunstan <and...@dunslane.net> wrote:
> > On 06/04/2015 09:23 AM, Amit Kapila wrote: > >> >> >> >> 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. >> >> > > Where is it used? I can't see it called at all in tablespace.c or xlog.c. > > Below files use S_ISLINK check basebackup.c, fd.c, initdb.c, copy_fetch.c, pg_rewind/filemap.c and all these places use it with #ifndef WIN32 > Perhaps I'm being overcautious, but here's more or less what I had in mind. > What is making you feel nervous, if it is that we should not use unlink call without checking S_ISLINK, then we are already doing the same at many other places (rewriteheap.c, slru.c, timeline.c, xlog.c). It is already defined for Windows as pgunlink. Theoretically, I don't see much problem by changing the checks way you have done in patch, but it becomes different than what we have in destroy_tablespace_directories() and it is slightly changing the way check was originally done in create_tablespace_directories(), basically original check will try unlink if lstat returns non-zero return code. If you want to proceed with the changed checks as in v3, then may be we can modify comments on top of function remove_tablespace_symlink() which indicates that it works like destroy_tablespace_directories(). With Regards, Amit Kapila. EnterpriseDB: http://www.enterprisedb.com
