invalid combination of options "-D - -F t -X stream" in pg_basebackup

Started by Fujii Masaoabout 9 years ago5 messages

masao.fujii@gmail.com

about 9 years ago

Hi,

Isn't it better to forbid the conbination of the options "-D -", "-F t" and
"-X stream" in pg_basebackup? This is obviously invalid setting and the docs
warns this as follows. But currently users can specify such setting and
pg_basebackup can exit unexpectedly with an error.

-----------------------
If the value - (dash) is specified as target directory, the tar contents will
be written to standard output, suitable for piping to for example gzip.
This is only possible if the cluster has no additional tablespaces.
-----------------------

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

Magnus Hagander

magnus@hagander.net

about 9 years ago

In reply to: Fujii Masao (#1)

Re: invalid combination of options "-D - -F t -X stream" in pg_basebackup

On Mon, Dec 19, 2016 at 5:39 PM, Fujii Masao <masao.fujii@gmail.com> wrote:

Hi,

Isn't it better to forbid the conbination of the options "-D -", "-F t" and
"-X stream" in pg_basebackup? This is obviously invalid setting and the
docs
warns this as follows. But currently users can specify such setting and
pg_basebackup can exit unexpectedly with an error.

-----------------------
If the value - (dash) is specified as target directory, the tar contents
will
be written to standard output, suitable for piping to for example gzip.
This is only possible if the cluster has no additional tablespaces.
-----------------------

Yes, definitely. I'd say that's an oversight in implementing the support
for stream-to-tar that it did not detect this issue.

Do you want to provide a patch for it, or should I?

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

Fujii Masao

masao.fujii@gmail.com

about 9 years ago

In reply to: Magnus Hagander (#2)

1 attachment(s)

Re: invalid combination of options "-D - -F t -X stream" in pg_basebackup

On Tue, Dec 20, 2016 at 1:43 AM, Magnus Hagander <magnus@hagander.net> wrote:

On Mon, Dec 19, 2016 at 5:39 PM, Fujii Masao <masao.fujii@gmail.com> wrote:

Hi,

Isn't it better to forbid the conbination of the options "-D -", "-F t"
and
"-X stream" in pg_basebackup? This is obviously invalid setting and the
docs
warns this as follows. But currently users can specify such setting and
pg_basebackup can exit unexpectedly with an error.

-----------------------
If the value - (dash) is specified as target directory, the tar contents
will
be written to standard output, suitable for piping to for example gzip.
This is only possible if the cluster has no additional tablespaces.
-----------------------

Yes, definitely. I'd say that's an oversight in implementing the support for
stream-to-tar that it did not detect this issue.

Do you want to provide a patch for it, or should I?

What about the attached patch?

+        fprintf(stderr, _("Try \"%s --help\" for more information.\n"),
+                progname);

I added the above hint message because other codes checking invalid
options also have such hint messages. But there is no additional
useful information about valid combination of options in the help
messages, so I'm a bit tempted to remove the above hint message.

Regards,

--
Fujii Masao

Attachments:

invalid-options-basebackup.patchtext/x-patch; charset=US-ASCII; name=invalid-options-basebackup.patchDownload

*** a/src/bin/pg_basebackup/pg_basebackup.c
--- b/src/bin/pg_basebackup/pg_basebackup.c
***************
*** 2268,2273 **** main(int argc, char **argv)
--- 2268,2283 ----
  		exit(1);
  	}
  
+ 	if (format == 't' && streamwal && strcmp(basedir, "-") == 0)
+ 	{
+ 		fprintf(stderr,
+ 			_("%s: cannot stream transaction logs in tar mode to stdout\n"),
+ 				progname);
+ 		fprintf(stderr, _("Try \"%s --help\" for more information.\n"),
+ 				progname);
+ 		exit(1);
+ 	}
+ 
  	if (replication_slot && !streamwal)
  	{
  		fprintf(stderr,

Magnus Hagander

magnus@hagander.net

about 9 years ago

In reply to: Fujii Masao (#3)

Re: invalid combination of options "-D - -F t -X stream" in pg_basebackup

On Tue, Dec 20, 2016 at 6:56 AM, Fujii Masao <masao.fujii@gmail.com> wrote:

On Tue, Dec 20, 2016 at 1:43 AM, Magnus Hagander <magnus@hagander.net>
wrote:

On Mon, Dec 19, 2016 at 5:39 PM, Fujii Masao <masao.fujii@gmail.com>

wrote:

Hi,

Isn't it better to forbid the conbination of the options "-D -", "-F t"
and
"-X stream" in pg_basebackup? This is obviously invalid setting and the
docs
warns this as follows. But currently users can specify such setting and
pg_basebackup can exit unexpectedly with an error.

-----------------------
If the value - (dash) is specified as target directory, the tar contents
will
be written to standard output, suitable for piping to for example gzip.
This is only possible if the cluster has no additional tablespaces.
-----------------------

Yes, definitely. I'd say that's an oversight in implementing the support

for

stream-to-tar that it did not detect this issue.

Do you want to provide a patch for it, or should I?

What about the attached patch?
+        fprintf(stderr, _("Try \"%s --help\" for more information.\n"),
+                progname);
I added the above hint message because other codes checking invalid
options also have such hint messages. But there is no additional
useful information about valid combination of options in the help
messages, so I'm a bit tempted to remove the above hint message.

Looks good to me.

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

Fujii Masao

masao.fujii@gmail.com

about 9 years ago

In reply to: Magnus Hagander (#4)

Re: invalid combination of options "-D - -F t -X stream" in pg_basebackup

On Tue, Dec 20, 2016 at 9:22 PM, Magnus Hagander <magnus@hagander.net> wrote:

On Tue, Dec 20, 2016 at 6:56 AM, Fujii Masao <masao.fujii@gmail.com> wrote:
On Tue, Dec 20, 2016 at 1:43 AM, Magnus Hagander <magnus@hagander.net>
wrote:

On Mon, Dec 19, 2016 at 5:39 PM, Fujii Masao <masao.fujii@gmail.com>
wrote:

Hi,

Isn't it better to forbid the conbination of the options "-D -", "-F t"
and
"-X stream" in pg_basebackup? This is obviously invalid setting and the
docs
warns this as follows. But currently users can specify such setting and
pg_basebackup can exit unexpectedly with an error.

-----------------------
If the value - (dash) is specified as target directory, the tar
contents
will
be written to standard output, suitable for piping to for example gzip.
This is only possible if the cluster has no additional tablespaces.
-----------------------

Yes, definitely. I'd say that's an oversight in implementing the support
for
stream-to-tar that it did not detect this issue.

Do you want to provide a patch for it, or should I?

What about the attached patch?
+        fprintf(stderr, _("Try \"%s --help\" for more information.\n"),
+                progname);
I added the above hint message because other codes checking invalid
options also have such hint messages. But there is no additional
useful information about valid combination of options in the help
messages, so I'm a bit tempted to remove the above hint message.
Looks good to me.

Thanks for the review! Pushed.

I left the hint message for consistency with other code.

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