proposal: pg_restore --convert-to-text

classic Classic list List threaded Threaded
18 messages Options
Reply | Threaded
Open this post in threaded view
|

proposal: pg_restore --convert-to-text

Andrew Gierth
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)

Reply | Threaded
Open this post in threaded view
|

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

Euler Taveira
Em qua, 13 de fev de 2019 às 19:56, Andrew Gierth
<[hidden email]> 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

Reply | Threaded
Open this post in threaded view
|

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

Tom Lane-2
Euler Taveira <[hidden email]> writes:
> Em qua, 13 de fev de 2019 às 19:56, Andrew Gierth
> <[hidden email]> 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

Reply | Threaded
Open this post in threaded view
|

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

Andreas Karlsson
In reply to this post by Euler Taveira
On 14/02/2019 01.31, Euler Taveira wrote:

> Em qua, 13 de fev de 2019 às 19:56, Andrew Gierth
> <[hidden email]> 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

Reply | Threaded
Open this post in threaded view
|

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

Euler Taveira
Em qui, 14 de fev de 2019 às 22:41, Andreas Karlsson
<[hidden email]> 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

0001-pg_restore-supports-stdout-in-file.patch (8K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

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

Tom Lane-2
Euler Taveira <[hidden email]> 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

Reply | Threaded
Open this post in threaded view
|

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

Euler Taveira
Em seg, 18 de fev de 2019 às 19:21, Tom Lane <[hidden email]> escreveu:
>
> Euler Taveira <[hidden email]> 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

v2-0001-pg_restore-supports-stdout-in-file.patch (6K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

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

Imai, Yoshikazu
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 <[hidden email]> escreveu:
> >
> > Euler Taveira <[hidden email]> 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

Reply | Threaded
Open this post in threaded view
|

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

José Arthur Benetasso Villanova


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

Reply | Threaded
Open this post in threaded view
|

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

Imai, Yoshikazu
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
Reply | Threaded
Open this post in threaded view
|

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

Euler Taveira
In reply to this post by Imai, Yoshikazu
Em qua, 27 de fev de 2019 às 23:48, Imai, Yoshikazu
<[hidden email]> 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

v3-0001-pg_restore-supports-stdout-in-file.patch (6K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

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

José Arthur Benetasso Villanova


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
Reply | Threaded
Open this post in threaded view
|

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

Imai, Yoshikazu
In reply to this post by Euler Taveira
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
Reply | Threaded
Open this post in threaded view
|

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

Imai, Yoshikazu
In reply to this post by Imai, Yoshikazu
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

Reply | Threaded
Open this post in threaded view
|

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

Alvaro Herrera-9
In reply to this post by Euler Taveira
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


Reply | Threaded
Open this post in threaded view
|

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

Daniel Verite
In reply to this post by José Arthur Benetasso Villanova
        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


Reply | Threaded
Open this post in threaded view
|

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

Andrew Gierth
>>>>> "Daniel" == Daniel Verite <[hidden email]> 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)


Reply | Threaded
Open this post in threaded view
|

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

Alvaro Herrera-9
In reply to this post by Daniel Verite
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
  ...