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

On Thu, Jun 4, 2015 at 10:14 AM, Amit Kapila <amit(dot)kapila16(at)gmail(dot)com>
wrote:

> On Thu, Jun 4, 2015 at 1:52 AM, Andrew Dunstan <andrew(at)dunslane(dot)net>
> wrote:
>
>>
>> On 06/02/2015 11:55 PM, Amit Kapila wrote:
>>
>> On Tue, Jun 2, 2015 at 10:26 PM, Andrew Dunstan <andrew(at)dunslane(dot)net
>>> <mailto:andrew(at)dunslane(dot)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