option -T in pg_basebackup doesn't work on windows

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.

Steps to reproduce problem
------------------------------------------
1. Start server and connect psql client
2. Create Tablespace tbs location 'C:\database\tbs';
3. pg_basebackup.exe -D C:\Data -Fp -x -T C:\database\tbs=C:\database\tbs1

pg_basebackup: could not remove symbolic link "C:\Data/pg_tblspc/16390":
Permission denied

With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com

From: "Amit Kapila" <amit(dot)kapila16(at)gmail(dot)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(at)gmail(dot)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

On Wed, Aug 13, 2014 at 9:20 PM, MauMau <maumau307(at)gmail(dot)com> wrote:
>
> From: "Amit Kapila" <amit(dot)kapila16(at)gmail(dot)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

It failed due to one of recent commits as mentioned by
Michael. Please find the rebased patch attached with this
mail

> 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);

Yeah, though this is not introduced by patch, but I think
this should be fixed and I have fixed it attached patch.

With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com

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 Fri, Aug 15, 2014 at 1:03 PM, MauMau <maumau307(at)gmail(dot)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.

With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com

On 8/16/14 8:46 AM, Amit Kapila wrote:
> On Fri, Aug 15, 2014 at 1:03 PM, MauMau <maumau307(at)gmail(dot)com
> <mailto:maumau307(at)gmail(dot)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(at)gmx(dot)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 7:08 AM, Michael Paquier <michael(dot)paquier(at)gmail(dot)com>
wrote:
>
> On Mon, Aug 18, 2014 at 9:37 AM, Peter Eisentraut <peter_e(at)gmx(dot)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.

With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com

On Mon, Aug 18, 2014 at 11:51 AM, Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
> On Mon, Aug 18, 2014 at 7:08 AM, Michael Paquier <michael(dot)paquier(at)gmail(dot)com>
> wrote:
>>
>> On Mon, Aug 18, 2014 at 9:37 AM, Peter Eisentraut <peter_e(at)gmx(dot)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 Mon, Aug 18, 2014 at 7:50 PM, Heikki Linnakangas <hlinnakangas(at)vmware(dot)com>
wrote:
>
> 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?

Good question.

>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.

With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com

On Tue, Aug 19, 2014 at 9:51 AM, Amit Kapila <amit(dot)kapila16(at)gmail(dot)com>
wrote:
> On Mon, Aug 18, 2014 at 7:50 PM, Heikki Linnakangas <
hlinnakangas(at)vmware(dot)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.

Do you see any need to change below line in docs:
"If a tablespace is relocated in this way, the symbolic links inside the
main data directory are updated to point to the new location."
Refer link:
http://www.postgresql.org/docs/devel/static/app-pgbasebackup.html

I was not sure whether docs need any change, so kept them
intact.

With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com

On Thu, Aug 21, 2014 at 3:44 PM, Amit Kapila <amit(dot)kapila16(at)gmail(dot)com>
wrote:
>
> On Tue, Aug 19, 2014 at 9:51 AM, Amit Kapila <amit(dot)kapila16(at)gmail(dot)com>
wrote:
> > On Mon, Aug 18, 2014 at 7:50 PM, Heikki Linnakangas <
hlinnakangas(at)vmware(dot)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.

With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com

On 08/22/2014 07:08 AM, Amit Kapila wrote:
> On Thu, Aug 21, 2014 at 3:44 PM, Amit Kapila <amit(dot)kapila16(at)gmail(dot)com>
> wrote:
>>
>> On Tue, Aug 19, 2014 at 9:51 AM, Amit Kapila <amit(dot)kapila16(at)gmail(dot)com>
> wrote:
>>> On Mon, Aug 18, 2014 at 7:50 PM, Heikki Linnakangas <
> hlinnakangas(at)vmware(dot)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 Fri, Aug 22, 2014 at 1:00 PM, Heikki Linnakangas <hlinnakangas(at)vmware(dot)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).

With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com

On 08/22/2014 11:35 AM, Amit Kapila wrote:
> On Fri, Aug 22, 2014 at 1:00 PM, Heikki Linnakangas <hlinnakangas(at)vmware(dot)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