On Tue, May 12, 2015 at 5:50 AM, Andrew Dunstan <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>> 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