On 12/20/2014 05:59 AM, Amit Kapila wrote:
On Wed, Dec 17, 2014 at 11:32 AM, Amit Kapila <amit.kapil...@gmail.com
<mailto:amit.kapil...@gmail.com>> wrote:
> On Tue, Dec 16, 2014 at 10:11 PM, Heikki Linnakangas
<hlinnakan...@vmware.com <mailto:hlinnakan...@vmware.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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers