Re: [HACKERS] New option for pg_basebackup, to specify a different directory for pg_xlog

2014-02-15 Thread Peter Eisentraut
On 2/9/14, 11:16 PM, Haribabu Kommi wrote:
 I also felt a lot of work for little benefit but as of now I am not able
 to find an easier solution to handle this problem. 
 can we handle the same later if it really requires?

I think we leave everything as is.



-- 
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] New option for pg_basebackup, to specify a different directory for pg_xlog

2014-02-09 Thread Haribabu Kommi
On Sat, Feb 8, 2014 at 12:10 PM, Peter Eisentraut pete...@gmx.net wrote:

 On 1/29/14, 7:37 PM, Haribabu Kommi wrote:
 
  On Tue, Jan 28, 2014 at 1:17 PM, Peter Eisentraut wrote:
 
  On 11/30/13, 6:59 AM, Haribabu kommi wrote:
   To detect provided data and xlog directories are same or not, I
  reused the
   Existing make_absolute_path() code as follows.
 
  I note that initdb does not detect whether the data and xlog
 directories
  are the same.  I think there is no point in addressing this only in
  pg_basebackup.  If we want to forbid it, it should be done in initdb
  foremost.
 
   Thanks for pointing it. if the following approach is fine for
  identifying the identical directories
   then I will do the same for initdb also.

 I wouldn't bother.  It's a lot of work for little benefit.  Any mistake
 a user would make can easily be fixed.


I also felt a lot of work for little benefit but as of now I am not able to
find an easier solution to handle this problem.
can we handle the same later if it really requires?

-- 
Regards,
Hari Babu
Fujitsu Australia Software Technology


Re: [HACKERS] New option for pg_basebackup, to specify a different directory for pg_xlog

2014-02-07 Thread Peter Eisentraut
On 1/29/14, 7:37 PM, Haribabu Kommi wrote:
 
 On Tue, Jan 28, 2014 at 1:17 PM, Peter Eisentraut wrote:
 
 On 11/30/13, 6:59 AM, Haribabu kommi wrote:
  To detect provided data and xlog directories are same or not, I
 reused the
  Existing make_absolute_path() code as follows.
 
 I note that initdb does not detect whether the data and xlog directories
 are the same.  I think there is no point in addressing this only in
 pg_basebackup.  If we want to forbid it, it should be done in initdb
 foremost.
 
  Thanks for pointing it. if the following approach is fine for
 identifying the identical directories
  then I will do the same for initdb also.

I wouldn't bother.  It's a lot of work for little benefit.  Any mistake
a user would make can easily be fixed.

 I'm not sure it's worth the trouble, but if I were to do it, I'd just
 stat() the two directories and compare their inodes.  That seems much
 easier and more robust than comparing path strings
 
 stat() is having problems in windows, because of that reason the patch is
 written to identify the directories with string comparison.

If stat() is having problems on Windows, then those problems would need
to be addressed.

I don't think a string comparison is going to be reliable.  It can
easily be tricked by using multiple slashes for example, or various
kinds of links or bind mounts.  You'd need to put in an awful lot of
work, and it still wouldn't work all the time.



-- 
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] New option for pg_basebackup, to specify a different directory for pg_xlog

2014-01-31 Thread Fujii Masao
On Thu, Jan 30, 2014 at 9:37 AM, Haribabu Kommi
kommi.harib...@gmail.com wrote:

 On Tue, Jan 28, 2014 at 1:17 PM, Peter Eisentraut wrote:

 On 11/30/13, 6:59 AM, Haribabu kommi wrote:
  To detect provided data and xlog directories are same or not, I reused
  the
  Existing make_absolute_path() code as follows.

 I note that initdb does not detect whether the data and xlog directories
 are the same.  I think there is no point in addressing this only in
 pg_basebackup.  If we want to forbid it, it should be done in initdb
 foremost.


  Thanks for pointing it. if the following approach is fine for identifying
 the identical directories
  then I will do the same for initdb also.

I'm feeling the similar way as Peter. And, ISTM that we need much changes of
source though its benefit is small

Regards,

-- 
Fujii Masao


-- 
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] New option for pg_basebackup, to specify a different directory for pg_xlog

2014-01-29 Thread Haribabu Kommi
On Tue, Jan 28, 2014 at 1:17 PM, Peter Eisentraut wrote:

 On 11/30/13, 6:59 AM, Haribabu kommi wrote:
  To detect provided data and xlog directories are same or not, I reused
 the
  Existing make_absolute_path() code as follows.

 I note that initdb does not detect whether the data and xlog directories
 are the same.  I think there is no point in addressing this only in
 pg_basebackup.  If we want to forbid it, it should be done in initdb
 foremost.


 Thanks for pointing it. if the following approach is fine for identifying
the identical directories
 then I will do the same for initdb also.


 I'm not sure it's worth the trouble, but if I were to do it, I'd just
 stat() the two directories and compare their inodes.  That seems much
 easier and more robust than comparing path strings


stat() is having problems in windows, because of that reason the patch is
written to identify the directories with string comparison.

Regards,
Hari Babu


Re: [HACKERS] New option for pg_basebackup, to specify a different directory for pg_xlog

2014-01-27 Thread Peter Eisentraut
On 11/30/13, 6:59 AM, Haribabu kommi wrote:
 To detect provided data and xlog directories are same or not, I reused the
 Existing make_absolute_path() code as follows.

I note that initdb does not detect whether the data and xlog directories
are the same.  I think there is no point in addressing this only in
pg_basebackup.  If we want to forbid it, it should be done in initdb
foremost.

I'm not sure it's worth the trouble, but if I were to do it, I'd just
stat() the two directories and compare their inodes.  That seems much
easier and more robust than comparing path strings.



-- 
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] New option for pg_basebackup, to specify a different directory for pg_xlog

2013-12-20 Thread Haribabu kommi
On 20 December 2013 02:02 Bruce Momjian wrote:
 On Thu, Dec 19, 2013 at 05:14:50AM +, Haribabu kommi wrote:
  On 19 December 2013 05:31 Bruce Momjian wrote:
   On Wed, Dec 11, 2013 at 10:22:32AM +, Haribabu kommi wrote:
The make_absolute_path() function moving to port is changed in
   similar
way as Bruce Momjian approach. The psprintf is used to store the
   error
string which Occurred in the function. But psprintf is not used
for storing the absolute path As because it is giving problems in
freeing
   the allocated memory in SelectConfigFiles.
Because the same memory is allocated in a different code branch
from
   guc_malloc.
   
After adding the make_absolute_path() function with psprintf
 stuff
in path.c file It is giving linking problem in compilation of
ecpg. I am
   not able to find the problem.
So I added another file abspath.c in port which contains these
 two
   functions.
  
   What errors are you seeing?
 
  If I move the make_absolute_path function from abspath.c to path.c, I
  was getting following linking errors while compiling ecpg.
 
  ../../../../src/port/libpgport.a(path.o): In function
 `make_absolute_path':
  /home/hari/postgres/src/port/path.c:795: undefined reference to
 `psprintf'
  /home/hari/postgres/src/port/path.c:809: undefined reference to
 `psprintf'
  /home/hari/postgres/src/port/path.c:818: undefined reference to
 `psprintf'
  /home/hari/postgres/src/port/path.c:830: undefined reference to
 `psprintf'
  collect2: ld returned 1 exit status
  make[4]: *** [ecpg] Error 1
  make[3]: *** [all-preproc-recurse] Error 2
  make[2]: *** [all-ecpg-recurse] Error 2
  make[1]: *** [all-interfaces-recurse] Error 2
  make: *** [all-src-recurse] Error 2
 
 You didn't show the actual command that is generating the error, but I
 assume it is linking ecpg, not creating libecpg.  I think the issue is
 that path.c is specially handled when it is included in libecpg.  Here
 is a comment from the libecpg Makefile:
 
   # We use some port modules verbatim, but since we need to
   # compile with appropriate options to build a shared lib, we
 can't
   # necessarily use the same object files as the backend uses.
 Instead,
   # symlink the source files in here and build our own object file.
 
 My guess is that libecpg isn't marked as linking to libpgcommon, and
 when you called psprintf in path.c, it added a libpgcommon link
 requirement.
 
 My guess is that if you compiled common/psprintf.c like port/path.c in
 libecpg's Makefile, it would link fine.

Sorry for not mentioning the command, yes it is giving problem while linking 
ecpg.

From the compilation I observed as libpgcommon is linking while building ecpg.
I tested the same by using psprintf directly in ecpg code.

I modified the libecpg's Makefile as suggested by you which is attached in the 
mail,
Still the errors are occurring. Please let me know is there anything missed?

Regards,
Hari babu.




make_abs_path_v3.patch
Description: make_abs_path_v3.patch

-- 
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] New option for pg_basebackup, to specify a different directory for pg_xlog

2013-12-20 Thread Alvaro Herrera
Haribabu kommi escribió:

 From the compilation I observed as libpgcommon is linking while building ecpg.
 I tested the same by using psprintf directly in ecpg code.
 
 I modified the libecpg's Makefile as suggested by you which is attached in 
 the mail,
 Still the errors are occurring. Please let me know is there anything missed?

I don't know what's the cause of the error you're seeing, but IIRC you
can't have a file in src/port depend on src/common functionality (which
psprintf is IIRC).  So you need to fix things up so that the psprintf()
doesn't occur in src/port at all.  Not sure what's the best way to go
about this; perhaps the whole new function should be in src/common; or
perhaps you need part of it in src/port and the bits with the funny
error reporting in src/common, where psprintf can be called without
issue.

-- 
Álvaro Herrerahttp://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training  Services


-- 
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] New option for pg_basebackup, to specify a different directory for pg_xlog

2013-12-19 Thread Bruce Momjian
On Thu, Dec 19, 2013 at 05:14:50AM +, Haribabu kommi wrote:
 On 19 December 2013 05:31 Bruce Momjian wrote:
  On Wed, Dec 11, 2013 at 10:22:32AM +, Haribabu kommi wrote:
   The make_absolute_path() function moving to port is changed in
  similar
   way as Bruce Momjian approach. The psprintf is used to store the
  error
   string which Occurred in the function. But psprintf is not used for
   storing the absolute path As because it is giving problems in freeing
  the allocated memory in SelectConfigFiles.
   Because the same memory is allocated in a different code branch from
  guc_malloc.
  
   After adding the make_absolute_path() function with psprintf stuff in
   path.c file It is giving linking problem in compilation of ecpg. I am
  not able to find the problem.
   So I added another file abspath.c in port which contains these two
  functions.
  
  What errors are you seeing?
 
 If I move the make_absolute_path function from abspath.c to path.c,
 I was getting following linking errors while compiling ecpg.
 
 ../../../../src/port/libpgport.a(path.o): In function `make_absolute_path':
 /home/hari/postgres/src/port/path.c:795: undefined reference to `psprintf'
 /home/hari/postgres/src/port/path.c:809: undefined reference to `psprintf'
 /home/hari/postgres/src/port/path.c:818: undefined reference to `psprintf'
 /home/hari/postgres/src/port/path.c:830: undefined reference to `psprintf'
 collect2: ld returned 1 exit status
 make[4]: *** [ecpg] Error 1
 make[3]: *** [all-preproc-recurse] Error 2
 make[2]: *** [all-ecpg-recurse] Error 2
 make[1]: *** [all-interfaces-recurse] Error 2
 make: *** [all-src-recurse] Error 2

