proposal: pg_restore --convert-to-text

classic Classic list List threaded Threaded
14 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