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

On Fri, Jun 5, 2015 at 7:29 AM, Andrew Dunstan <andrew(at)dunslane(dot)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