On 05/11/2015 11:01 PM, Amit Kapila wrote:
On Tue, May 12, 2015 at 5:50 AM, Andrew Dunstan <and...@dunslane.net
<mailto:and...@dunslane.net>> wrote:
>
>
> On 05/11/2015 02:02 AM, Amit Kapila wrote:
>>
>> On Sun, May 10, 2015 at 6:01 AM, Andrew Dunstan
<and...@dunslane.net <mailto:and...@dunslane.net>
<mailto:and...@dunslane.net <mailto:and...@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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
