Re: [HACKERS] option -T in pg_basebackup doesn't work on windows
On 08/22/2014 07:08 AM, Amit Kapila wrote: On Thu, Aug 21, 2014 at 3:44 PM, Amit Kapila amit.kapil...@gmail.com wrote: On Tue, Aug 19, 2014 at 9:51 AM, Amit Kapila amit.kapil...@gmail.com wrote: On Mon, Aug 18, 2014 at 7:50 PM, Heikki Linnakangas hlinnakan...@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
Re: [HACKERS] option -T in pg_basebackup doesn't work on windows
On Fri, Aug 22, 2014 at 1:00 PM, Heikki Linnakangas hlinnakan...@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
Re: [HACKERS] option -T in pg_basebackup doesn't work on windows
On 08/22/2014 11:35 AM, Amit Kapila wrote: On Fri, Aug 22, 2014 at 1:00 PM, Heikki Linnakangas hlinnakan...@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
Re: [HACKERS] option -T in pg_basebackup doesn't work on windows
On Tue, Aug 19, 2014 at 9:51 AM, Amit Kapila amit.kapil...@gmail.com wrote: On Mon, Aug 18, 2014 at 7:50 PM, Heikki Linnakangas hlinnakan...@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 pg_basebackup_relocate_tablespace_v3.patch Description: Binary data -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] option -T in pg_basebackup doesn't work on windows
On Thu, Aug 21, 2014 at 3:44 PM, Amit Kapila amit.kapil...@gmail.com wrote: On Tue, Aug 19, 2014 at 9:51 AM, Amit Kapila amit.kapil...@gmail.com wrote: On Mon, Aug 18, 2014 at 7:50 PM, Heikki Linnakangas hlinnakan...@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 pg_basebackup_relocate_tablespace_v4.patch Description: Binary data -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] option -T in pg_basebackup doesn't work on windows
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
Re: [HACKERS] option -T in pg_basebackup doesn't work on windows
On Mon, Aug 18, 2014 at 7:50 PM, Heikki Linnakangas hlinnakan...@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
Re: [HACKERS] option -T in pg_basebackup doesn't work on windows
On 8/16/14 8:46 AM, Amit Kapila wrote: On Fri, Aug 15, 2014 at 1:03 PM, MauMau maumau...@gmail.com mailto:maumau...@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
Re: [HACKERS] option -T in pg_basebackup doesn't work on windows
On Mon, Aug 18, 2014 at 9:37 AM, Peter Eisentraut pete...@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_6wlbq1re...@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
diff --git a/src/bin/pg_basebackup/pg_basebackup.c b/src/bin/pg_basebackup/pg_basebackup.c
index 3d26e22..ef7cd94 100644
--- a/src/bin/pg_basebackup/pg_basebackup.c
+++ b/src/bin/pg_basebackup/pg_basebackup.c
@@ -1119,7 +1119,7 @@ update_tablespace_symlink(Oid oid, const char *old_dir)
if (strcmp(old_dir, new_dir) != 0)
{
- char *linkloc = psprintf(%s/pg_tblspc/%d, basedir, oid);
+ char *linkloc = psprintf(%s/pg_tblspc/%u, basedir, oid);
if (unlink(linkloc) != 0 errno != ENOENT)
{
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] option -T in pg_basebackup doesn't work on windows
On Mon, Aug 18, 2014 at 7:08 AM, Michael Paquier michael.paqu...@gmail.com wrote: On Mon, Aug 18, 2014 at 9:37 AM, Peter Eisentraut pete...@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_6wlbq1re...@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
Re: [HACKERS] option -T in pg_basebackup doesn't work on windows
On Mon, Aug 18, 2014 at 11:51 AM, Amit Kapila amit.kapil...@gmail.com wrote: On Mon, Aug 18, 2014 at 7:08 AM, Michael Paquier michael.paqu...@gmail.com wrote: On Mon, Aug 18, 2014 at 9:37 AM, Peter Eisentraut pete...@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_6wlbq1re...@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
Re: [HACKERS] option -T in pg_basebackup doesn't work on windows
On Fri, Aug 15, 2014 at 1:03 PM, MauMau maumau...@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
Re: [HACKERS] option -T in pg_basebackup doesn't work on windows
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
Re: [HACKERS] option -T in pg_basebackup doesn't work on windows
On Wed, Aug 13, 2014 at 9:20 PM, MauMau maumau...@gmail.com wrote: From: Amit Kapila amit.kapil...@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 pg_basebackup_relocate_tablespace_v2.patch Description: Binary data -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] option -T in pg_basebackup doesn't work on windows
From: Amit Kapila amit.kapil...@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
Re: [HACKERS] option -T in pg_basebackup doesn't work on windows
On Thu, Aug 14, 2014 at 12:50 AM, MauMau maumau...@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
