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

On 05/15/2015 02:21 AM, Amit Kapila wrote:
> On Thu, May 14, 2015 at 10:29 PM, Andrew Dunstan <andrew(at)dunslane(dot)net
> <mailto:andrew(at)dunslane(dot)net>> wrote:
> >
> >
> > On 05/14/2015 10:52 AM, Robert Haas wrote:
> >>
> >> On Thu, May 14, 2015 at 12:12 AM, Amit Kapila
> <amit(dot)kapila16(at)gmail(dot)com <mailto:amit(dot)kapila16(at)gmail(dot)com>> wrote:
> >>>
> >>> On Thu, May 14, 2015 at 2:10 AM, Andrew Dunstan
> <andrew(at)dunslane(dot)net <mailto:andrew(at)dunslane(dot)net>> wrote:
> >>>>
> >>>> How about if we simply abort if we find a non-symlink where we
> want the
> >>>> symlink to be, and only remove something that is actually a
> symlink (or a
> >>>> junction point, which is more or less the same thing)?
> >>>
> >>> We can do that way and for that I think we need to use rmdir
> >>> instead of rmtree in the code being discussed (recovery path),
> >>> OTOH we should try to minimize the errors raised during
> >>> recovery.
> >>
> >> I'm not sure I understand this issue in detail, but why would using
> >> rmtree() on something you expect to be a symlink ever be a good idea?
> >> It seems like if things are the way you expect them to be, it has no
> >> benefit, but if they are different from what you expect, you might
> >> blow away a ton of important data.
> >>
> >> Maybe I am just confused.
> >>
> >
> >
> > The suggestion is to get rid of using rmtree. Instead, if we find a
> non-symlink in pg_tblspc we'll make the user clean it up before we can
> continue. So your instinct is in tune with my suggestion.
> >
>
> 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. The function should
probably return a boolean to indicate any error, rather than void, so
that the caller can take action accordingly. In the present case we
should probably be stopping recovery dead in its tracks, but I certainly
don't think we should just be ignoring it.

cheers

andrew