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

On 06/08/2015 12:08 AM, Amit Kapila wrote:
> On Mon, Jun 8, 2015 at 5:52 AM, Andrew Dunstan <andrew@dunslane.net 
> <mailto:andrew@dunslane.net>> wrote:
> >
> > On 06/05/2015 11:08 PM, Amit Kapila wrote:
> >>
> >>
> >>     Okay, I think I can understand why you want to be cautious for
> >>     having a different check for this path, but in that case there is a
> >>     chance that recovery might fail when it will try to create a 
> symlink
> >>     for that file.  Shouldn't we then try to call this new function 
> only
> >>     when we are trying to restore from tablespace_map file and also
> >>     is there a need of ifdef S_ISLINK in remove_tablespace_link?
> >>
> >>
> >> Shall I send an updated patch on these lines or do you want to
> >> proceed with v3 version?
> >>
> >>
> >
> > The point seems to me that we should not be removing anything that's 
> not an empty directory or symlink, and that nothing else has any 
> business being in pg_tblspc. If we encounter such a thing whose name 
> conflicts with the name of a tablespace link we wish to create, rather 
> than quietly blowing it away we should complain loudly, and error out, 
> making it the user's responsibility to clean up their mess. Am I 
> missing something?
> >
>
> How about if it is just a flat file with same name as tablespace link,
> why we want to give error for that case?  I think now it just don't do
> anything with that file (unlink will fail with ENOENT and it will be
> ignored, atleast thats the way currently it behaves in Windows) and
> create a separate symlink with same name which seems okay to
> me and in the change proposed by you it will give error, do you see
> any reason for doing so?
>
>


This is surely wrong. unlink won't fail with ENOENT if the file is 
present; ENOENT means that the file is NOT present. It will succeed if 
the file is present, which is exactly what I'm saying is wrong.

I realize our existing code just more or less assumes that that it's a 
symlink. I think we've probably been a bit careless there.

cheers

andrew