proposal: pg_restore --convert-to-text

Em qua, 13 de fev de 2019 às 19:56, Andrew Gierth
<andrew@tao11.riddles.org.uk> escreveu:

One of the remarkably common user errors with pg_restore is users
leaving off the -d option. (We get cases of it regularly on the IRC
channel, including one just now which prompted me to finally propose
this)

I'm not sure it is a common error. If you want to restore schema
and/or data it is natural that I should specify the database name (or
at least PGDATABASE).

I propose we add a new option: --convert-to-text or some such name, and
then make pg_restore throw a usage error if neither -d nor the new
option is given.

However, I agree that pg_restore to stdout if -d wasn't specified is
not a good default. The current behavior is the same as "-f -"
(however, pg_restore doesn't allow - meaning stdout). Isn't it the
case to error out if -d or -f wasn't specified? If we go to this road,
-f option should allow - (stdout) as parameter.

(Yes, it will break the scripts of anyone who is currently scripting
pg_restore to output SQL text. How many people do that?)

I use pg_restore to stdout a lot but I wouldn't bother to specify an
option to get it (such as pg_restore -Fc -f - /tmp/foo.dmp).

--
Euler Taveira Timbira -
http://www.timbira.com.br/
PostgreSQL: Consultoria, Desenvolvimento, Suporte 24x7 e Treinamento

Euler Taveira <euler@timbira.com.br> writes:

Em qua, 13 de fev de 2019 às 19:56, Andrew Gierth
<andrew@tao11.riddles.org.uk> escreveu:

I propose we add a new option: --convert-to-text or some such name, and
then make pg_restore throw a usage error if neither -d nor the new
option is given.

However, I agree that pg_restore to stdout if -d wasn't specified is
not a good default. The current behavior is the same as "-f -"
(however, pg_restore doesn't allow - meaning stdout). Isn't it the
case to error out if -d or -f wasn't specified? If we go to this road,
-f option should allow - (stdout) as parameter.

I won't take a position on whether it's okay to break backwards
compatibility here (although I can think of some people who are
likely to complain ;-)). But if we decide to do it, I like
Euler's suggestion for how to do it. A separate --convert-to-text
switch seems kind of ugly, plus if that's the approach it'd be hard
to write scripts that work with different pg_restore versions.

The idea of cross-version-compatible scripts suggests that we
might consider back-patching the addition of "-f -", though not
the change that says you must write it.

regards, tom lane

On 14/02/2019 01.31, Euler Taveira wrote:

Em qua, 13 de fev de 2019 às 19:56, Andrew Gierth
<andrew@tao11.riddles.org.uk> escreveu:

I propose we add a new option: --convert-to-text or some such name, and
then make pg_restore throw a usage error if neither -d nor the new
option is given.

However, I agree that pg_restore to stdout if -d wasn't specified is
not a good default. The current behavior is the same as "-f -"
(however, pg_restore doesn't allow - meaning stdout). Isn't it the
case to error out if -d or -f wasn't specified? If we go to this road,
-f option should allow - (stdout) as parameter.

Agreed, "-f -" would be acceptable. I use pg_restore to stdout a lot,
but almost always manually and would have to have to remember and type
"--convert-to-text".

Andreas

Em qui, 14 de fev de 2019 às 22:41, Andreas Karlsson
<andreas@proxel.se> escreveu:

Agreed, "-f -" would be acceptable. I use pg_restore to stdout a lot,
but almost always manually and would have to have to remember and type
"--convert-to-text".

Since no one has stepped up, I took a stab at it. It will prohibit
standard output unless '-f -' be specified. -l option also has the
same restriction.

It breaks backward compatibility and as Tom suggested a variant of
this patch (without docs) should be applied to back branches.

--
Euler Taveira Timbira -
http://www.timbira.com.br/
PostgreSQL: Consultoria, Desenvolvimento, Suporte 24x7 e Treinamento

Euler Taveira <euler@timbira.com.br> writes:

Since no one has stepped up, I took a stab at it. It will prohibit
standard output unless '-f -' be specified. -l option also has the
same restriction.

Hm, don't really see the need to break -l usage here.

Pls add to next CF, if you didn't already.

regards, tom lane

Em seg, 18 de fev de 2019 às 19:21, Tom Lane <tgl@sss.pgh.pa.us> escreveu:

Euler Taveira <euler@timbira.com.br> writes:

Since no one has stepped up, I took a stab at it. It will prohibit
standard output unless '-f -' be specified. -l option also has the
same restriction.

Hm, don't really see the need to break -l usage here.

After thinking about it, revert it.

Pls add to next CF, if you didn't already.

Done.

--
Euler Taveira Timbira -
http://www.timbira.com.br/
PostgreSQL: Consultoria, Desenvolvimento, Suporte 24x7 e Treinamento

Hi,

On Tue, Feb 19, 2019 at 8:20 PM, Euler Taveira wrote:

Em seg, 18 de fev de 2019 às 19:21, Tom Lane <tgl@sss.pgh.pa.us> escreveu:

Euler Taveira <euler@timbira.com.br> writes:

Since no one has stepped up, I took a stab at it. It will prohibit
standard output unless '-f -' be specified. -l option also has the
same restriction.

Hm, don't really see the need to break -l usage here.

After thinking about it, revert it.

Pls add to next CF, if you didn't already.

Done.

I saw the patch.

Is there no need to rewrite the Description in the Doc to state we should specify either -d or -f option?
(and also it might be better to write if -l option is given, neither -d nor -f option isn't necessarily needed.)

I also have the simple question in the code.

I thought the below if-else condition

+	if (filename && strcmp(filename, "-") == 0)
+		fn = fileno(stdout);
+	else if (filename)
 		fn = -1;
    else if (AH->FH)

can also be written by the form below.

if (filename)
{
if(strcmp(filename, "-") == 0)
fn = fileno(stdout);
else
fn = -1;
}
else if (AH->FH)

I think the former one looks like pretty, but which one is preffered?

--
Yoshikazu Imai

On Thu, 28 Feb 2019, Imai, Yoshikazu wrote:

Is there no need to rewrite the Description in the Doc to state we should specify either -d or -f option?
(and also it might be better to write if -l option is given, neither -d nor -f option isn't necessarily needed.)

Since the default part of text was removed, looks ok to me.

I also have the simple question in the code.

I thought the below if-else condition

+	if (filename && strcmp(filename, "-") == 0)
+		fn = fileno(stdout);
+	else if (filename)
fn = -1;
else if (AH->FH)

can also be written by the form below.

if (filename)
{
if(strcmp(filename, "-") == 0)
fn = fileno(stdout);
else
fn = -1;
}
else if (AH->FH)

I think the former one looks like pretty, but which one is preffered?

Aside the above question, I tested the code against a up-to-date
repository. It compiled, worked as expected and passed all tests.

--
Jose Arthur Benetasso Villanova

Hi Jose,

Sorry for my late reply.

On Wed, Mar 6, 2019 at 10:58 AM, José Arthur Benetasso Villanova wrote:

On Thu, 28 Feb 2019, Imai, Yoshikazu wrote:

Is there no need to rewrite the Description in the Doc to state we should

specify either -d or -f option?

(and also it might be better to write if -l option is given, neither
-d nor -f option isn't necessarily needed.)

Since the default part of text was removed, looks ok to me.

Ah, yeah, after looking again, I also think it's ok.

I also have the simple question in the code.

I thought the below if-else condition

+	if (filename && strcmp(filename, "-") == 0)
+		fn = fileno(stdout);
+	else if (filename)
fn = -1;
else if (AH->FH)

can also be written by the form below.

if (filename)
{
if(strcmp(filename, "-") == 0)
fn = fileno(stdout);
else
fn = -1;
}
else if (AH->FH)

I think the former one looks like pretty, but which one is preffered?

Aside the above question, I tested the code against a up-to-date
repository. It compiled, worked as expected and passed all tests.

It still can be applied to HEAD by cfbot.

Upon committing this, we have to care this patch break backwards compatibility, but I haven't seen any complaints so far. If there are no objections, I will set this patch to ready for committer.

--
Yoshikazu Imai

Em qua, 27 de fev de 2019 às 23:48, Imai, Yoshikazu
<imai.yoshikazu@jp.fujitsu.com> escreveu:

Is there no need to rewrite the Description in the Doc to state we should specify either -d or -f option?
(and also it might be better to write if -l option is given, neither -d nor -f option isn't necessarily needed.)

I don't think so. The description is already there (see "pg_restore
can operate in two modes..."). I left -l as is which means that
'pg_restore -l foo.dump' dumps to standard output and 'pg_restore -f -
-l foo.dump' has the same behavior).

I think the former one looks like pretty, but which one is preffered?

I don't have a style preference but decided to change to your
suggestion. New version attached.

--
Euler Taveira Timbira -
http://www.timbira.com.br/
PostgreSQL: Consultoria, Desenvolvimento, Suporte 24x7 e Treinamento

On Sat, 16 Mar 2019, Euler Taveira wrote:

I think the former one looks like pretty, but which one is preffered?

I don't have a style preference but decided to change to your
suggestion. New version attached.

Again, the patch compiles and works as expected.

--
José Arthur Benetasso Villanova

On Sat, Mar 16, 2019 at 10:55 PM, Euler Taveira wrote:

Is there no need to rewrite the Description in the Doc to state we should

specify either -d or -f option?

(and also it might be better to write if -l option is given, neither
-d nor -f option isn't necessarily needed.)

I don't think so. The description is already there (see "pg_restore can
operate in two modes..."). I left -l as is which means that 'pg_restore
-l foo.dump' dumps to standard output and 'pg_restore -f - -l foo.dump'
has the same behavior).

Ah, I understand it.

I think the former one looks like pretty, but which one is preffered?

I don't have a style preference but decided to change to your suggestion.
New version attached.

I checked it. It may be a trivial matter, so thanks for taking it consideration.

--
Yoshikazu Imai

On Fri, Mar 15, 2019 at 6:20 AM, Imai, Yoshikazu wrote:

Upon committing this, we have to care this patch break backwards
compatibility, but I haven't seen any complaints so far. If there are
no objections, I will set this patch to ready for committer.

Jose had set this to ready for committer. Thanks.

--
Yoshikazu Imai

I just pushed it with trivial changes:

* rebased for cc8d41511721

* changed wording in the error message

* added a new test for the condition in t/001_basic.pl

* Added the "-" in the --help line of -f.

Andrew G. never confirmed that this change is sufficient to appease
users being confused by the previous behavior. I hope it is ...

Thanks!

--
�lvaro Herrera https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

José Arthur Benetasso Villanova wrote:

On Thu, 28 Feb 2019, Imai, Yoshikazu wrote:

Is there no need to rewrite the Description in the Doc to state we should specify either -d or -f option?
(and also it might be better to write if -l option is given, neither -d nor -f option isn't necessarily needed.)

Since the default part of text was removed, looks ok to me.

[4 months later]

While testing pg_restore on v12, I'm stumbling on this too.
pg_restore without argument fails like that:

$ pg_restore
pg_restore: error: one of -d/--dbname and -f/--file must be specified

But that's not right since "pg_restore -l dumpfile" would work.
I don't understand why the discussion seems to conclude that it's okay.

Also, in the doc at https://www.postgresql.org/docs/devel/app-pgrestore.html
the synopsis is

pg_restore [connection-option...] [option...] [filename]

so the invocation without argument seems possible while in fact
it's not.

On a subjective note, I must say that I don't find the change to be
helpful.
In particular saying that --file must be specified leaves me with
no idea what file it's talking about: the dumpfile or the output file?
My first inclination is to transform "pg_restore dumpfile" into
"pg_restore -f dumpfile", which does nothing but wait
(it waits for the dumpfile on the standard input, before
I realize that it's not working and hit ^C. Fortunately it doesn't
overwrite the dumpfile with an empty output).

So the change in v12 removes the standard output as default,
but does not remove the standard input as default.
Isn't that weird?

Best regards,
--
Daniel Vérité
PostgreSQL-powered mailer: http://www.manitou-mail.org
Twitter: @DanielVerite

"Daniel" == Daniel Verite <daniel@manitou-mail.org> writes:

Daniel> While testing pg_restore on v12, I'm stumbling on this too.
Daniel> pg_restore without argument fails like that:

Daniel> $ pg_restore
Daniel> pg_restore: error: one of -d/--dbname and -f/--file must be specified

Yeah, that's not good.

How about:

pg_restore: error: no destination specified for restore
pg_restore: error: use -d/--dbname to restore into a database, or -f/--file to output SQL text

--
Andrew (irc:RhodiumToad)

On 2019-Jun-12, Daniel Verite wrote:

While testing pg_restore on v12, I'm stumbling on this too.

Thanks for testing.

pg_restore without argument fails like that:

$ pg_restore
pg_restore: error: one of -d/--dbname and -f/--file must be specified

But that's not right since "pg_restore -l dumpfile" would work.

So you suggest that it should be

pg_restore: error: one of -d/--dbname, -f/--file and -l/--list must be specified
?

Also, in the doc at https://www.postgresql.org/docs/devel/app-pgrestore.html
the synopsis is

pg_restore [connection-option...] [option...] [filename]

so the invocation without argument seems possible while in fact
it's not.

So you suggest that it should be
pg_restore [connection-option...] { -d | -f | -l } [option...] [filename]
?

Maybe we should do that and split out the "output destination options"
from other options in the list of options, to make this clearer; see
a proposal after my sig.

In particular saying that --file must be specified leaves me with
no idea what file it's talking about: the dumpfile or the output file?

If you want to submit a patch (for pg13) to rename --file to
--output-file (and make --file an alias of that), you're welcome to, and
endure the resulting discussion and possible rejection. I don't think
we're changing that at this point of pg12.

My first inclination is to transform "pg_restore dumpfile" into
"pg_restore -f dumpfile", which does nothing but wait
(it waits for the dumpfile on the standard input, before
I realize that it's not working and hit ^C. Fortunately it doesn't
overwrite the dumpfile with an empty output).

Would you have it emit to stderr a message saying "reading standard
input" when it is?

So the change in v12 removes the standard output as default,
but does not remove the standard input as default.
Isn't that weird?

I don't think they have the same surprise factor.

--
�lvaro Herrera https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Usage:
pg_restore [connection-option...] { -d | -f | -l } [option...] [filename]

General options:
-F, --format=c|d|t backup file format (should be automatic)
-v, --verbose verbose mode
-V, --version output version information, then exit
-?, --help show this help, then exit

Output target options:
-l, --list print summarized TOC of the archive
-d, --dbname=NAME connect to database name
-f, --file=FILENAME output file name (- for stdout)

Options controlling the restore:
-a, --data-only restore only the data, no schema
-c, --clean clean (drop) database objects before recreating
...

Alvaro Herrera wrote:

So you suggest that it should be

pg_restore: error: one of -d/--dbname, -f/--file and -l/--list must be
specified
?

I'd suggest this minimal fix :

(int argc, char **argv)
	/* Complain if neither -f nor -d was specified (except if dumping
TOC) */
	if (!opts->dbname && !opts->filename && !opts->tocSummary)
	{
-		pg_log_error("one of -d/--dbname and -f/--file must be
specified");
+		pg_log_error("-d/--dbname or -f/--file or -l/--list must be
specified");
+		fprintf(stderr, _("Try \"%s --help\" for more
information.\n"),
+				progname);
		exit_nicely(1);
	}

So you suggest that it should be
pg_restore [connection-option...] { -d | -f | -l } [option...] [filename]
?

Looking at the other commands, it doesn't seem that we use this form for
any of those that require at least one argument, for instance:

===
$ ./pg_basebackup
pg_basebackup: error: no target directory specified
Try "pg_basebackup --help" for more information.

$ ./pg_basebackup --help
pg_basebackup takes a base backup of a running PostgreSQL server.

Usage:
pg_basebackup [OPTION]...

$ ./pg_checksums
pg_checksums: error: no data directory specified
Try "pg_checksums --help" for more information.

$ ./pg_checksums --help
pg_checksums enables, disables, or verifies data checksums in a PostgreSQL
database cluster.

Usage:
pg_checksums [OPTION]... [DATADIR]

$ ./pg_rewind
pg_rewind: error: no source specified (--source-pgdata or --source-server)
Try "pg_rewind --help" for more information.

$ ./pg_rewind --help
pg_rewind resynchronizes a PostgreSQL cluster with another copy of the
cluster.

Usage:
pg_rewind [OPTION]...
===

"pg_restore [OPTION]... [FILE]" appears to be consistent with those, even
with the new condition that no option is an error, so it's probably okay.

Output target options:
-l, --list print summarized TOC of the archive
-d, --dbname=NAME connect to database name
-f, --file=FILENAME output file name (- for stdout)

That makes sense. I would put this section first, before
"General options".

Best regards,
--
Daniel Vérité
PostgreSQL-powered mailer: http://www.manitou-mail.org
Twitter: @DanielVerite