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

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:
>
>
> On 05/15/2015 02:21 AM, Amit Kapila wrote:
>
>
> Find the patch which gets rid of rmtree usage. I have made it as
> a separate function because the same code is used from
> create_tablespace_directories() as well. I thought of
> extending the
> same API for using it from destroy_tablespace_directories() as
> well,
> but due to special handling (especially for ENOENT) in that
> function,
> I left it as of now.
>
>
>
>
>
>
> 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

Given that the function raises an error on failure, I think it will
otherwise be OK as is.

cheers

andrew