On 06/04/2015 11:35 PM, Amit Kapila wrote:
> On Fri, Jun 5, 2015 at 7:29 AM, Andrew Dunstan <andrew@dunslane.net
> <mailto:andrew@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().
>
The difference is that here we're getting the list from a base backup
and it seems to me the risk of having a file we don't really want to
unlink is significantly greater.
cheers
andrew
