Re: pg_basebackup vs. Windows and tablespaces

On 05/11/2015 11:01 PM, Amit Kapila wrote:
>
>
> On Tue, May 12, 2015 at 5:50 AM, Andrew Dunstan <andrew@dunslane.net 
> <mailto:andrew@dunslane.net>> wrote:
> >
> >
> > On 05/11/2015 02:02 AM, Amit Kapila wrote:
> >>
> >> On Sun, May 10, 2015 at 6:01 AM, Andrew Dunstan 
> <andrew@dunslane.net <mailto:andrew@dunslane.net> 
> <mailto:andrew@dunslane.net <mailto:andrew@dunslane.net>>> wrote:
> >> >
> >> >
> >> >
> >> > This generally looks good, but I have a couple of questions 
> before I commit it.
> >> >
> >> > First, why is the new option for the  BASE_BACKUP command of the 
> Streaming Replication protcol "TAR"? It seems rather misleading. 
> Shouldn't it be something like "TABLESPACEMAP"?
> >> >
> >>
> >> The reason to keep new option's name as TAR was that tablespace_map
> >> was generated for that format type, but I agree with you that something
> >> like "TABLESPACEMAP" suits better, so I have changed it to
> >> "TABLESPACE_MAP".  Putting '_' in name makes it somewhat consistent
> >> with other names and filename it generates with this new option.
> >>
> >>
> >> > Second, these lines in xlog.c seem wrong:
> >> >
> >> >         else if ((ch == '\n' || ch == '\r') && prev_ch == '\\')
> >> >             str[i-1] = '\n';
> >> >
> >> > It looks to me like we should be putting ch in the string, not 
> arbitrarily transforming \r into \n.
> >> >
> >>
> >> You are right, I have changed it as per your suggestion.
> >>
> >>
> >
> >
> > OK, I have cleaned this up a bit - I had already started so I didn't 
> take your latest patch but instead applied relevant changes to my 
> changeset. Here is my latest version.
> >
> > In testing I notice that now "pg_baseback -F t" leaves it completely 
> up to the user on all platforms to create the relevant links in 
> pg_tblspc/. It includes the tablespace_map file in base.tar, but 
> that's really just informational.
> >
>
> Sorry, but I am not able to follow your point.   User don't need to create
> the relevant links, they will get created during first startup (during 
> recovery)
> from the backup.  I have tested and it works both on Windows and Linux.
>
> Refer below code of patch in xlog.c
>
> +
> + /* read the tablespace_map file if present and create symlinks. */
> + if (read_tablespace_map(&tablespaces))
> + {
> ..
>
> > I think we need to add something to the pg_basebackup docs about 
> that, at the very least (and it will also need to be a release note item.)
> >
>
> Currently, below lines in patch suggest that this file is required for 
> recovery.
> Do you expect more information to be added?
>
> +    These files are not merely for your information; their presence and
> +    contents are critical to the proper operation of the system's 
> recovery
> +    process.
>


Yes, sorry, I had a moment of brain fade yesterday. However, I think 
we're a bit under-documented on the pg_basebackup page, regarding both 
tar mode and tablespace_map (which isn't even mentioned).

And there is this which I noticed this morning: the --tablespace-mapping 
feature of pg_basebackup seems to be quite broken in --format=tar mode - 
it simply has no effect AFAICT. I assume it was broken before, but we 
should either fix it (possibly hard) or disallow the combination (which 
would be a pity).

I'm going to go ahead and commit this in the state I have it now, 
because for the most part these are preexisting deficiencies.

cheers

andrew