Thread: proposal: pg_restore --convert-to-text

proposal: pg_restore --convert-to-text

From
Andrew Gierth
Date:
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 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.

Opinions?

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

-- 
Andrew (irc:RhodiumToad)


Re: proposal: pg_restore --convert-to-text

From
Euler Taveira
Date:
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


Re: proposal: pg_restore --convert-to-text

From
Tom Lane
Date:
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


Re: proposal: pg_restore --convert-to-text

From
Andreas Karlsson
Date:
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


Re: proposal: pg_restore --convert-to-text

From
Euler Taveira
Date:
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

Attachment

Re: proposal: pg_restore --convert-to-text

From
Tom Lane
Date:
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


Re: proposal: pg_restore --convert-to-text

From
Euler Taveira
Date:
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

Attachment

RE: proposal: pg_restore --convert-to-text

From
"Imai, Yoshikazu"
Date:
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


RE: proposal: pg_restore --convert-to-text

From
"José Arthur Benetasso Villanova"
Date:

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


RE: proposal: pg_restore --convert-to-text

From
"Imai, Yoshikazu"
Date:
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

Re: proposal: pg_restore --convert-to-text

From
Euler Taveira
Date:
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

Attachment

Re: proposal: pg_restore --convert-to-text

From
"José Arthur Benetasso Villanova"
Date:

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

RE: proposal: pg_restore --convert-to-text

From
"Imai, Yoshikazu"
Date:
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

RE: proposal: pg_restore --convert-to-text

From
"Imai, Yoshikazu"
Date:
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


Re: proposal: pg_restore --convert-to-text

From
Alvaro Herrera
Date:
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



RE: proposal: pg_restore --convert-to-text

From
"Daniel Verite"
Date:
    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



Re: proposal: pg_restore --convert-to-text

From
Andrew Gierth
Date:
>>>>> "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)



Re: proposal: pg_restore --convert-to-text

From
Alvaro Herrera
Date:
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
  ...



Re: proposal: pg_restore --convert-to-text

From
"Daniel Verite"
Date:
    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