You didn't show the actual command that is generating the error, but I
assume it is linking ecpg, not creating libecpg.  I think the issue is
that path.c is specially handled when it is included in libecpg.  Here
is a comment from the libecpg Makefile:

# We use some port modules verbatim, but since we need to
# compile with appropriate options to build a shared lib, we can't
# necessarily use the same object files as the backend uses. Instead,
# symlink the source files in here and build our own object file.

My guess is that libecpg isn't marked as linking to libpgcommon, and
when you called psprintf in path.c, it added a libpgcommon link
requirement.

My guess is that if you compiled common/psprintf.c like port/path.c in
libecpg's Makefile, it would link fine.

-- 
  Bruce Momjian  br...@momjian.ushttp://momjian.us
  EnterpriseDB http://enterprisedb.com

  + Everyone has their own god. +


-- 
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] New option for pg_basebackup, to specify a different directory for pg_xlog

2013-12-18 Thread Bruce Momjian
On Wed, Dec 11, 2013 at 10:22:32AM +, Haribabu kommi wrote:
 The make_absolute_path() function moving to port is changed in similar way as
 Bruce Momjian approach. The psprintf is used to store the error string which
 Occurred in the function. But psprintf is not used for storing the absolute 
 path
 As because it is giving problems in freeing the allocated memory in 
 SelectConfigFiles.
 Because the same memory is allocated in a different code branch from 
 guc_malloc.
 
 After adding the make_absolute_path() function with psprintf stuff in path.c 
 file
 It is giving linking problem in compilation of ecpg. I am not able to find 
 the problem.
 So I added another file abspath.c in port which contains these two functions.

What errors are you seeing?

-- 
  Bruce Momjian  br...@momjian.ushttp://momjian.us
  EnterpriseDB http://enterprisedb.com

  + Everyone has their own god. +


-- 
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] New option for pg_basebackup, to specify a different directory for pg_xlog

2013-12-18 Thread Haribabu kommi
On 19 December 2013 05:31 Bruce Momjian wrote:
 On Wed, Dec 11, 2013 at 10:22:32AM +, Haribabu kommi wrote:
  The make_absolute_path() function moving to port is changed in
 similar
  way as Bruce Momjian approach. The psprintf is used to store the
 error
  string which Occurred in the function. But psprintf is not used for
  storing the absolute path As because it is giving problems in freeing
 the allocated memory in SelectConfigFiles.
  Because the same memory is allocated in a different code branch from
 guc_malloc.
 
  After adding the make_absolute_path() function with psprintf stuff in
  path.c file It is giving linking problem in compilation of ecpg. I am
 not able to find the problem.
  So I added another file abspath.c in port which contains these two
 functions.
 
 What errors are you seeing?

If I move the make_absolute_path function from abspath.c to path.c,
I was getting following linking errors while compiling ecpg.

