Thread: proposal: pg_restore --convert-to-text
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)
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
Attachment
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
Attachment
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
Attachment
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