pg_basebackup --xlog compatibility break

Started by Peter Eisentrautover 13 years ago14 messages
#1Peter Eisentraut
peter_e@gmx.net

In 9.1, the pg_basebackup option --xlog takes no argument. In 9.2, it
takes a required argument. I think such compatibility breaks should be
avoided, especially in client-side programs. Now you can't write a
script running pg_basebackup that works with 9.1 and 9.2, if you need to
include the WAL.

I think the behavior of -x/--xlog should be restored to the state of
9.1, and a new option should be added to select between the fetch and
stream methods. (With a suitable default, this would also increase
usability a bit.)

#2Magnus Hagander
magnus@hagander.net
In reply to: Peter Eisentraut (#1)
Re: pg_basebackup --xlog compatibility break

On Mon, May 28, 2012 at 10:11 PM, Peter Eisentraut <peter_e@gmx.net> wrote:

In 9.1, the pg_basebackup option --xlog takes no argument.  In 9.2, it
takes a required argument.  I think such compatibility breaks should be
avoided, especially in client-side programs.  Now you can't write a
script running pg_basebackup that works with 9.1 and 9.2, if you need to
include the WAL.

I think the behavior of -x/--xlog should be restored to the state of
9.1, and a new option should be added to select between the fetch and
stream methods.  (With a suitable default, this would also increase
usability a bit.)

Just to be clear - it's not possible to actually accept -x with an
*optional* parameter, is it? Meaning "-x" would mean the same as "-x
fetch" and therefor become backwards compatible?

IIRC I did try that, and didn't get it to work - but if that's doable,
that seems like the cleanest way?

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

#3Fujii Masao
masao.fujii@gmail.com
In reply to: Magnus Hagander (#2)
Re: pg_basebackup --xlog compatibility break

On Tue, May 29, 2012 at 5:38 AM, Magnus Hagander <magnus@hagander.net> wrote:

On Mon, May 28, 2012 at 10:11 PM, Peter Eisentraut <peter_e@gmx.net> wrote:

In 9.1, the pg_basebackup option --xlog takes no argument.  In 9.2, it
takes a required argument.  I think such compatibility breaks should be
avoided, especially in client-side programs.  Now you can't write a
script running pg_basebackup that works with 9.1 and 9.2, if you need to
include the WAL.

I think the behavior of -x/--xlog should be restored to the state of
9.1, and a new option should be added to select between the fetch and
stream methods.  (With a suitable default, this would also increase
usability a bit.)

Just to be clear - it's not possible to actually accept -x with an
*optional* parameter, is it? Meaning "-x" would mean the same as "-x
fetch" and therefor become backwards compatible?

IIRC I did try that, and didn't get it to work - but if that's doable,
that seems like the cleanest way?

+1 ISTM you need to change getopt_long() to do that.

Regards,

--
Fujii Masao

#4Robert Haas
robertmhaas@gmail.com
In reply to: Magnus Hagander (#2)
Re: pg_basebackup --xlog compatibility break

On Mon, May 28, 2012 at 4:38 PM, Magnus Hagander <magnus@hagander.net> wrote:

On Mon, May 28, 2012 at 10:11 PM, Peter Eisentraut <peter_e@gmx.net> wrote:

In 9.1, the pg_basebackup option --xlog takes no argument.  In 9.2, it
takes a required argument.  I think such compatibility breaks should be
avoided, especially in client-side programs.  Now you can't write a
script running pg_basebackup that works with 9.1 and 9.2, if you need to
include the WAL.

I think the behavior of -x/--xlog should be restored to the state of
9.1, and a new option should be added to select between the fetch and
stream methods.  (With a suitable default, this would also increase
usability a bit.)

Just to be clear - it's not possible to actually accept -x with an
*optional* parameter, is it? Meaning "-x" would mean the same as "-x
fetch" and therefor become backwards compatible?

IIRC I did try that, and didn't get it to work - but if that's doable,
that seems like the cleanest way?

Aren't you still going to have situations where it's the behavior
changes, if you go this route?

Consider this command line:

$ foo -b bar

Is bar an argument to -b, or an argument to foo? If -b required or
forbade an argument it would be clear, but if the argument is optional
then it's fuzzy. Similarly, consider:

$ foo -bar

If -b takes no argument then this means the same thing as "foo -b -a
-r", but and if -b requires an argument then ar is the argument to
foo. If -b takes an optional argument, then it's ambiguous.

I don't remember the exact behavior of getopt_long(), but I bet if we
go this route we'll find that there are cases where the behavior
changes vs. older releases; they'll just be subtler. Peter's
suggestion of a separate switch seems better to me for that reason.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

#5Fujii Masao
masao.fujii@gmail.com
In reply to: Robert Haas (#4)
Re: pg_basebackup --xlog compatibility break

On Wed, May 30, 2012 at 2:03 AM, Robert Haas <robertmhaas@gmail.com> wrote:

On Mon, May 28, 2012 at 4:38 PM, Magnus Hagander <magnus@hagander.net> wrote:

On Mon, May 28, 2012 at 10:11 PM, Peter Eisentraut <peter_e@gmx.net> wrote:

In 9.1, the pg_basebackup option --xlog takes no argument.  In 9.2, it
takes a required argument.  I think such compatibility breaks should be
avoided, especially in client-side programs.  Now you can't write a
script running pg_basebackup that works with 9.1 and 9.2, if you need to
include the WAL.

I think the behavior of -x/--xlog should be restored to the state of
9.1, and a new option should be added to select between the fetch and
stream methods.  (With a suitable default, this would also increase
usability a bit.)

Just to be clear - it's not possible to actually accept -x with an
*optional* parameter, is it? Meaning "-x" would mean the same as "-x
fetch" and therefor become backwards compatible?

IIRC I did try that, and didn't get it to work - but if that's doable,
that seems like the cleanest way?

Aren't you still going to have situations where it's the behavior
changes, if you go this route?

Consider this command line:

$ foo -b bar

Is bar an argument to -b, or an argument to foo?  If -b required or
forbade an argument it would be clear, but if the argument is optional
then it's fuzzy.  Similarly, consider:

$ foo -bar

If -b takes no argument then this means the same thing as "foo -b -a
-r", but and if -b requires an argument then ar is the argument to
foo.  If -b takes an optional argument, then it's ambiguous.

I don't remember the exact behavior of getopt_long(), but I bet if we
go this route we'll find that there are cases where the behavior
changes vs. older releases; they'll just be subtler.  Peter's
suggestion of a separate switch seems better to me for that reason.

You're right. I thought that optional parameter is possible because
I recalled GNU extended getopt(3) supported that. After reading its man,
I found that an argument must be in the same word as the option name
to specify an argument, e.g., -xfetch (not -x fetch). This optional
parameter looks confusing to a user. So I agree to add another parameter.

Regards,

--
Fujii Masao

#6Magnus Hagander
magnus@hagander.net
In reply to: Fujii Masao (#5)
Re: pg_basebackup --xlog compatibility break

On Tue, May 29, 2012 at 8:51 PM, Fujii Masao <masao.fujii@gmail.com> wrote:

On Wed, May 30, 2012 at 2:03 AM, Robert Haas <robertmhaas@gmail.com> wrote:

On Mon, May 28, 2012 at 4:38 PM, Magnus Hagander <magnus@hagander.net> wrote:

On Mon, May 28, 2012 at 10:11 PM, Peter Eisentraut <peter_e@gmx.net> wrote:

In 9.1, the pg_basebackup option --xlog takes no argument.  In 9.2, it
takes a required argument.  I think such compatibility breaks should be
avoided, especially in client-side programs.  Now you can't write a
script running pg_basebackup that works with 9.1 and 9.2, if you need to
include the WAL.

I think the behavior of -x/--xlog should be restored to the state of
9.1, and a new option should be added to select between the fetch and
stream methods.  (With a suitable default, this would also increase
usability a bit.)

Just to be clear - it's not possible to actually accept -x with an
*optional* parameter, is it? Meaning "-x" would mean the same as "-x
fetch" and therefor become backwards compatible?

IIRC I did try that, and didn't get it to work - but if that's doable,
that seems like the cleanest way?

Aren't you still going to have situations where it's the behavior
changes, if you go this route?

Consider this command line:

$ foo -b bar

Is bar an argument to -b, or an argument to foo?  If -b required or
forbade an argument it would be clear, but if the argument is optional
then it's fuzzy.  Similarly, consider:

$ foo -bar

If -b takes no argument then this means the same thing as "foo -b -a
-r", but and if -b requires an argument then ar is the argument to
foo.  If -b takes an optional argument, then it's ambiguous.

I don't remember the exact behavior of getopt_long(), but I bet if we
go this route we'll find that there are cases where the behavior
changes vs. older releases; they'll just be subtler.  Peter's
suggestion of a separate switch seems better to me for that reason.

You're right. I thought that optional parameter is possible because
I recalled GNU extended getopt(3) supported that. After reading its man,
I found that an argument must be in the same word as the option name
to specify an argument, e.g., -xfetch (not -x fetch). This optional
parameter looks confusing to a user. So I agree to add another parameter.

Yeah, good arguments all around, i agree too :-) Next question is -
suggestions for naming of said paramter?

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

#7Peter Eisentraut
peter_e@gmx.net
In reply to: Magnus Hagander (#6)
Re: pg_basebackup --xlog compatibility break

On tis, 2012-05-29 at 22:31 +0200, Magnus Hagander wrote:

Yeah, good arguments all around, i agree too :-) Next question is -
suggestions for naming of said paramter?

--xlog-method=something? And/or -Xsomething, which would automatically
enable -x?

#8Magnus Hagander
magnus@hagander.net
In reply to: Peter Eisentraut (#7)
1 attachment(s)
Re: pg_basebackup --xlog compatibility break

On Mon, Jun 4, 2012 at 2:14 PM, Peter Eisentraut <peter_e@gmx.net> wrote:

On tis, 2012-05-29 at 22:31 +0200, Magnus Hagander wrote:

Yeah, good arguments all around, i agree too :-) Next question is -
suggestions for naming of said paramter?

--xlog-method=something?  And/or -Xsomething, which would automatically
enable -x?

How's this?

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

Attachments:

basebackup_xlog_param.patchapplication/octet-stream; name=basebackup_xlog_param.patchDownload
diff --git a/doc/src/sgml/ref/pg_basebackup.sgml b/doc/src/sgml/ref/pg_basebackup.sgml
index 102d649..d03bedd 100644
--- a/doc/src/sgml/ref/pg_basebackup.sgml
+++ b/doc/src/sgml/ref/pg_basebackup.sgml
@@ -186,8 +186,8 @@ PostgreSQL documentation
      </varlistentry>
 
      <varlistentry>
-      <term><option>-x <replaceable class="parameter">method</replaceable></option></term>
-      <term><option>--xlog=<replaceable class="parameter">method</replaceable></option></term>
+      <term><option>-X <replaceable class="parameter">method</replaceable></option></term>
+      <term><option>--xlog-method=<replaceable class="parameter">method</replaceable></option></term>
       <listitem>
        <para>
         Includes the required transaction log files (WAL files) in the
@@ -238,6 +238,17 @@ PostgreSQL documentation
      </varlistentry>
 
      <varlistentry>
+      <term><option>-x</option></term>
+      <term><option>--xlog</option></term>
+      <listitem>
+       <para>
+        Using this option is equivalent of using <literal>-X</literal> with
+        method <literal>fetch</literal>.
+       </para>
+      </listitem>
+     </varlistentry>
+
+     <varlistentry>
       <term><option>-z</option></term>
       <term><option>--gzip</option></term>
       <listitem>
diff --git a/src/bin/pg_basebackup/pg_basebackup.c b/src/bin/pg_basebackup/pg_basebackup.c
index c3a0d89..438a5cc 100644
--- a/src/bin/pg_basebackup/pg_basebackup.c
+++ b/src/bin/pg_basebackup/pg_basebackup.c
@@ -106,7 +106,9 @@ usage(void)
 	printf(_("\nOptions controlling the output:\n"));
 	printf(_("  -D, --pgdata=DIRECTORY   receive base backup into directory\n"));
 	printf(_("  -F, --format=p|t         output format (plain (default), tar)\n"));
-	printf(_("  -x, --xlog=fetch|stream  include required WAL files in backup\n"));
+	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(_("  -z, --gzip               compress tar output\n"));
 	printf(_("  -Z, --compress=0-9       compress tar output with given compression level\n"));
 	printf(_("\nGeneral options:\n"));
@@ -1193,7 +1195,8 @@ main(int argc, char **argv)
 		{"pgdata", required_argument, NULL, 'D'},
 		{"format", required_argument, NULL, 'F'},
 		{"checkpoint", required_argument, NULL, 'c'},
-		{"xlog", required_argument, NULL, 'x'},
+		{"xlog", no_argument, NULL, 'x'},
+		{"xlog-method", required_argument, NULL, 'X'},
 		{"gzip", no_argument, NULL, 'z'},
 		{"compress", required_argument, NULL, 'Z'},
 		{"label", required_argument, NULL, 'l'},
@@ -1229,7 +1232,7 @@ main(int argc, char **argv)
 		}
 	}
 
-	while ((c = getopt_long(argc, argv, "D:F:x:l:zZ:c:h:p:U:s:wWvP",
+	while ((c = getopt_long(argc, argv, "D:F:xX:l:zZ:c:h:p:U:s:wWvP",
 							long_options, &option_index)) != -1)
 	{
 		switch (c)
@@ -1250,6 +1253,24 @@ main(int argc, char **argv)
 				}
 				break;
 			case 'x':
+				if (includewal)
+				{
+					fprintf(stderr, _("%s: cannot specify both --xlog and --xlog-method\n"),
+							progname);
+					exit(1);
+				}
+
+				includewal = true;
+				streamwal = false;
+				break;
+			case 'X':
+				if (includewal)
+				{
+					fprintf(stderr, _("%s: cannot specify both --xlog and --xlog-method\n"),
+							progname);
+					exit(1);
+				}
+
 				includewal = true;
 				if (strcmp(optarg, "f") == 0 ||
 					strcmp(optarg, "fetch") == 0)
@@ -1259,7 +1280,7 @@ main(int argc, char **argv)
 					streamwal = true;
 				else
 				{
-					fprintf(stderr, _("%s: invalid xlog option \"%s\", must be empty, \"fetch\", or \"stream\"\n"),
+					fprintf(stderr, _("%s: invalid xlog-method option \"%s\", must be empty, \"fetch\", or \"stream\"\n"),
 							progname, optarg);
 					exit(1);
 				}
#9Fujii Masao
masao.fujii@gmail.com
In reply to: Magnus Hagander (#8)
Re: pg_basebackup --xlog compatibility break

On Sun, Jun 10, 2012 at 8:43 PM, Magnus Hagander <magnus@hagander.net> wrote:

On Mon, Jun 4, 2012 at 2:14 PM, Peter Eisentraut <peter_e@gmx.net> wrote:

On tis, 2012-05-29 at 22:31 +0200, Magnus Hagander wrote:

Yeah, good arguments all around, i agree too :-) Next question is -
suggestions for naming of said paramter?

--xlog-method=something?  And/or -Xsomething, which would automatically
enable -x?

How's this?

Looks good to me.

Regards,

--
Fujii Masao

#10Magnus Hagander
magnus@hagander.net
In reply to: Fujii Masao (#9)
Re: pg_basebackup --xlog compatibility break

On Sun, Jun 10, 2012 at 2:48 PM, Fujii Masao <masao.fujii@gmail.com> wrote:

On Sun, Jun 10, 2012 at 8:43 PM, Magnus Hagander <magnus@hagander.net> wrote:

On Mon, Jun 4, 2012 at 2:14 PM, Peter Eisentraut <peter_e@gmx.net> wrote:

On tis, 2012-05-29 at 22:31 +0200, Magnus Hagander wrote:

Yeah, good arguments all around, i agree too :-) Next question is -
suggestions for naming of said paramter?

--xlog-method=something?  And/or -Xsomething, which would automatically
enable -x?

How's this?

Looks good to me.

Ok, thanks. Applied.

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

#11Peter Eisentraut
peter_e@gmx.net
In reply to: Magnus Hagander (#8)
Re: pg_basebackup --xlog compatibility break

On sön, 2012-06-10 at 13:43 +0200, Magnus Hagander wrote:

On Mon, Jun 4, 2012 at 2:14 PM, Peter Eisentraut <peter_e@gmx.net> wrote:

On tis, 2012-05-29 at 22:31 +0200, Magnus Hagander wrote:

Yeah, good arguments all around, i agree too :-) Next question is -
suggestions for naming of said paramter?

--xlog-method=something? And/or -Xsomething, which would automatically
enable -x?

How's this?

I wouldn't make -x and -X exclusive. The way I understood this is, -x
means include xlog, and -X says how to.

I guess either way of looking at it has its merits.

#12Magnus Hagander
magnus@hagander.net
In reply to: Peter Eisentraut (#11)
Re: pg_basebackup --xlog compatibility break

On Mon, Jun 11, 2012 at 11:28 PM, Peter Eisentraut <peter_e@gmx.net> wrote:

On sön, 2012-06-10 at 13:43 +0200, Magnus Hagander wrote:

On Mon, Jun 4, 2012 at 2:14 PM, Peter Eisentraut <peter_e@gmx.net> wrote:

On tis, 2012-05-29 at 22:31 +0200, Magnus Hagander wrote:

Yeah, good arguments all around, i agree too :-) Next question is -
suggestions for naming of said paramter?

--xlog-method=something?  And/or -Xsomething, which would automatically
enable -x?

How's this?

I wouldn't make -x and -X exclusive.  The way I understood this is, -x
means include xlog, and -X says how to.

I guess either way of looking at it has its merits.

I guess it's basically two ways of doing the same thing. I'm not
especially attached to either one of them, so if you think the ohter
one is better, I won't object to changing it.

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

#13Thom Brown
thom@linux.com
In reply to: Magnus Hagander (#12)
Re: pg_basebackup --xlog compatibility break

On 11 June 2012 22:40, Magnus Hagander <magnus@hagander.net> wrote:

On Mon, Jun 11, 2012 at 11:28 PM, Peter Eisentraut <peter_e@gmx.net> wrote:

On sön, 2012-06-10 at 13:43 +0200, Magnus Hagander wrote:

On Mon, Jun 4, 2012 at 2:14 PM, Peter Eisentraut <peter_e@gmx.net> wrote:

On tis, 2012-05-29 at 22:31 +0200, Magnus Hagander wrote:

Yeah, good arguments all around, i agree too :-) Next question is -
suggestions for naming of said paramter?

--xlog-method=something?  And/or -Xsomething, which would automatically
enable -x?

How's this?

I wouldn't make -x and -X exclusive.  The way I understood this is, -x
means include xlog, and -X says how to.

I guess either way of looking at it has its merits.

I guess it's basically two ways of doing the same thing. I'm not
especially attached to either one of them, so if you think the ohter
one is better, I won't object to changing it.

+1 for not telling the user off for being explicit by stating both options.

--
Thom

#14Fujii Masao
masao.fujii@gmail.com
In reply to: Peter Eisentraut (#11)
Re: pg_basebackup --xlog compatibility break

On Tue, Jun 12, 2012 at 6:28 AM, Peter Eisentraut <peter_e@gmx.net> wrote:

On sön, 2012-06-10 at 13:43 +0200, Magnus Hagander wrote:

On Mon, Jun 4, 2012 at 2:14 PM, Peter Eisentraut <peter_e@gmx.net> wrote:

On tis, 2012-05-29 at 22:31 +0200, Magnus Hagander wrote:

Yeah, good arguments all around, i agree too :-) Next question is -
suggestions for naming of said paramter?

--xlog-method=something?  And/or -Xsomething, which would automatically
enable -x?

How's this?

I wouldn't make -x and -X exclusive.  The way I understood this is, -x
means include xlog, and -X says how to.

And we can specify -X without -x like -Z and -z option of pg_basebackup?
If no, I disagree with you because I don't want to specify two options to
use log streaming method.

Regards,

--
Fujii Masao