Re: pg_basebackup vs. Windows and tablespaces

On 12/20/2014 05:59 AM, Amit Kapila wrote:
> On Wed, Dec 17, 2014 at 11:32 AM, Amit Kapila <amit(dot)kapila16(at)gmail(dot)com
> <mailto:amit(dot)kapila16(at)gmail(dot)com>> wrote:
> > On Tue, Dec 16, 2014 at 10:11 PM, Heikki Linnakangas
> <hlinnakangas(at)vmware(dot)com <mailto:hlinnakangas(at)vmware(dot)com>> wrote:
> > >
> > > On 12/16/2014 06:30 PM, Andrew Dunstan wrote:
> > >>
> > >> I'm not clear why human readability is the major criterion here.
> As for
> > >> that, it will be quite difficult for a human to distinguish a
> name with
> > >> a space at the end from one without. I really think a simple encoding
> > >> scheme would be much the best.
> >
> > Yeah that could work, but we need the special encoding mainly for
> newline,
> > other's would work with current patch. However it might be worth to do
> > it for all kind of spaces. Currently it just reads the line upto
> newline using
> > fscanf, but if we use special encoding, we might need to read the file
> > character by character and check for newline without backslash(or other
> > special encoding character); do you have something like that in mind?
> >
> > Another thing is that we need to take care that we encode/decode link
> > path for tar format, as plain format might already be working.
> >
>
> Attached patch handles the newline and other characters that are allowed
> in tablespace path, as we need escape character only for newline, I have
> added the same only for newline. So after patch, the tablespace_map
> file will look like below for different kind of paths, as you can see for
> tablespace id 16393 which contains newline, there is additional escape
> sequence "\" before each newline where as other paths containing space
> works as it is.
>
> 16391 /home/akapila/mywork/workspace_pg/master/tbs1
> 16393 /home/akapila/mywork/workspace_pg/master/tbs\
> a\
> b\
>
> 16392 /home/akapila/mywork/workspace_pg/master/tbs 2
>
> So with this, I have handled all review comments raised for this patch
> and it is ready for review, as the status of this patch is changed from
> "Ready for Committer" to "Waiting on Author", so ideally I think it
> should go back to "Ready for Committer", however as I am not very sure
> about this point, I will change it to "Needs Review" (correct me if I am
> wrong).
>
> Summarization of latest changes:
> 1. Change file name from symlink_label to tablespace_map and changed
> the same every where in comments and variable names.
> 2. This feature will be supportted for both windows and linux;
> tablespace_map
> file will be generated on both windows and linux to restore tablespace
> links
> during archive recovery.
> 3. Handling for special characters in tablesapce path name.
> 4. Updation of docs.
>

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"? I realize we ask for it
when pg_basebackup is operating in TAR format mode, but the backend has
no notion of that, does it? The only thing this does is trigger the
sending of the tablespace map, so surely that's what the protocol option
should suggest.

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.

cheers

andrew