Re: Re: [COMMITTERS] pgsql: Map basebackup tablespaces using a tablespace_map file

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