Thread: option -T in pg_basebackup doesn't work on windows

From: "Amit Kapila" <amit.kapila16@gmail.com>
> During my recent work on pg_basebackup, I noticed that
> -T option doesn't seem to work on Windows.
> The reason for the same is that while updating symlinks
> it doesn't consider that on Windows, junction points can
> be directories due to which it is not able to update the
> symlink location.
> Fix is to make the code work like symlink removal code
> in destroy_tablespace_directories.  Attached patch fixes
> problem.

I could reproduce the problem on my Windows machine.

The code change appears correct, but the patch application failed against 
the latest source code.  I don't know why.  Could you confirm this?

patching file src/bin/pg_basebackup/pg_basebackup.c
Hunk #1 FAILED at 1119.
1 out of 1 hunk FAILED -- saving rejects to file 
src/bin/pg_basebackup/pg_basebackup.c.rej


On the following line, I think %d must be %u, because Oid is an unsigned 
integer.
 char    *linkloc = psprintf("%s/pg_tblspc/%d", basedir, oid);

Regards
MauMau

On Thu, Aug 14, 2014 at 12:50 AM, MauMau <maumau307@gmail.com> wrote:
> The code change appears correct, but the patch application failed against
> the latest source code.  I don't know why.  Could you confirm this?
>
> patching file src/bin/pg_basebackup/pg_basebackup.c
> Hunk #1 FAILED at 1119.
> 1 out of 1 hunk FAILED -- saving rejects to file
> src/bin/pg_basebackup/pg_basebackup.c.rej
This conflict is caused by f25e0bf.

> On the following line, I think %d must be %u, because Oid is an unsigned
> integer.
>  char    *linkloc = psprintf("%s/pg_tblspc/%d", basedir, oid);
That's correct.
-- 
Michael

Thank you.  The code looks correct.  I confirmed that the pg_basebackup 
could relocate the tablespace directory on Windows.

I marked this patch as ready for committer.

Regards
MauMau

On 8/16/14 8:46 AM, Amit Kapila wrote:
> On Fri, Aug 15, 2014 at 1:03 PM, MauMau <maumau307@gmail.com
> <mailto:maumau307@gmail.com>> wrote:
>>
>> Thank you.  The code looks correct.  I confirmed that the
> pg_basebackup could relocate the tablespace directory on Windows.
>>
>> I marked this patch as ready for committer.
> 
> Thanks for the review.

It's not ready for committer if the current patch does not apply.

On Mon, Aug 18, 2014 at 9:37 AM, Peter Eisentraut <peter_e@gmx.net> wrote:
> It's not ready for committer if the current patch does not apply.
FWIW, the latest version sent by Amit here applies correctly:
http://www.postgresql.org/message-id/CAA4eK1+cC9RB1S9Q4+nSOkfas1YufVFGFxUHxY_6wLBQ1ReroQ@mail.gmail.com
I haven't tested it myself though.

However, independently on this patch and as pointed by MauMau, the
code that has been committed in fb05f3c is incorrect in the way it
defines the tablespace path, this:
psprintf("%s/pg_tblspc/%d", basedir, oid);
should be this:
psprintf("%s/pg_tblspc/%u", basedir, oid);
I am separating this fix (that should be backpatched to REL9_4_STABLE
as well), in the patch attached if this helps.
Regards,
--
Michael

On Mon, Aug 18, 2014 at 11:51 AM, Amit Kapila <amit.kapila16@gmail.com> wrote:
> On Mon, Aug 18, 2014 at 7:08 AM, Michael Paquier <michael.paquier@gmail.com>
> wrote:
>>
>> On Mon, Aug 18, 2014 at 9:37 AM, Peter Eisentraut <peter_e@gmx.net> wrote:
>> > It's not ready for committer if the current patch does not apply.
>> FWIW, the latest version sent by Amit here applies correctly:
>>
>> http://www.postgresql.org/message-id/CAA4eK1+cC9RB1S9Q4+nSOkfas1YufVFGFxUHxY_6wLBQ1ReroQ@mail.gmail.com
>> I haven't tested it myself though.
>>
>> However, independently on this patch and as pointed by MauMau, the
>> code that has been committed in fb05f3c is incorrect in the way it
>> defines the tablespace path, this:
>> psprintf("%s/pg_tblspc/%d", basedir, oid);
>> should be this:
>> psprintf("%s/pg_tblspc/%u", basedir, oid);
>> I am separating this fix (that should be backpatched to REL9_4_STABLE
>> as well), in the patch attached if this helps.
>
> I think the patch provided by me needs to be back patched to
> 9.4 as this is a new option introduced in 9.4 which is not working
> on windows.
Yep. Definitely. This will fix both issues at the same time.
-- 
Michael

I didn't follow the original discussions, but now that I look at this I 
have to wonder:

Why does pg_basebackup -T create the symlink pointing to the wrong 
location in the first place, only to fix it later? Wouldn't it make a 
lot more sense to create it correctly in the first place?

- Heikki

On 08/22/2014 07:08 AM, Amit Kapila wrote:
> On Thu, Aug 21, 2014 at 3:44 PM, Amit Kapila <amit.kapila16@gmail.com>
> wrote:
>>
>> On Tue, Aug 19, 2014 at 9:51 AM, Amit Kapila <amit.kapila16@gmail.com>
> wrote:
>>> On Mon, Aug 18, 2014 at 7:50 PM, Heikki Linnakangas <
> hlinnakangas@vmware.com> wrote:
>>>> Wouldn't it make a lot more sense to create it correctly in the first
> place?
>>>
>>> Looking at the code, I think it is very well possible to create
>>> it correctly in the first place without much extra work.  I will
>>> send a patch if nobody sees any problem with this change.
>>
>> Attached patch implements the above suggested fix.
>> I have removed the earlier code which was used to update the
>> symlink path.
>
> Today morning, I realised that there is one problem with the
> patch I sent yesterday and the problem is that incase user
> has not given -T option, it will not be able to create the symlink
> for appropriate path.  Attached patch fix this issue.

Thanks, committed with minor changes:

* fixed the error message to print the mapped path that it actually 
tried to create, instead of the original.
* there's no need to copy the mapped path string, so I just used a pointer
* made the code to do the basetablespace mapping in top of the function 
a little bit tidier (IMHO anyway), although it wasn't really this patch's.
* I noticed that the mappings now apply to any symlinks in the data 
directory. I think that's OK, we don't expect there to be any other 
symlinks, especially not pointing to a tablespace location, but if there 
are, it's arguably a good thing that they are mapped too.

- Heikki

On 08/22/2014 11:35 AM, Amit Kapila wrote:
> On Fri, Aug 22, 2014 at 1:00 PM, Heikki Linnakangas <hlinnakangas@vmware.com>
> wrote:
>> On 08/22/2014 07:08 AM, Amit Kapila wrote:
>>> Today morning, I realised that there is one problem with the
>>> patch I sent yesterday and the problem is that incase user
>>> has not given -T option, it will not be able to create the symlink
>>> for appropriate path.  Attached patch fix this issue.
>>
>>
>> Thanks, committed with minor changes:
>
> Thank you.  One minor point I observed is that in comments below,
> you have used --tablespace as option name rather than
> --tablespace-mapping, is it intentional?
>
> +                    * the location of a tablespace. Apply any tablespace
> +                    * mapping given on the command line (--tablespace).

Sorry, my mistake. Fixed.

- Heikki