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>
+       <term><option>--xlogdir=<replaceable class="parameter">xlogdir</replaceable></option></term>
+       <listitem>
+        <para>
+         Specifies the location for the transaction log directory. 
+         <replaceable>xlogdir</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>
        <term><option>-x</option></term>
        <term><option>--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 ----
  					{
  						/*
  						 * When streaming WAL, pg_xlog will have been created
! 						 * by the wal receiver process. Also, when transaction
! 						 * log directory location was specified, pg_xlog has
! 						 * already been created as a symbolic link before
! 						 * starting the actual backup. So just ignore failure
! 						 * on them.
  						 */
! 						if ((!streamwal && (strcmp(xlog_dir, "") == 0))
! 							|| strcmp(filename + strlen(filename) - 8, "/pg_xlog") != 0)
  						{
  							fprintf(stderr,
  							_("%s: could not create directory \"%s\": %s\n"),
***************
*** 1666,1671 **** main(int argc, char **argv)
--- 1672,1678 ----
  		{"status-interval", required_argument, NULL, 's'},
  		{"verbose", no_argument, NULL, 'v'},
  		{"progress", no_argument, NULL, 'P'},
+ 		{"xlogdir", required_argument, NULL, 1},
  		{NULL, 0, NULL, 0}
  	};
  	int			c;
***************
*** 1750,1755 **** main(int argc, char **argv)
--- 1757,1765 ----
  					exit(1);
  				}
  				break;
+ 			case 1:
+ 				xlog_dir = pg_strdup(optarg);
+ 				break;
  			case 'l':
  				label = pg_strdup(optarg);
  				break;
***************
*** 1872,1877 **** main(int argc, char **argv)
--- 1882,1911 ----
  		exit(1);
  	}
  
+ 	if (strcmp(xlog_dir, "") != 0)
+ 	{
+ 		if (format != 'p')
+ 		{
+ 			fprintf(stderr,
+ 					_("%s: transaction log directory location can only be specified in plain mode\n"),
+ 					progname);
+ 			fprintf(stderr, _("Try \"%s --help\" for more information.\n"),
+ 					progname);
+ 			exit(1);
+ 		}
+ 
+ 		/* clean up xlog directory name, check it's absolute */
+ 		canonicalize_path(xlog_dir);
+ 		if (!is_absolute_path(xlog_dir))
+ 		{
+ 			fprintf(stderr, _("%s: transaction log directory location must be "
+ 							  "an absolute path\n"), progname);
+ 			fprintf(stderr, _("Try \"%s --help\" for more information.\n"),
+ 					progname);
+ 			exit(1);
+ 		}
+ 	}
+ 
  #ifndef HAVE_LIBZ
  	if (compresslevel != 0)
  	{
***************
*** 1890,1895 **** main(int argc, char **argv)
--- 1924,1953 ----
  	if (format == 'p' || strcmp(basedir, "-") != 0)
  		verify_dir_is_empty_or_create(basedir);
  
+ 	/* Create transaction log symlink, if required */
+ 	if (strcmp(xlog_dir, "") != 0)
+ 	{
+ 		char	   *linkloc;
+ 
+ 		verify_dir_is_empty_or_create(xlog_dir);
+ 
+ 		/* form name of the place where the symlink must go */
+ 		linkloc = psprintf("%s/pg_xlog", basedir);
+ 
+ #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"));
+ 		exit(1);
+ #endif
+ 		free(linkloc);
+ 	}
+ 
  	BaseBackup();
  
  	return 0;
-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Reply via email to