../../../../src/port/libpgport.a(path.o): In function `make_absolute_path':
/home/hari/postgres/src/port/path.c:795: undefined reference to `psprintf'
/home/hari/postgres/src/port/path.c:809: undefined reference to `psprintf'
/home/hari/postgres/src/port/path.c:818: undefined reference to `psprintf'
/home/hari/postgres/src/port/path.c:830: undefined reference to `psprintf'
collect2: ld returned 1 exit status
make[4]: *** [ecpg] Error 1
make[3]: *** [all-preproc-recurse] Error 2
make[2]: *** [all-ecpg-recurse] Error 2
make[1]: *** [all-interfaces-recurse] Error 2
make: *** [all-src-recurse] Error 2


Regards,
Hari babu.


-- 
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] New option for pg_basebackup, to specify a different directory for pg_xlog

2013-12-11 Thread Haribabu kommi
On 10 December 2013 19:55 Alvaro Herrera wrote:
 Haribabu kommi escribió:
 
  To detect provided data and xlog directories are same or not, I
 reused
  the Existing make_absolute_path() code as follows.
 
  1. Moved the make_absolute_path() function from miscinit.c to path.c
  and Changed all error reporting functions. And also it returns NULL
  incase of any error.
 
  2. Added a new file called fe_path.c which contains
  make_absolute_path() only for frontend code.
 
 Whatever you do, please don't add #include lines to postgres_fe.h.  Add
 them to whatever .c files that need to include the new header, instead.
 (This results in a longer patch, yes, but that consideration shouldn't
 drive anything.  There is a desire to include as less headers as
 possible in each source file, and adding more include lines to
 postgres_fe.h means the new header will be included by every single
 frontend file, even those not in core.)
 
 See a nearby patch by Bruce Momjian to deal with getpwnam() and
 getpwuid() failures; perhaps the idea of returning an error string
 should be designed similarly in both these patches.  Also consider
 using the psprintf stuff, which works on both backend and frontend,
 avoiding malloc etc so that code can be shared by both frontend and
 backend, eliminating the duplicity.

The make_absolute_path() function moving to port is changed in similar way as
Bruce Momjian approach. The psprintf is used to store the error string which
Occurred in the function. But psprintf is not used for storing the absolute path
As because it is giving problems in freeing the allocated memory in 
SelectConfigFiles.
Because the same memory is allocated in a different code branch from guc_malloc.

After adding the make_absolute_path() function with psprintf stuff in path.c 
file
It is giving linking problem in compilation of ecpg. I am not able to find the 
problem.
So I added another file abspath.c in port which contains these two functions.

Updated patches are attached in the mail. Please provide your suggestions.

Regards,
Hari babu.


same_dir_error_v2.patch
Description: same_dir_error_v2.patch


make_abs_path_v2.patch
Description: make_abs_path_v2.patch

-- 
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] New option for pg_basebackup, to specify a different directory for pg_xlog

2013-12-10 Thread Alvaro Herrera
Haribabu kommi escribió:

 To detect provided data and xlog directories are same or not, I reused the
 Existing make_absolute_path() code as follows.
 
 1. Moved the make_absolute_path() function from miscinit.c to path.c and
 Changed all error reporting functions. And also it returns NULL incase of
 any error.
 
 2. Added a new file called fe_path.c which contains make_absolute_path()
 only for frontend code.

Whatever you do, please don't add #include lines to postgres_fe.h.  Add
them to whatever .c files that need to include the new header, instead.
(This results in a longer patch, yes, but that consideration shouldn't
drive anything.  There is a desire to include as less headers as
possible in each source file, and adding more include lines to
postgres_fe.h means the new header will be included by every single
frontend file, even those not in core.)

See a nearby patch by Bruce Momjian to deal with getpwnam() and
getpwuid() failures; perhaps the idea of returning an error string
should be designed similarly in both these patches.  Also consider using
the psprintf stuff, which works on both backend and frontend, avoiding
malloc etc so that code can be shared by both frontend and backend,
eliminating the duplicity.

Thanks,

-- 
Álvaro Herrerahttp://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training  Services


-- 
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] New option for pg_basebackup, to specify a different directory for pg_xlog

2013-11-30 Thread Haribabu kommi
On 27 November 2013 10:35 Fujii Masao wrote:
 On Wed, Nov 27, 2013 at 1:27 PM, Haribabu kommi
 haribabu.ko...@huawei.com wrote:
  On 26 November 2013 23:11 Fujii Masao wrote:
  On Wed, Nov 20, 2013 at 7:43 PM, Haribabu kommi
  haribabu.ko...@huawei.com wrote:
   I tried using of stat'ing in two directories, which is having a
  problem in windows.
   So modified old approach to detect limited errors. Updated patch
  attached.
   This will detect and throw an error in the following scenarios.
   1. pg_basebackup -D /home/data --xlogdir=/home/data 2.
   pg_basebackup -D data --xlogdir=/home/data -- home is the CWD 3.
   pg_basebackup -D ../data --xlogdir=/data -- home is the CWD
  
   Please let me know your suggestions.
 
  Checking only #1 and #2 cases looks sufficient to at least me. If
  we do that, we can refactor the patch so that it reuses
  make_absolute_path() instead of adding new functions, as pointed
in
  upthread. Anyway, what about committing the core patch (Updated
  version of core patch attached) first? Then, we can discuss the
 check logic more.
 
  Yes it is fine. The core patch attached in the mail is working fine.

 Okay. Committed!

To detect provided data and xlog directories are same or not, I reused the
Existing make_absolute_path() code as follows.

1. Moved the make_absolute_path() function from miscinit.c to path.c and
Changed all error reporting functions. And also it returns NULL incase of
any error.

2. Added a new file called fe_path.c which contains make_absolute_path()
only for frontend code.

The patches are attached in the mail for both approaches, please review and let
Me know your suggestions.

On top those patches, the error detection logic is added in pg_basebackup and 
the
Same is attached in the mail.

Regards,
Hari babu.


same_dir_error_v1.patch
Description: same_dir_error_v1.patch


frontend_make_abs_path_v1.patch
Description: frontend_make_abs_path_v1.patch


make_abs_path_v1.patch
Description: make_abs_path_v1.patch

-- 
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] New option for pg_basebackup, to specify a different directory for pg_xlog

2013-11-28 Thread Haribabu kommi
On 27 November 2013 10:35 Fujii Masao wrote:
 On Wed, Nov 27, 2013 at 1:27 PM, Haribabu kommi
 haribabu.ko...@huawei.com wrote:
  On 26 November 2013 23:11 Fujii Masao wrote:
  On Wed, Nov 20, 2013 at 7:43 PM, Haribabu kommi
  haribabu.ko...@huawei.com wrote:
   I tried using of stat'ing in two directories, which is having a
  problem in windows.
   So modified old approach to detect limited errors. Updated patch
  attached.
   This will detect and throw an error in the following scenarios.
   1. pg_basebackup -D /home/data --xlogdir=/home/data 2.
   pg_basebackup -D data --xlogdir=/home/data -- home is the CWD 3.
   pg_basebackup -D ../data --xlogdir=/data -- home is the CWD
  
   Please let me know your suggestions.
 
  Checking only #1 and #2 cases looks sufficient to at least me. If we
  do that, we can refactor the patch so that it reuses
  make_absolute_path() instead of adding new functions, as pointed in
  upthread. Anyway, what about committing the core patch (Updated
  version of core patch attached) first? Then, we can discuss the
 check logic more.
 
  Yes it is fine. The core patch attached in the mail is working fine.
 
 Okay. Committed!

Thanks. Regarding refactoring the code to use make_absolute_path for checking
the data and xlog directories are same or not, this function exists in two 
places
1. pg_regress.c 2.miscinit.c.

Solution -1: if we move the backend function to path.c, it cannot use the 
ereport
Functions. So it may cause problem to backends.

Solution -2: Rename the backend function as make_absolute_path_internal and add 
a
New function to the path.c

Please let me know your opinion on the same or is there anything I missed?

Regards,
Hari babu.


-- 
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] New option for pg_basebackup, to specify a different directory for pg_xlog

2013-11-26 Thread Fujii Masao
On Wed, Nov 20, 2013 at 7:43 PM, Haribabu kommi
haribabu.ko...@huawei.com wrote:
 On 19 November 2013 19:12 Fujii Masao wrote:
 On Tue, Nov 19, 2013 at 9:14 PM, Haribabu kommi
 haribabu.ko...@huawei.com wrote:
  On 18 November 2013 23:30 Fujii Masao wrote:
  On Tue, Nov 19, 2013 at 12:01 AM, Haribabu kommi
  haribabu.ko...@huawei.com wrote:
 
  Thanks for newer version of the patch!
 
  I found that the empty base directory is created and remains even
  when the same directory is specified in both -D and --xlogdir and
 the
  error occurs.
  I think that it's
  better to throw an error in that case before creating any new
 directory.
  Thought?
 
  By creating the base directory only the patch finds whether provided
  base and Xlog directories are same or not? To solve this problem
  following options are possible
 
  1. No problem as it is just an empty base directory, so it can be
 reused in the next time
 Leave it as it is.
  2. Once the error is identified, the base directory can be deleted.
  3. write a new function to remove the parent references from the
 provided path to identify
 the absolute path used for detecting base and Xlog directories are
 same or not?
 
  Please provide your suggestions.
 
  +xlogdir = get_absolute_path(xlog_dir);
 
  xlog_dir must be an absolute path. ISTM we don't do the above. No?
 
  It is required. As user can provide the path as
 /home/installation/bin/../bin/data.
  The provided path is considered as absolute path only but while
  comparing the same With data directory path it will not match.

 Okay, maybe I understand you. In order to know the real absolute path,
 basically we need to create the directory specified in --xlogdir,
 change the working directory to it and calculate the parent path.
 You're worried about the cases as follows, for example.
 Right?

 * path containing .. like /aaa/bbb/../ccc is specified in --xlogdir
 * symbolic link is specified in --xlogdir

 On the second thought, I'm thinking that it might be overkill to add
 such not simple code for that small benefit.

 I tried using of stat'ing in two directories, which is having a problem in 
 windows.
 So modified old approach to detect limited errors. Updated patch attached.
 This will detect and throw an error in the following scenarios.
 1. pg_basebackup -D /home/data --xlogdir=/home/data
 2. pg_basebackup -D data --xlogdir=/home/data -- home is the CWD
 3. pg_basebackup -D ../data --xlogdir=/data -- home is the CWD

 Please let me know your suggestions.

Checking only #1 and #2 cases looks sufficient to at least me. If we do that,
we can refactor the patch so that it reuses make_absolute_path() instead of
adding new functions, as pointed in upthread. Anyway, what about committing
the core patch (Updated version of core patch attached) first? Then, we can
discuss the check logic more.

Regards,

-- 
Fujii Masao
*** a/doc/src/sgml/ref/pg_basebackup.sgml
--- b/doc/src/sgml/ref/pg_basebackup.sgml
***
*** 203,208  PostgreSQL documentation
--- 203,220 
   /varlistentry
  
   varlistentry
+   termoption--xlogdir=replaceable class=parameterxlogdir/replaceable/option/term
+   listitem
+para
+ Specifies the location for the transaction log directory. 
+ replaceablexlogdir/replaceable must be an absolute path.
+ The transaction log directory can only be specified when
+ the backup is in plain mode.
+/para
+   /listitem
+  /varlistentry
+ 
+  varlistentry
termoption-x/option/term
termoption--xlog/option/term
listitem
*** a/src/bin/pg_basebackup/pg_basebackup.c
--- b/src/bin/pg_basebackup/pg_basebackup.c
***
*** 35,40 
--- 35,41 
  
  /* Global options */
  char	   *basedir = NULL;
+ static char *xlog_dir = ;
  char		format = 'p';		/* p(lain)/t(ar) */
  char	   *label = pg_basebackup base backup;
  bool		showprogress = false;
***
*** 115,120  usage(void)
--- 116,122 
  	printf(_(  -x, --xlog include required WAL files in backup (fetch mode)\n));
  	printf(_(  -X, --xlog-method=fetch|stream\n
  			  include required WAL files with specified method\n));
+ 	printf(_(  --xlogdir=XLOGDIR  location for the transaction log directory\n));
  	printf(_(  -z, --gzip compress tar output\n));
  	printf(_(  -Z, --compress=0-9 compress tar output with given compression level\n));
  	printf(_(\nGeneral options:\n));
***
*** 980,989  ReceiveAndUnpackTarFile(PGconn *conn, PGresult *res, int rownum)
  	{
  		/*
  		 * When streaming WAL, pg_xlog will have been created
! 		 * by the wal receiver process, so just ignore failure
! 		 * on that.
  		 */
! 		if (!streamwal || strcmp(filename + strlen(filename) - 8, /pg_xlog) != 0)
  		{
  			fprintf(stderr,
  			_(%s: could not create directory \%s\: %s\n),
--- 982,995 
  	{
  		/*
  	

Re: [HACKERS] New option for pg_basebackup, to specify a different directory for pg_xlog

2013-11-26 Thread Haribabu kommi
On 26 November 2013 23:11 Fujii Masao wrote:
 On Wed, Nov 20, 2013 at 7:43 PM, Haribabu kommi
 haribabu.ko...@huawei.com wrote:
  I tried using of stat'ing in two directories, which is having a
 problem in windows.
  So modified old approach to detect limited errors. Updated patch
 attached.
  This will detect and throw an error in the following scenarios.
  1. pg_basebackup -D /home/data --xlogdir=/home/data 2. pg_basebackup
  -D data --xlogdir=/home/data -- home is the CWD 3. pg_basebackup -D
  ../data --xlogdir=/data -- home is the CWD
 
  Please let me know your suggestions.
 
 Checking only #1 and #2 cases looks sufficient to at least me. If we do
 that, we can refactor the patch so that it reuses make_absolute_path()
 instead of adding new functions, as pointed in upthread. Anyway, what
 about committing the core patch (Updated version of core patch attached)
 first? Then, we can discuss the check logic more.

Yes it is fine. The core patch attached in the mail is working fine.

I will provide another patch with check logic by incorporating your suggestions.

Regards,
Hari babu.


-- 
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] New option for pg_basebackup, to specify a different directory for pg_xlog

2013-11-26 Thread Fujii Masao
On Wed, Nov 27, 2013 at 1:27 PM, Haribabu kommi
haribabu.ko...@huawei.com wrote:
 On 26 November 2013 23:11 Fujii Masao wrote:
 On Wed, Nov 20, 2013 at 7:43 PM, Haribabu kommi
 haribabu.ko...@huawei.com wrote:
  I tried using of stat'ing in two directories, which is having a
 problem in windows.
  So modified old approach to detect limited errors. Updated patch
 attached.
  This will detect and throw an error in the following scenarios.
  1. pg_basebackup -D /home/data --xlogdir=/home/data 2. pg_basebackup
  -D data --xlogdir=/home/data -- home is the CWD 3. pg_basebackup -D
  ../data --xlogdir=/data -- home is the CWD
 
  Please let me know your suggestions.

 Checking only #1 and #2 cases looks sufficient to at least me. If we do
 that, we can refactor the patch so that it reuses make_absolute_path()
 instead of adding new functions, as pointed in upthread. Anyway, what
 about committing the core patch (Updated version of core patch attached)
 first? Then, we can discuss the check logic more.

 Yes it is fine. The core patch attached in the mail is working fine.

Okay. Committed!

Regards,

-- 
Fujii Masao


-- 
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] New option for pg_basebackup, to specify a different directory for pg_xlog

2013-11-20 Thread Haribabu kommi
On 19 November 2013 19:12 Fujii Masao wrote:
 On Tue, Nov 19, 2013 at 9:14 PM, Haribabu kommi
 haribabu.ko...@huawei.com wrote:
  On 18 November 2013 23:30 Fujii Masao wrote:
  On Tue, Nov 19, 2013 at 12:01 AM, Haribabu kommi
  haribabu.ko...@huawei.com wrote:
 
  Thanks for newer version of the patch!
 
  I found that the empty base directory is created and remains even
  when the same directory is specified in both -D and --xlogdir and
 the
  error occurs.
  I think that it's
  better to throw an error in that case before creating any new
 directory.
  Thought?
 
  By creating the base directory only the patch finds whether provided
  base and Xlog directories are same or not? To solve this problem
  following options are possible
 
  1. No problem as it is just an empty base directory, so it can be
 reused in the next time
 Leave it as it is.
  2. Once the error is identified, the base directory can be deleted.
  3. write a new function to remove the parent references from the
 provided path to identify
 the absolute path used for detecting base and Xlog directories are
 same or not?
 
  Please provide your suggestions.
 
  +xlogdir = get_absolute_path(xlog_dir);
 
  xlog_dir must be an absolute path. ISTM we don't do the above. No?
 
  It is required. As user can provide the path as
 /home/installation/bin/../bin/data.
  The provided path is considered as absolute path only but while
  comparing the same With data directory path it will not match.
 
 Okay, maybe I understand you. In order to know the real absolute path,
 basically we need to create the directory specified in --xlogdir,
 change the working directory to it and calculate the parent path.
 You're worried about the cases as follows, for example.
 Right?
 
 * path containing .. like /aaa/bbb/../ccc is specified in --xlogdir
 * symbolic link is specified in --xlogdir
 
 On the second thought, I'm thinking that it might be overkill to add
 such not simple code for that small benefit.

I tried using of stat'ing in two directories, which is having a problem in 
windows.
So modified old approach to detect limited errors. Updated patch attached. 
This will detect and throw an error in the following scenarios.
1. pg_basebackup -D /home/data --xlogdir=/home/data
2. pg_basebackup -D data --xlogdir=/home/data -- home is the CWD
3. pg_basebackup -D ../data --xlogdir=/data -- home is the CWD

Please let me know your suggestions.

Regards,
Hari babu.


UserSpecifiedxlogDir_v6.patch
Description: UserSpecifiedxlogDir_v6.patch

-- 
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] New option for pg_basebackup, to specify a different directory for pg_xlog

2013-11-20 Thread Gavin Flower

On 20/11/13 23:43, Haribabu kommi wrote:

On 19 November 2013 19:12 Fujii Masao wrote:

On Tue, Nov 19, 2013 at 9:14 PM, Haribabu kommi
haribabu.ko...@huawei.com wrote:

On 18 November 2013 23:30 Fujii Masao wrote:

On Tue, Nov 19, 2013 at 12:01 AM, Haribabu kommi
haribabu.ko...@huawei.com wrote:

Thanks for newer version of the patch!

I found that the empty base directory is created and remains even
when the same directory is specified in both -D and --xlogdir and

the

error occurs.
I think that it's
better to throw an error in that case before creating any new

directory.

Thought?

By creating the base directory only the patch finds whether provided
base and Xlog directories are same or not? To solve this problem
following options are possible

1. No problem as it is just an empty base directory, so it can be

reused in the next time

Leave it as it is.
2. Once the error is identified, the base directory can be deleted.
3. write a new function to remove the parent references from the

provided path to identify

the absolute path used for detecting base and Xlog directories are

same or not?

Please provide your suggestions.


+xlogdir = get_absolute_path(xlog_dir);

xlog_dir must be an absolute path. ISTM we don't do the above. No?

It is required. As user can provide the path as

/home/installation/bin/../bin/data.

The provided path is considered as absolute path only but while
comparing the same With data directory path it will not match.

Okay, maybe I understand you. In order to know the real absolute path,
basically we need to create the directory specified in --xlogdir,
change the working directory to it and calculate the parent path.
You're worried about the cases as follows, for example.
Right?

* path containing .. like /aaa/bbb/../ccc is specified in --xlogdir
* symbolic link is specified in --xlogdir

On the second thought, I'm thinking that it might be overkill to add
such not simple code for that small benefit.

I tried using of stat'ing in two directories, which is having a problem in 
windows.
So modified old approach to detect limited errors. Updated patch attached.
This will detect and throw an error in the following scenarios.
1. pg_basebackup -D /home/data --xlogdir=/home/data
2. pg_basebackup -D data --xlogdir=/home/data -- home is the CWD
3. pg_basebackup -D ../data --xlogdir=/data -- home is the CWD

Please let me know your suggestions.

Regards,
Hari babu.


I don't think Postgres on other systems should be hobbled by the 
limitations of Microsoft software!


If certain features of Postgres are either not available, or are 
available in a reduced form on Microsoft platforms, then this should be 
documented - might provide subtle hints to upgrade to Linux.



Cheers,
Gavin


Re: [HACKERS] New option for pg_basebackup, to specify a different directory for pg_xlog

2013-11-20 Thread Haribabu kommi
on 20 November 2013 23:44 Gavin Flower wrote:
On 20/11/13 23:43, Haribabu kommi wrote:

I tried using of stat'ing in two directories, which is having a problem in 
windows.

So modified old approach to detect limited errors. Updated patch attached.

This will detect and throw an error in the following scenarios.

1. pg_basebackup -D /home/data --xlogdir=/home/data

2. pg_basebackup -D data --xlogdir=/home/data -- home is the CWD

3. pg_basebackup -D ../data --xlogdir=/data -- home is the CWD


I don't think Postgres on other systems should be hobbled by the limitations 
of Microsoft software!
If certain features of Postgres are either not available, or are available in 
a reduced form on Microsoft platforms, then this should be documented - might 
provide subtle hints to upgrade to Linux.

The patch which sent in earlier mail provides the detection of base and xlog 
directories are same or not
In both windows and linux with a different way of identifying the same without 
stat'ing.
If other also agrees to add stat'ing then I will change the patch accordingly 
and document the behavior
Change for windows.

Regards,
Hari babu.



Re: [HACKERS] New option for pg_basebackup, to specify a different directory for pg_xlog

2013-11-19 Thread Haribabu kommi
On 18 November 2013 23:30 Fujii Masao wrote:
 On Tue, Nov 19, 2013 at 12:01 AM, Haribabu kommi
 haribabu.ko...@huawei.com wrote:
  On 18 November 2013 18:45 Fujii Masao wrote:
  On Mon, Nov 18, 2013 at 6:31 PM, Haribabu kommi
  haribabu.ko...@huawei.com wrote:
  
   On 18 November 2013 11:19 Haribabu kommi wrote:
   On 17 November 2013 00:55 Fujii Masao wrote:
On Sat, Nov 16, 2013 at 2:27 PM, Haribabu kommi
haribabu.ko...@huawei.com wrote:
 on 15 November 2013 17:26 Magnus Hagander wrote:

On Fri, Nov 15, 2013 at 12:10 PM, Haribabu kommi
haribabu.ko...@huawei.com wrote:

On 14 November 2013 23:59 Fujii Masao wrote:
 On Thu, Nov 14, 2013 at 9:08 PM, Haribabu kommi
 haribabu.ko...@huawei.com wrote:
  Please find attached the patch, for adding a new option
  for pg_basebackup, to specify a different directory for
 pg_xlog.

 Sounds good! Here are the review comments:
 Don't we need to prevent users from specifying the same
   directory
 in both --pgdata and --xlogdir?

I feel no need to prevent, even if user specifies both
--pgdata
   and
--xlogdir as same directory all the transaction log files
will be created in the base directory  instead of pg_xlog
 directory.



Given how easy it would be to prevent that, I think we should.
  It
would be  an easy misunderstanding to specify that when you
   actually
want it in  wherever/pg_xlog. Specifying that would be
redundant in the first place,  but people ca do that, but it

would also be very easy to do it by mistake, and you'd end up
with something that's really bad, including a recursive
 symlink.



 Presently with initdb also user can specify both data and
 xlog
 directories as same.

 To prevent the data directory and xlog directory as same,
 there is
   a
 way in windows (_fullpath api) to get absolute path from a
 relative path, but I didn't get any such possibilities in
 linux.

 I didn't find any other way to check it, if anyone have any
 idea regarding this please let me know.
   
What about make_absolute_path() in miscinit.c?
  
   The make_absoulte_patch() function gets the current working
  directory
   and adds The relative path to CWD, this is not giving proper
  absolute
   path.
  
   I have added a new function verify_data_and_xlog_dir_same() which
   will change the Current working directory to data directory and
   gets the CWD and the same way for transaction log directory.
   Compare the both data and xlog directories and throw an error.
   Please check it
  once.
  
   Is there any other way to identify that both data and xlog
   directories are pointing to the same Instead of comparing both
  absolute paths?
  
   Updated patch attached in the mail.
  
   Failure when the xlogdir doesn't exist is fixed in the updated
   patch
  attached in the mail.
 
  Thanks for updating the patch!
 
  With the patch, when I specify the same directory in both -D and --
  xlogdir, I got the following error.
 
  $ cd /tmp
  $ pg_basebackup -D hoge --xlogdir=/tmp/hoge -X fetch
  pg_basebackup: could not change directory to hoge: No such file or
  directory
 
  Thanks. After finding the xlog directory absolute path returning back
  to current working Directory is missed, because of this reason it is
  failing while finding the absolute Path for data directory. Apart
 from this change the code is reorganized a bit.
 
  Updated patch is attached in the mail. Please review it once.
 
 Thanks for newer version of the patch!
 
 I found that the empty base directory is created and remains even when
 the same directory is specified in both -D and --xlogdir and the error
 occurs.
 I think that it's
 better to throw an error in that case before creating any new directory.
 Thought?

By creating the base directory only the patch finds whether provided base and
Xlog directories are same or not? To solve this problem following options are 
possible

1. No problem as it is just an empty base directory, so it can be reused in the 
next time
   Leave it as it is.
2. Once the error is identified, the base directory can be deleted.
3. write a new function to remove the parent references from the provided path 
to identify
   the absolute path used for detecting base and Xlog directories are same or 
not? 

Please provide your suggestions.

 +xlogdir = get_absolute_path(xlog_dir);
 
 xlog_dir must be an absolute path. ISTM we don't do the above. No?

It is required. As user can provide the path as 
/home/installation/bin/../bin/data.
The provided path is considered as absolute path only but while comparing the 
same
With data directory path it will not match.

Regards,
Hari babu.


-- 
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] New option for pg_basebackup, to specify a different directory for pg_xlog

2013-11-19 Thread Fujii Masao
On Tue, Nov 19, 2013 at 9:14 PM, Haribabu kommi
haribabu.ko...@huawei.com wrote:
 On 18 November 2013 23:30 Fujii Masao wrote:
 On Tue, Nov 19, 2013 at 12:01 AM, Haribabu kommi
 haribabu.ko...@huawei.com wrote:
  On 18 November 2013 18:45 Fujii Masao wrote:
  On Mon, Nov 18, 2013 at 6:31 PM, Haribabu kommi
  haribabu.ko...@huawei.com wrote:
  
   On 18 November 2013 11:19 Haribabu kommi wrote:
   On 17 November 2013 00:55 Fujii Masao wrote:
On Sat, Nov 16, 2013 at 2:27 PM, Haribabu kommi
haribabu.ko...@huawei.com wrote:
 on 15 November 2013 17:26 Magnus Hagander wrote:

On Fri, Nov 15, 2013 at 12:10 PM, Haribabu kommi
haribabu.ko...@huawei.com wrote:

On 14 November 2013 23:59 Fujii Masao wrote:
 On Thu, Nov 14, 2013 at 9:08 PM, Haribabu kommi
 haribabu.ko...@huawei.com wrote:
  Please find attached the patch, for adding a new option
  for pg_basebackup, to specify a different directory for
 pg_xlog.

 Sounds good! Here are the review comments:
 Don't we need to prevent users from specifying the same
   directory
 in both --pgdata and --xlogdir?

I feel no need to prevent, even if user specifies both
--pgdata
   and
--xlogdir as same directory all the transaction log files
will be created in the base directory  instead of pg_xlog
 directory.



Given how easy it would be to prevent that, I think we should.
  It
would be  an easy misunderstanding to specify that when you
   actually
want it in  wherever/pg_xlog. Specifying that would be
redundant in the first place,  but people ca do that, but it

would also be very easy to do it by mistake, and you'd end up
with something that's really bad, including a recursive
 symlink.



 Presently with initdb also user can specify both data and
 xlog
 directories as same.

 To prevent the data directory and xlog directory as same,
 there is
   a
 way in windows (_fullpath api) to get absolute path from a
 relative path, but I didn't get any such possibilities in
 linux.

 I didn't find any other way to check it, if anyone have any
 idea regarding this please let me know.
   
What about make_absolute_path() in miscinit.c?
  
   The make_absoulte_patch() function gets the current working
  directory
   and adds The relative path to CWD, this is not giving proper
  absolute
   path.
  
   I have added a new function verify_data_and_xlog_dir_same() which
   will change the Current working directory to data directory and
   gets the CWD and the same way for transaction log directory.
   Compare the both data and xlog directories and throw an error.
   Please check it
  once.
  
   Is there any other way to identify that both data and xlog
   directories are pointing to the same Instead of comparing both
  absolute paths?
  
   Updated patch attached in the mail.
  
   Failure when the xlogdir doesn't exist is fixed in the updated
   patch
  attached in the mail.
 
  Thanks for updating the patch!
 
  With the patch, when I specify the same directory in both -D and --
  xlogdir, I got the following error.
 
  $ cd /tmp
  $ pg_basebackup -D hoge --xlogdir=/tmp/hoge -X fetch
  pg_basebackup: could not change directory to hoge: No such file or
  directory
 
  Thanks. After finding the xlog directory absolute path returning back
  to current working Directory is missed, because of this reason it is
  failing while finding the absolute Path for data directory. Apart
 from this change the code is reorganized a bit.
 
  Updated patch is attached in the mail. Please review it once.

 Thanks for newer version of the patch!

 I found that the empty base directory is created and remains even when
 the same directory is specified in both -D and --xlogdir and the error
 occurs.
 I think that it's
 better to throw an error in that case before creating any new directory.
 Thought?

 By creating the base directory only the patch finds whether provided base and
 Xlog directories are same or not? To solve this problem following options are 
 possible

 1. No problem as it is just an empty base directory, so it can be reused in 
 the next time
Leave it as it is.
 2. Once the error is identified, the base directory can be deleted.
 3. write a new function to remove the parent references from the provided 
 path to identify
the absolute path used for detecting base and Xlog directories are same or 
 not?

 Please provide your suggestions.

 +xlogdir = get_absolute_path(xlog_dir);

 xlog_dir must be an absolute path. ISTM we don't do the above. No?

 It is required. As user can provide the path as 
 /home/installation/bin/../bin/data.
 The provided path is considered as absolute path only but while comparing the 
 same
 With data directory path it will not match.

Okay, maybe I understand you. In order to know the real absolute path, basically
we need to create the directory specified in --xlogdir, change 

Re: [HACKERS] New option for pg_basebackup, to specify a different directory for pg_xlog

2013-11-19 Thread Tom Lane
Fujii Masao masao.fu...@gmail.com writes:
 On the second thought, I'm thinking that it might be overkill to add
 such not simple code for that small benefit.

Yeah --- there's a limit to how much code we should expend on detecting
this type of error.  It's not like the case is all that plausible.

One idea that I don't think got discussed is stat'ing the two directories
and verifying that their dev/inode numbers are different.  I don't know
how portable that is though.

regards, tom lane


-- 
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] New option for pg_basebackup, to specify a different directory for pg_xlog

2013-11-19 Thread Magnus Hagander
On Tue, Nov 19, 2013 at 2:41 PM, Fujii Masao masao.fu...@gmail.com wrote:

 On Tue, Nov 19, 2013 at 9:14 PM, Haribabu kommi
 haribabu.ko...@huawei.com wrote:
  On 18 November 2013 23:30 Fujii Masao wrote:
  On Tue, Nov 19, 2013 at 12:01 AM, Haribabu kommi
  haribabu.ko...@huawei.com wrote:
   On 18 November 2013 18:45 Fujii Masao wrote:
   On Mon, Nov 18, 2013 at 6:31 PM, Haribabu kommi
   haribabu.ko...@huawei.com wrote:
   
On 18 November 2013 11:19 Haribabu kommi wrote:
On 17 November 2013 00:55 Fujii Masao wrote:
 On Sat, Nov 16, 2013 at 2:27 PM, Haribabu kommi
 haribabu.ko...@huawei.com wrote:
  on 15 November 2013 17:26 Magnus Hagander wrote:
 
 On Fri, Nov 15, 2013 at 12:10 PM, Haribabu kommi
 haribabu.ko...@huawei.com wrote:
 
 On 14 November 2013 23:59 Fujii Masao wrote:
  On Thu, Nov 14, 2013 at 9:08 PM, Haribabu kommi
  haribabu.ko...@huawei.com wrote:
   Please find attached the patch, for adding a new option
   for pg_basebackup, to specify a different directory for
  pg_xlog.
 
  Sounds good! Here are the review comments:
  Don't we need to prevent users from specifying the same
directory
  in both --pgdata and --xlogdir?
 
 I feel no need to prevent, even if user specifies both
 --pgdata
and
 --xlogdir as same directory all the transaction log files
 will be created in the base directory  instead of pg_xlog
  directory.
 
 
 
 Given how easy it would be to prevent that, I think we should.
   It
 would be  an easy misunderstanding to specify that when you
actually
 want it in  wherever/pg_xlog. Specifying that would be
 redundant in the first place,  but people ca do that, but it
 
 would also be very easy to do it by mistake, and you'd end up
 with something that's really bad, including a recursive
  symlink.
 
 
 
  Presently with initdb also user can specify both data and
  xlog
  directories as same.
 
  To prevent the data directory and xlog directory as same,
  there is
a
  way in windows (_fullpath api) to get absolute path from a
  relative path, but I didn't get any such possibilities in
  linux.
 
  I didn't find any other way to check it, if anyone have any
  idea regarding this please let me know.

 What about make_absolute_path() in miscinit.c?
   
The make_absoulte_patch() function gets the current working
   directory
and adds The relative path to CWD, this is not giving proper
   absolute
path.
   
I have added a new function verify_data_and_xlog_dir_same() which
will change the Current working directory to data directory and
gets the CWD and the same way for transaction log directory.
Compare the both data and xlog directories and throw an error.
Please check it
   once.
   
Is there any other way to identify that both data and xlog
directories are pointing to the same Instead of comparing both
   absolute paths?
   
Updated patch attached in the mail.
   
Failure when the xlogdir doesn't exist is fixed in the updated
patch
   attached in the mail.
  
   Thanks for updating the patch!
  
   With the patch, when I specify the same directory in both -D and --
   xlogdir, I got the following error.
  
   $ cd /tmp
   $ pg_basebackup -D hoge --xlogdir=/tmp/hoge -X fetch
   pg_basebackup: could not change directory to hoge: No such file or
   directory
  
   Thanks. After finding the xlog directory absolute path returning back
   to current working Directory is missed, because of this reason it is
   failing while finding the absolute Path for data directory. Apart
  from this change the code is reorganized a bit.
  
   Updated patch is attached in the mail. Please review it once.
 
  Thanks for newer version of the patch!
 
  I found that the empty base directory is created and remains even when
  the same directory is specified in both -D and --xlogdir and the error
  occurs.
  I think that it's
  better to throw an error in that case before creating any new directory.
  Thought?
 
  By creating the base directory only the patch finds whether provided
 base and
  Xlog directories are same or not? To solve this problem following
 options are possible
 
  1. No problem as it is just an empty base directory, so it can be reused
 in the next time
 Leave it as it is.
  2. Once the error is identified, the base directory can be deleted.
  3. write a new function to remove the parent references from the
 provided path to identify
 the absolute path used for detecting base and Xlog directories are
 same or not?
 
  Please provide your suggestions.
 
  +xlogdir = get_absolute_path(xlog_dir);
 
  xlog_dir must be an absolute path. ISTM we don't do the above. No?
 
  It is required. As user can provide the path as
 /home/installation/bin/../bin/data.
  The provided path is considered as absolute path only but while
 

Re: [HACKERS] New option for pg_basebackup, to specify a different directory for pg_xlog

2013-11-18 Thread Haribabu kommi

On 18 November 2013 11:19 Haribabu kommi wrote:
 On 17 November 2013 00:55 Fujii Masao wrote:
  On Sat, Nov 16, 2013 at 2:27 PM, Haribabu kommi
  haribabu.ko...@huawei.com wrote:
   on 15 November 2013 17:26 Magnus Hagander wrote:
  
  On Fri, Nov 15, 2013 at 12:10 PM, Haribabu kommi
  haribabu.ko...@huawei.com wrote:
  
  On 14 November 2013 23:59 Fujii Masao wrote:
   On Thu, Nov 14, 2013 at 9:08 PM, Haribabu kommi
   haribabu.ko...@huawei.com wrote:
Please find attached the patch, for adding a new option for
pg_basebackup, to specify a different directory for pg_xlog.
  
   Sounds good! Here are the review comments:
   Don't we need to prevent users from specifying the same
 directory
   in both --pgdata and --xlogdir?
  
  I feel no need to prevent, even if user specifies both --pgdata
 and
  --xlogdir as same directory all the transaction log files will be
  created in the base directory  instead of pg_xlog directory.
  
  
  
  Given how easy it would be to prevent that, I think we should. It
  would be  an easy misunderstanding to specify that when you
 actually
  want it in  wherever/pg_xlog. Specifying that would be redundant
  in the first place,  but people ca do that, but it
  
  would also be very easy to do it by mistake, and you'd end up with
  something that's really bad, including a recursive symlink.
  
  
  
   Presently with initdb also user can specify both data and xlog
   directories as same.
  
   To prevent the data directory and xlog directory as same, there is
 a
   way in windows (_fullpath api) to get absolute path from a relative
   path, but I didn't get any such possibilities in linux.
  
   I didn't find any other way to check it, if anyone have any idea
   regarding this please let me know.
 
  What about make_absolute_path() in miscinit.c?
 
 The make_absoulte_patch() function gets the current working directory
 and adds The relative path to CWD, this is not giving proper absolute
 path.
 
 I have added a new function verify_data_and_xlog_dir_same() which will
 change the Current working directory to data directory and gets the CWD
 and the same way for transaction log directory. Compare the both data
 and xlog directories and throw an error. Please check it once.
 
 Is there any other way to identify that both data and xlog directories
 are pointing to the same Instead of comparing both absolute paths?
 
 Updated patch attached in the mail.

Failure when the xlogdir doesn't exist is fixed in the updated patch attached 
in the mail.

Regards,
Hari babu.



UserSpecifiedxlogDir_v4.patch
Description: UserSpecifiedxlogDir_v4.patch

-- 
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] New option for pg_basebackup, to specify a different directory for pg_xlog

2013-11-18 Thread Fujii Masao
On Mon, Nov 18, 2013 at 6:31 PM, Haribabu kommi
haribabu.ko...@huawei.com wrote:

 On 18 November 2013 11:19 Haribabu kommi wrote:
 On 17 November 2013 00:55 Fujii Masao wrote:
  On Sat, Nov 16, 2013 at 2:27 PM, Haribabu kommi
  haribabu.ko...@huawei.com wrote:
   on 15 November 2013 17:26 Magnus Hagander wrote:
  
  On Fri, Nov 15, 2013 at 12:10 PM, Haribabu kommi
  haribabu.ko...@huawei.com wrote:
  
  On 14 November 2013 23:59 Fujii Masao wrote:
   On Thu, Nov 14, 2013 at 9:08 PM, Haribabu kommi
   haribabu.ko...@huawei.com wrote:
Please find attached the patch, for adding a new option for
pg_basebackup, to specify a different directory for pg_xlog.
  
   Sounds good! Here are the review comments:
   Don't we need to prevent users from specifying the same
 directory
   in both --pgdata and --xlogdir?
  
  I feel no need to prevent, even if user specifies both --pgdata
 and
  --xlogdir as same directory all the transaction log files will be
  created in the base directory  instead of pg_xlog directory.
  
  
  
  Given how easy it would be to prevent that, I think we should. It
  would be  an easy misunderstanding to specify that when you
 actually
  want it in  wherever/pg_xlog. Specifying that would be redundant
  in the first place,  but people ca do that, but it
  
  would also be very easy to do it by mistake, and you'd end up with
  something that's really bad, including a recursive symlink.
  
  
  
   Presently with initdb also user can specify both data and xlog
   directories as same.
  
   To prevent the data directory and xlog directory as same, there is
 a
   way in windows (_fullpath api) to get absolute path from a relative
   path, but I didn't get any such possibilities in linux.
  
   I didn't find any other way to check it, if anyone have any idea
   regarding this please let me know.
 
  What about make_absolute_path() in miscinit.c?

 The make_absoulte_patch() function gets the current working directory
 and adds The relative path to CWD, this is not giving proper absolute
 path.

 I have added a new function verify_data_and_xlog_dir_same() which will
 change the Current working directory to data directory and gets the CWD
 and the same way for transaction log directory. Compare the both data
 and xlog directories and throw an error. Please check it once.

 Is there any other way to identify that both data and xlog directories
 are pointing to the same Instead of comparing both absolute paths?

 Updated patch attached in the mail.

 Failure when the xlogdir doesn't exist is fixed in the updated patch attached 
 in the mail.

Thanks for updating the patch!

With the patch, when I specify the same directory in both -D and --xlogdir,
I got the following error.

$ cd /tmp
$ pg_basebackup -D hoge --xlogdir=/tmp/hoge -X fetch
pg_basebackup: could not change directory to hoge: No such file or directory

Regards,

-- 
Fujii Masao


-- 
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] New option for pg_basebackup, to specify a different directory for pg_xlog

2013-11-18 Thread Haribabu kommi
On 18 November 2013 18:45 Fujii Masao wrote:
 On Mon, Nov 18, 2013 at 6:31 PM, Haribabu kommi
 haribabu.ko...@huawei.com wrote:
 
  On 18 November 2013 11:19 Haribabu kommi wrote:
  On 17 November 2013 00:55 Fujii Masao wrote:
   On Sat, Nov 16, 2013 at 2:27 PM, Haribabu kommi
   haribabu.ko...@huawei.com wrote:
on 15 November 2013 17:26 Magnus Hagander wrote:
   
   On Fri, Nov 15, 2013 at 12:10 PM, Haribabu kommi
   haribabu.ko...@huawei.com wrote:
   
   On 14 November 2013 23:59 Fujii Masao wrote:
On Thu, Nov 14, 2013 at 9:08 PM, Haribabu kommi
haribabu.ko...@huawei.com wrote:
 Please find attached the patch, for adding a new option for
 pg_basebackup, to specify a different directory for pg_xlog.
   
Sounds good! Here are the review comments:
Don't we need to prevent users from specifying the same
  directory
in both --pgdata and --xlogdir?
   
   I feel no need to prevent, even if user specifies both --pgdata
  and
   --xlogdir as same directory all the transaction log files will
   be created in the base directory  instead of pg_xlog directory.
   
   
   
   Given how easy it would be to prevent that, I think we should.
 It
   would be  an easy misunderstanding to specify that when you
  actually
   want it in  wherever/pg_xlog. Specifying that would be
   redundant in the first place,  but people ca do that, but it
   
   would also be very easy to do it by mistake, and you'd end up
   with something that's really bad, including a recursive symlink.
   
   
   
Presently with initdb also user can specify both data and xlog
directories as same.
   
To prevent the data directory and xlog directory as same, there
is
  a
way in windows (_fullpath api) to get absolute path from a
relative path, but I didn't get any such possibilities in linux.
   
I didn't find any other way to check it, if anyone have any idea
regarding this please let me know.
  
   What about make_absolute_path() in miscinit.c?
 
  The make_absoulte_patch() function gets the current working
 directory
  and adds The relative path to CWD, this is not giving proper
 absolute
  path.
 
  I have added a new function verify_data_and_xlog_dir_same() which
  will change the Current working directory to data directory and gets
  the CWD and the same way for transaction log directory. Compare the
  both data and xlog directories and throw an error. Please check it
 once.
 
  Is there any other way to identify that both data and xlog
  directories are pointing to the same Instead of comparing both
 absolute paths?
 
  Updated patch attached in the mail.
 
  Failure when the xlogdir doesn't exist is fixed in the updated patch
 attached in the mail.
 
 Thanks for updating the patch!
 
 With the patch, when I specify the same directory in both -D and --
 xlogdir, I got the following error.
 
 $ cd /tmp
 $ pg_basebackup -D hoge --xlogdir=/tmp/hoge -X fetch
 pg_basebackup: could not change directory to hoge: No such file or
 directory

Thanks. After finding the xlog directory absolute path returning back to 
current working 
Directory is missed, because of this reason it is failing while finding the 
absolute
Path for data directory. Apart from this change the code is reorganized a bit.

Updated patch is attached in the mail. Please review it once.

Regards,
Hari babu.


UserSpecifiedxlogDir_v5.patch
Description: UserSpecifiedxlogDir_v5.patch

-- 
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] New option for pg_basebackup, to specify a different directory for pg_xlog

2013-11-18 Thread Andres Freund
On 2013-11-18 15:01:42 +, Haribabu kommi wrote:
  
  /*
 + * Returns the malloced string of containing current working directory.
 + * The caller has to take care of freeing the memory.
 + * On failure exits with error code.
 + */
 +static char *
 +get_current_working_dir()
 +{
 + char   *buf;
 + size_t  buflen;
 +
 + buflen = MAXPGPATH;
 + for (;;)
 + {
 + buf = pg_malloc(buflen);
 + if (getcwd(buf, buflen))
 + break;
 + else if (errno == ERANGE)
 + {
 + pg_free(buf);
 + buflen *= 2;
 + continue;
 + }
 + else
 + {
 + pg_free(buf);
 + fprintf(stderr,
 + _(%s: could not get current working 
 directory: %s\n),
 + progname, strerror(errno));
 + exit(1);
 + }
 + }
 +
 + return buf;
 +}
 +
 +/*
 + * calculates the absolute path for the given path. The output absolute path
 + * is a malloced string. The caller needs to take care of freeing the memory.
 + */
 +static char *
 +get_absolute_path(const char *input_path)
 +{
 + char   *pwd = NULL;
 + char   *abspath = NULL;
 +
 + /* Getting the present working directory */
 + pwd = get_current_working_dir();
 +
 + if (chdir(input_path)  0)
 + {
 + /* Directory doesn't exist */
 + if (errno == ENOENT)
 + return NULL;
 +
 + fprintf(stderr, _(%s: could not change directory to \%s\: 
 %s\n),
 + progname, input_path, strerror(errno));
 + exit(1);
 + }
 +
 + abspath = get_current_working_dir();
 +
 + /* Returning back to old working directory */
 + if (chdir(pwd)  0)
 + {
 + fprintf(stderr, _(%s: could not change directory to \%s\: 
 %s\n),
 + progname, pwd, strerror(errno));
 + exit(1);
 + }
 +
 + pg_free(pwd);
 + return abspath;
 +}

This looks like it should be replaced by moving make_absolute_path from
pg_regress.c to path.[ch] and then use that.

Greetings,

Andres Freund

-- 
 Andres Freund http://www.2ndQuadrant.com/
 PostgreSQL Development, 24x7 Support, Training  Services


-- 
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] New option for pg_basebackup, to specify a different directory for pg_xlog

2013-11-18 Thread Fujii Masao
On Tue, Nov 19, 2013 at 12:01 AM, Haribabu kommi
haribabu.ko...@huawei.com wrote:
 On 18 November 2013 18:45 Fujii Masao wrote:
 On Mon, Nov 18, 2013 at 6:31 PM, Haribabu kommi
 haribabu.ko...@huawei.com wrote:
 
  On 18 November 2013 11:19 Haribabu kommi wrote:
  On 17 November 2013 00:55 Fujii Masao wrote:
   On Sat, Nov 16, 2013 at 2:27 PM, Haribabu kommi
   haribabu.ko...@huawei.com wrote:
on 15 November 2013 17:26 Magnus Hagander wrote:
   
   On Fri, Nov 15, 2013 at 12:10 PM, Haribabu kommi
   haribabu.ko...@huawei.com wrote:
   
   On 14 November 2013 23:59 Fujii Masao wrote:
On Thu, Nov 14, 2013 at 9:08 PM, Haribabu kommi
haribabu.ko...@huawei.com wrote:
 Please find attached the patch, for adding a new option for
 pg_basebackup, to specify a different directory for pg_xlog.
   
Sounds good! Here are the review comments:
Don't we need to prevent users from specifying the same
  directory
in both --pgdata and --xlogdir?
   
   I feel no need to prevent, even if user specifies both --pgdata
  and
   --xlogdir as same directory all the transaction log files will
   be created in the base directory  instead of pg_xlog directory.
   
   
   
   Given how easy it would be to prevent that, I think we should.
 It
   would be  an easy misunderstanding to specify that when you
  actually
   want it in  wherever/pg_xlog. Specifying that would be
   redundant in the first place,  but people ca do that, but it
   
   would also be very easy to do it by mistake, and you'd end up
   with something that's really bad, including a recursive symlink.
   
   
   
Presently with initdb also user can specify both data and xlog
directories as same.
   
To prevent the data directory and xlog directory as same, there
is
  a
way in windows (_fullpath api) to get absolute path from a
relative path, but I didn't get any such possibilities in linux.
   
I didn't find any other way to check it, if anyone have any idea
regarding this please let me know.
  
   What about make_absolute_path() in miscinit.c?
 
  The make_absoulte_patch() function gets the current working
 directory
  and adds The relative path to CWD, this is not giving proper
 absolute
  path.
 
  I have added a new function verify_data_and_xlog_dir_same() which
  will change the Current working directory to data directory and gets
  the CWD and the same way for transaction log directory. Compare the
  both data and xlog directories and throw an error. Please check it
 once.
 
  Is there any other way to identify that both data and xlog
  directories are pointing to the same Instead of comparing both
 absolute paths?
 
  Updated patch attached in the mail.
 
  Failure when the xlogdir doesn't exist is fixed in the updated patch
 attached in the mail.

 Thanks for updating the patch!

 With the patch, when I specify the same directory in both -D and --
 xlogdir, I got the following error.

 $ cd /tmp
 $ pg_basebackup -D hoge --xlogdir=/tmp/hoge -X fetch
 pg_basebackup: could not change directory to hoge: No such file or
 directory

 Thanks. After finding the xlog directory absolute path returning back to 
 current working
 Directory is missed, because of this reason it is failing while finding the 
 absolute
 Path for data directory. Apart from this change the code is reorganized a bit.

 Updated patch is attached in the mail. Please review it once.

Thanks for newer version of the patch!

I found that the empty base directory is created and remains even when the same
directory is specified in both -D and --xlogdir and the error occurs.
I think that it's
better to throw an error in that case before creating any new
directory. Thought?

+xlogdir = get_absolute_path(xlog_dir);

xlog_dir must be an absolute path. ISTM we don't do the above. No?

Regards,

-- 
Fujii Masao


-- 
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] New option for pg_basebackup, to specify a different directory for pg_xlog

2013-11-18 Thread Haribabu kommi
On 18 November 2013 23:25 Andres Freund wrote:
 On 2013-11-18 15:01:42 +, Haribabu kommi wrote:
 
   /*
  + * Returns the malloced string of containing current working
 directory.
  + * The caller has to take care of freeing the memory.
  + * On failure exits with error code.
  + */
  +static char *
  +get_current_working_dir()
  +{
  +   char   *buf;
  +   size_t  buflen;
  +
  +   buflen = MAXPGPATH;
  +   for (;;)
  +   {
  +   buf = pg_malloc(buflen);
  +   if (getcwd(buf, buflen))
  +   break;
  +   else if (errno == ERANGE)
  +   {
  +   pg_free(buf);
  +   buflen *= 2;
  +   continue;
  +   }
  +   else
  +   {
  +   pg_free(buf);
  +   fprintf(stderr,
  +   _(%s: could not get current working
 directory: %s\n),
  +   progname, strerror(errno));
  +   exit(1);
  +   }
  +   }
  +
  +   return buf;
  +}
  +
  +/*
  + * calculates the absolute path for the given path. The output
  +absolute path
  + * is a malloced string. The caller needs to take care of freeing
 the memory.
  + */
  +static char *
  +get_absolute_path(const char *input_path) {
  +   char   *pwd = NULL;
  +   char   *abspath = NULL;
  +
  +   /* Getting the present working directory */
  +   pwd = get_current_working_dir();
  +
  +   if (chdir(input_path)  0)
  +   {
  +   /* Directory doesn't exist */
  +   if (errno == ENOENT)
  +   return NULL;
  +
  +   fprintf(stderr, _(%s: could not change directory to
 \%s\: %s\n),
  +   progname, input_path, strerror(errno));
  +   exit(1);
  +   }
  +
  +   abspath = get_current_working_dir();
  +
  +   /* Returning back to old working directory */
  +   if (chdir(pwd)  0)
  +   {
  +   fprintf(stderr, _(%s: could not change directory to
 \%s\: %s\n),
  +   progname, pwd, strerror(errno));
  +   exit(1);
  +   }
  +
  +   pg_free(pwd);
  +   return abspath;
  +}
 
 This looks like it should be replaced by moving make_absolute_path from
 pg_regress.c to path.[ch] and then use that.

The get_absolute_path function removes any parent references, if exists, in 
the path
But the make_absolute_path doesn't. In this patch proper path is required to 
compare
The xlog and data directories provided are same or not?

I find two ways to do this
1. change to that provided directory and get the current working directory.
2. Write a function to remove the parent references in the path.

This patch has implemented the first approach. Please let me know your 
suggestions.

Regards,
Hari babu.


-- 
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] New option for pg_basebackup, to specify a different directory for pg_xlog

2013-11-17 Thread Haribabu kommi
On 17 November 2013 00:55 Fujii Masao wrote:
 On Sat, Nov 16, 2013 at 2:27 PM, Haribabu kommi
 haribabu.ko...@huawei.com wrote:
  on 15 November 2013 17:26 Magnus Hagander wrote:
 
 On Fri, Nov 15, 2013 at 12:10 PM, Haribabu kommi
 haribabu.ko...@huawei.com wrote:
 
 On 14 November 2013 23:59 Fujii Masao wrote:
  On Thu, Nov 14, 2013 at 9:08 PM, Haribabu kommi
  haribabu.ko...@huawei.com wrote:
   Please find attached the patch, for adding a new option for
   pg_basebackup, to specify a different directory for pg_xlog.
 
  Sounds good! Here are the review comments:
  Don't we need to prevent users from specifying the same directory
  in both --pgdata and --xlogdir?
 
 I feel no need to prevent, even if user specifies both --pgdata and
 --xlogdir as same directory all the transaction log files will be
 created in the base directory  instead of pg_xlog directory.
 
 
 
 Given how easy it would be to prevent that, I think we should. It
 would be  an easy misunderstanding to specify that when you actually
 want it in  wherever/pg_xlog. Specifying that would be redundant in
 the first place,  but people ca do that, but it
 
 would also be very easy to do it by mistake, and you'd end up with
 something that's really bad, including a recursive symlink.
 
 
 
  Presently with initdb also user can specify both data and xlog
  directories as same.
 
  To prevent the data directory and xlog directory as same, there is a
  way in windows (_fullpath api) to get absolute path from a relative
  path, but I didn't get any such possibilities in linux.
 
  I didn't find any other way to check it, if anyone have any idea
  regarding this please let me know.
 
 What about make_absolute_path() in miscinit.c?

The make_absoulte_patch() function gets the current working directory and adds
The relative path to CWD, this is not giving proper absolute path. 

I have added a new function verify_data_and_xlog_dir_same() which will change 
the 
Current working directory to data directory and gets the CWD and the same way 
for transaction
log directory. Compare the both data and xlog directories and throw an error. 
Please check it once.

Is there any other way to identify that both data and xlog directories are 
pointing to the same
Instead of comparing both absolute paths?

Updated patch attached in the mail.

Regards,
Hari babu.


UserSpecifiedxlogDir_v3.patch
Description: UserSpecifiedxlogDir_v3.patch

-- 
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] New option for pg_basebackup, to specify a different directory for pg_xlog

2013-11-16 Thread Fujii Masao
On Sat, Nov 16, 2013 at 2:27 PM, Haribabu kommi
haribabu.ko...@huawei.com wrote:
 on 15 November 2013 17:26 Magnus Hagander wrote:

On Fri, Nov 15, 2013 at 12:10 PM, Haribabu kommi
 haribabu.ko...@huawei.com wrote:

On 14 November 2013 23:59 Fujii Masao wrote:
 On Thu, Nov 14, 2013 at 9:08 PM, Haribabu kommi
 haribabu.ko...@huawei.com wrote:
  Please find attached the patch, for adding a new option for
  pg_basebackup, to specify a different directory for pg_xlog.

 Sounds good! Here are the review comments:
 Don't we need to prevent users from specifying the same directory in
 both --pgdata and --xlogdir?

I feel no need to prevent, even if user specifies both --pgdata and
 --xlogdir as same directory
all the transaction log files will be created in the base directory
 instead of pg_xlog directory.



Given how easy it would be to prevent that, I think we should. It would be
 an easy misunderstanding to specify that when you actually want it in
 wherever/pg_xlog. Specifying that would be redundant in the first place,
 but people ca do that, but it

would also be very easy to do it by mistake, and you'd end up with
 something that's really bad, including a recursive symlink.



 Presently with initdb also user can specify both data and xlog directories
 as same.

 To prevent the data directory and xlog directory as same, there is a way in
 windows (_fullpath api) to get absolute path from a relative path, but I
 didn’t get any such possibilities in linux.

 I didn’t find any other way to check it, if anyone have any idea regarding
 this please let me know.

What about make_absolute_path() in miscinit.c?

Regards,

-- 
Fujii Masao


-- 
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] New option for pg_basebackup, to specify a different directory for pg_xlog

2013-11-15 Thread Haribabu kommi
On 14 November 2013 23:59 Fujii Masao wrote:
 On Thu, Nov 14, 2013 at 9:08 PM, Haribabu kommi
 haribabu.ko...@huawei.com wrote:
  Please find attached the patch, for adding a new option for
  pg_basebackup, to specify a different directory for pg_xlog.
 
 Sounds good! Here are the review comments:
 
 +printf(_(--xlogdir=XLOGDIR   location for the
 transaction log directory\n));
 
 This message is not aligned well.

Fixed.
 
 -if (!streamwal || strcmp(filename +
 strlen(filename) - 8, /pg_xlog) != 0)
 +if ((!streamwal  (strcmp(xlog_dir, ) == 0))
 +|| strcmp(filename + strlen(filename) -
 8, /pg_xlog) != 0)
 
 You need to update the source code comment.

Corrected the source code comment. Please check once.
 
 +#ifdef HAVE_SYMLINK
 +if (symlink(xlog_dir, linkloc) != 0)
 +{
 +fprintf(stderr, _(%s: could not create symbolic link
 \%s\: %s\n),
 +progname, linkloc, strerror(errno));
 +exit(1);
 +}
 +#else
 +fprintf(stderr, _(%s: symlinks are not supported on this
 platform 
 +  cannot use xlogdir));
 +exit(1);
 +#endif
 +}
 
 Is it better to call pg_free() at the end? Even if we don't do that, it
 would be almost harmless, though.

Added pg_free to free up the linkloc.

 Don't we need to prevent users from specifying the same directory in
 both --pgdata and --xlogdir?

I feel no need to prevent, even if user specifies both --pgdata and --xlogdir 
as same directory
all the transaction log files will be created in the base directory instead of 
pg_xlog directory. 

Regards,
Hari babu.


UserSpecifiedxlogDir_v2.patch
Description: UserSpecifiedxlogDir_v2.patch

-- 
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] New option for pg_basebackup, to specify a different directory for pg_xlog

2013-11-15 Thread Magnus Hagander
On Fri, Nov 15, 2013 at 12:10 PM, Haribabu kommi
haribabu.ko...@huawei.comwrote:

 On 14 November 2013 23:59 Fujii Masao wrote:
  On Thu, Nov 14, 2013 at 9:08 PM, Haribabu kommi
  haribabu.ko...@huawei.com wrote:
   Please find attached the patch, for adding a new option for
   pg_basebackup, to specify a different directory for pg_xlog.
 
  Sounds good! Here are the review comments:
 
  +printf(_(--xlogdir=XLOGDIR   location for the
  transaction log directory\n));
 
  This message is not aligned well.

 Fixed.

  -if (!streamwal || strcmp(filename +
  strlen(filename) - 8, /pg_xlog) != 0)
  +if ((!streamwal  (strcmp(xlog_dir, ) == 0))
  +|| strcmp(filename + strlen(filename) -
  8, /pg_xlog) != 0)
 
  You need to update the source code comment.

 Corrected the source code comment. Please check once.

  +#ifdef HAVE_SYMLINK
  +if (symlink(xlog_dir, linkloc) != 0)
  +{
  +fprintf(stderr, _(%s: could not create symbolic link
  \%s\: %s\n),
  +progname, linkloc, strerror(errno));
  +exit(1);
  +}
  +#else
  +fprintf(stderr, _(%s: symlinks are not supported on this
  platform 
  +  cannot use xlogdir));
  +exit(1);
  +#endif
  +}
 
  Is it better to call pg_free() at the end? Even if we don't do that, it
  would be almost harmless, though.

 Added pg_free to free up the linkloc.

  Don't we need to prevent users from specifying the same directory in
  both --pgdata and --xlogdir?

 I feel no need to prevent, even if user specifies both --pgdata and
 --xlogdir as same directory
 all the transaction log files will be created in the base directory
 instead of pg_xlog directory.


Given how easy it would be to prevent that, I think we should. It would be
an easy misunderstanding to specify that when you actually want it in
wherever/pg_xlog. Specifying that would be redundant in the first place,
but people ca do that, but it would also be very easy to do it by mistake,
and you'd end up with something that's really bad, including a recursive
symlink.

I also think it would probably be worthwhile to support this in  tar format
as well, but I guess that's a separate patch really. There's really no
reason we should't support wal streaming into a separate tar file. But -
separate issue.


-- 
 Magnus Hagander
 Me: http://www.hagander.net/
 Work: http://www.redpill-linpro.com/


Re: [HACKERS] New option for pg_basebackup, to specify a different directory for pg_xlog

2013-11-15 Thread Haribabu kommi
On 15 November 2013 17:26 Magnus Hagander wrote:
On Fri, Nov 15, 2013 at 12:10 PM, Haribabu kommi 
haribabu.ko...@huawei.commailto:haribabu.ko...@huawei.com wrote:
On 14 November 2013 23:59 Fujii Masao wrote:
 On Thu, Nov 14, 2013 at 9:08 PM, Haribabu kommi
 haribabu.ko...@huawei.commailto:haribabu.ko...@huawei.com wrote:
  Please find attached the patch, for adding a new option for
  pg_basebackup, to specify a different directory for pg_xlog.

 Sounds good! Here are the review comments:

 +printf(_(--xlogdir=XLOGDIR   location for the
 transaction log directory\n));

 This message is not aligned well.
Fixed.

 -if (!streamwal || strcmp(filename +
 strlen(filename) - 8, /pg_xlog) != 0)
 +if ((!streamwal  (strcmp(xlog_dir, ) == 0))
 +|| strcmp(filename + strlen(filename) -
 8, /pg_xlog) != 0)

 You need to update the source code comment.
Corrected the source code comment. Please check once.

 +#ifdef HAVE_SYMLINK
 +if (symlink(xlog_dir, linkloc) != 0)
 +{
 +fprintf(stderr, _(%s: could not create symbolic link
 \%s\: %s\n),
 +progname, linkloc, strerror(errno));
 +exit(1);
 +}
 +#else
 +fprintf(stderr, _(%s: symlinks are not supported on this
 platform 
 +  cannot use xlogdir));
 +exit(1);
 +#endif
 +}

 Is it better to call pg_free() at the end? Even if we don't do that, it
 would be almost harmless, though.
Added pg_free to free up the linkloc.

 Don't we need to prevent users from specifying the same directory in
 both --pgdata and --xlogdir?
I feel no need to prevent, even if user specifies both --pgdata and --xlogdir 
as same directory
all the transaction log files will be created in the base directory instead of 
pg_xlog directory.


Re: [HACKERS] New option for pg_basebackup, to specify a different directory for pg_xlog

2013-11-15 Thread Haribabu kommi
on 15 November 2013 17:26 Magnus Hagander wrote:
On Fri, Nov 15, 2013 at 12:10 PM, Haribabu kommi 
haribabu.ko...@huawei.commailto:haribabu.ko...@huawei.com wrote:
On 14 November 2013 23:59 Fujii Masao wrote:
 On Thu, Nov 14, 2013 at 9:08 PM, Haribabu kommi
 haribabu.ko...@huawei.commailto:haribabu.ko...@huawei.com wrote:
  Please find attached the patch, for adding a new option for
  pg_basebackup, to specify a different directory for pg_xlog.

 Sounds good! Here are the review comments:
 Don't we need to prevent users from specifying the same directory in
 both --pgdata and --xlogdir?
I feel no need to prevent, even if user specifies both --pgdata and --xlogdir 
as same directory
all the transaction log files will be created in the base directory instead 
of pg_xlog directory.

Given how easy it would be to prevent that, I think we should. It would be an 
easy misunderstanding to specify that when you actually want it in 
wherever/pg_xlog. Specifying that would be redundant in the first place, but 
people ca do that, but it
would also be very easy to do it by mistake, and you'd end up with something 
that's really bad, including a recursive symlink.

Presently with initdb also user can specify both data and xlog directories as 
same.
To prevent the data directory and xlog directory as same, there is a way in 
windows (_fullpath api) to get absolute path from a relative path, but I didn't 
get any such possibilities in linux.
I didn't find any other way to check it, if anyone have any idea regarding this 
please let me know.

I also think it would probably be worthwhile to support this in  tar format as 
well, but I guess that's a separate patch really. There's really no reason we 
should't support wal streaming into a separate tar file. But - separate issue.

Sure. I will prepare a separate patch for the same and submit it in the next 
commit fest.

Regards,
Hari babu.


Re: [HACKERS] New option for pg_basebackup, to specify a different directory for pg_xlog

2013-11-14 Thread Fujii Masao
On Thu, Nov 14, 2013 at 9:08 PM, Haribabu kommi
haribabu.ko...@huawei.com wrote:
 Please find attached the patch, for adding a new option for pg_basebackup,
 to specify a different directory for pg_xlog.

Sounds good! Here are the review comments:

+printf(_(--xlogdir=XLOGDIR   location for the
transaction log directory\n));

This message is not aligned well.

-if (!streamwal || strcmp(filename +
strlen(filename) - 8, /pg_xlog) != 0)
+if ((!streamwal  (strcmp(xlog_dir, ) == 0))
+|| strcmp(filename + strlen(filename) -
8, /pg_xlog) != 0)

You need to update the source code comment.

+#ifdef HAVE_SYMLINK
+if (symlink(xlog_dir, linkloc) != 0)
+{
+fprintf(stderr, _(%s: could not create symbolic link
\%s\: %s\n),
+progname, linkloc, strerror(errno));
+exit(1);
+}
+#else
+fprintf(stderr, _(%s: symlinks are not supported on this platform 
+  cannot use xlogdir));
+exit(1);
+#endif
+}

Is it better to call pg_free() at the end? Even if we don't do that, it would be
almost harmless, though.

Don't we need to prevent users from specifying the same directory in both
--pgdata and --xlogdir?

Regards,

-- 
Fujii Masao


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers