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
Attachments:
pg_basebackup_relocate_tablespace_v1.patchapplication/octet-stream; name=pg_basebackup_relocate_tablespace_v1.patchDownload+22-4
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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On Wed, Aug 13, 2014 at 9:20 PM, MauMau <maumau307@gmail.com> wrote:
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
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
Attachments:
pg_basebackup_relocate_tablespace_v2.patchapplication/octet-stream; name=pg_basebackup_relocate_tablespace_v2.patchDownload+23-5
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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On Fri, Aug 15, 2014 at 1:03 PM, MauMau <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.
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@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.
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
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:
/messages/by-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
Attachments:
20140818_basebackup_tbspace_fix.patchtext/x-patch; charset=US-ASCII; name=20140818_basebackup_tbspace_fix.patchDownload+1-1
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:
/messages/by-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.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:
/messages/by-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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On Mon, Aug 18, 2014 at 7:50 PM, Heikki Linnakangas <hlinnakangas@vmware.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.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.
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
Attachments:
pg_basebackup_relocate_tablespace_v3.patchapplication/octet-stream; name=pg_basebackup_relocate_tablespace_v3.patchDownload+13-41
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.
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com
Attachments:
pg_basebackup_relocate_tablespace_v4.patchapplication/octet-stream; name=pg_basebackup_relocate_tablespace_v4.patchDownload+12-41
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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
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).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@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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers