Re: detect custom-format dumps in psql and emit a useful error

Hi all

Someone recently mentioned that there's no generate_series(numeric,
numeric, numeric) .

That strikes me as a great candidate for a
new-developer-learning-PostgreSQL TODO.

A couple of other things I occasionally run into that'd fit the bill:

* A user-level elog(...) / ereport(...) function callable from SQL.
Useful in CASE statements.

* A log_ option to log whenever pg switches to a new xlog segment.

* A 'hex' option to 'decode' that decodes regular hex into bytea, or an
equivalent decode_hex / hex_decode . That's for plain undecorated hex,
not \x literals.

* A corresponding encode_hex or hex_encode to emit hex 'text' without \x
prefix (not a bytea literal)

(Yes, I know you can form bytea literals with concatenation and decode
that way, and can strip the \x prefix from a literal on output, but it's
often pretty awkward).

* A user-accessible function to decode unicode escapes like \U1011 in
strings.

* A function that converts a json array to a PostgreSQL array of a given
type if all json members are compatible with the type

* Expanding the set of json/jsonb operations to introduce features that
people are used to from jquery, mongo, etc.
Replace-key-if-exists-without-adding, add-or-replace-key, etc.

* (not really Pg proper, but enough users run into this that I think we
should encourage interested people to tackle it): In PgAdmin-III either
support \copy, \c, etc or detect their use and emit an informative error
telling the user to use 'psql'.

* When a user tries to run "psql -f some_custom_format_backup", detect
this and emit a useful error message. Ditto stdin.

* Add a built-in aggregate for array_agg(anyarray), i.e. build an array
of dims n+1 from the input arrays of dims n. For n=1 this can be done
with a simple SQL level aggregate definition, so all it really needs is
to error on dims > 1 IMO.

* Add a built-in aggregate form of array_cat

... probably other things I'm forgetting.

Worth adding some to the TODO marked beginner?

--
Craig Ringer http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

On Tue, Jun 24, 2014 at 10:58:54AM +0800, Craig Ringer wrote:
> Hi all
>
> Someone recently mentioned that there's no generate_series(numeric,
> numeric, numeric) .
>
> That strikes me as a great candidate for a
> new-developer-learning-PostgreSQL TODO.
>
>
> A couple of other things I occasionally run into that'd fit the bill:
>
> * A user-level elog(...) / ereport(...) function callable from SQL.
> Useful in CASE statements.
>
> * A log_ option to log whenever pg switches to a new xlog segment.

The above seem good.

>
> * A 'hex' option to 'decode' that decodes regular hex into bytea, or an
> equivalent decode_hex / hex_decode . That's for plain undecorated hex,
> not \x literals.
>
> * A corresponding encode_hex or hex_encode to emit hex 'text' without \x
> prefix (not a bytea literal)
>
> (Yes, I know you can form bytea literals with concatenation and decode
> that way, and can strip the \x prefix from a literal on output, but it's
> often pretty awkward).

Uh, don't our SQL string function allow removal of \x?

> * A user-accessible function to decode unicode escapes like \U1011 in
> strings.

Makes sense.

> * A function that converts a json array to a PostgreSQL array of a given
> type if all json members are compatible with the type
>
> * Expanding the set of json/jsonb operations to introduce features that
> people are used to from jquery, mongo, etc.
> Replace-key-if-exists-without-adding, add-or-replace-key, etc.
>
> * (not really Pg proper, but enough users run into this that I think we
> should encourage interested people to tackle it): In PgAdmin-III either
> support \copy, \c, etc or detect their use and emit an informative error
> telling the user to use 'psql'.

I think you have to ask Andrew on these.

> * When a user tries to run "psql -f some_custom_format_backup", detect
> this and emit a useful error message. Ditto stdin.

Uh, good idea, but can we really do that in psql?

> * Add a built-in aggregate for array_agg(anyarray), i.e. build an array
> of dims n+1 from the input arrays of dims n. For n=1 this can be done
> with a simple SQL level aggregate definition, so all it really needs is
> to error on dims > 1 IMO.
>
> * Add a built-in aggregate form of array_cat
>
> ... probably other things I'm forgetting.

No idea on these.

--
Bruce Momjian <bruce(at)momjian(dot)us> http://momjian.us
EnterpriseDB http://enterprisedb.com

+ Everyone has their own god. +

On 07/29/2014 10:43 AM, Bruce Momjian wrote:

>> * A function that converts a json array to a PostgreSQL array of a given
>> type if all json members are compatible with the type
>>
>> * Expanding the set of json/jsonb operations to introduce features that
>> people are used to from jquery, mongo, etc.
>> Replace-key-if-exists-without-adding, add-or-replace-key, etc.
>>
>>
> I think you have to ask Andrew on these.

Both these might be possible. I am not planning on doing them, at least.
My current json plans for 9.5 are limited to implementing jsonb
equivalents of those json functions that didn't make it into the 9.4
jsonb work due to pressure of time, i.e. the json generating functions
and the aggregates. That work has been started and with luck will hit
the next commitfest.

cheers

andrew

On Wed, Jul 30, 2014 at 2:37 PM, Andrew Dunstan <andrew(at)dunslane(dot)net> wrote:
> Both these might be possible. I am not planning on doing them, at least. My
> current json plans for 9.5 are limited to implementing jsonb equivalents of
> those json functions that didn't make it into the 9.4 jsonb work due to
> pressure of time, i.e. the json generating functions and the aggregates.
> That work has been started and with luck will hit the next commitfest.

Does that include the concatenate operator? That's probably the single
biggest thing we missed.

--
Peter Geoghegan

On 07/30/2014 06:11 PM, Peter Geoghegan wrote:
> On Wed, Jul 30, 2014 at 2:37 PM, Andrew Dunstan <andrew(at)dunslane(dot)net> wrote:
>> Both these might be possible. I am not planning on doing them, at least. My
>> current json plans for 9.5 are limited to implementing jsonb equivalents of
>> those json functions that didn't make it into the 9.4 jsonb work due to
>> pressure of time, i.e. the json generating functions and the aggregates.
>> That work has been started and with luck will hit the next commitfest.
> Does that include the concatenate operator? That's probably the single
> biggest thing we missed.
>

No, the only thing I am doing to provide jsonb equivaents of existing
json functions where they don't currently exist. There is no existing
json concatenation operator.

I think there are quite a few operations that we could very usefully
provide. Given the buzz that our json work has been generating, that
would probably be a very productive area to work on.

cheers

andrew

On 07/29/2014 10:43 PM, Bruce Momjian wrote:
>> > * When a user tries to run "psql -f some_custom_format_backup", detect
>> > this and emit a useful error message. Ditto stdin.
>
> Uh, good idea, but can we really do that in psql?

Where stdin is a file, or an explicit -f is given, then yes.

Just look for PGDMP as the first five bytes and complain.

To do it more generally we'd need to look at each statement and see if
it seems to begin with PGDMP. I'm not sure that's worth it; handling the
simple cases of

psql -f mydb.dump

and

psql < mydb.dump

would be quite sufficient.

I'm not 100% sure if we can easily differentiate the second case from
"pg_dump | psql"; I know there's isatty(...) to test if stdin is a
terminal, but I'm not sure how easy/possible it is to tell if it's a
file not a pipe.

pg_restore already knows to tell you to use psql if it sees an SQL file
as input. Having something similar for pg_dump would be really useful.

--
Craig Ringer http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

On 2014-09-18 17:30:38 +0800, Craig Ringer wrote:
> On 07/29/2014 10:43 PM, Bruce Momjian wrote:
> >> > * When a user tries to run "psql -f some_custom_format_backup", detect
> >> > this and emit a useful error message. Ditto stdin.
> >
> > Uh, good idea, but can we really do that in psql?
>
> Where stdin is a file, or an explicit -f is given, then yes.
>
> Just look for PGDMP as the first five bytes and complain.
>
> To do it more generally we'd need to look at each statement and see if
> it seems to begin with PGDMP. I'm not sure that's worth it; handling the
> simple cases of
>
> psql -f mydb.dump
>
> and
>
> psql < mydb.dump
>
> would be quite sufficient.
>
> I'm not 100% sure if we can easily differentiate the second case from
> "pg_dump | psql"; I know there's isatty(...) to test if stdin is a
> terminal, but I'm not sure how easy/possible it is to tell if it's a
> file not a pipe.

I don't think we need to make any discinction between psql -f mydb.dump,
psql < mydb.dump, and whatever | psql. Just check, when noninteractively
reading the first line in mainloop.c:MainLoop(), whether it starts with
the magic header. That'd also trigger the warning on \i pg_restore_file,
but that's hardly a problem.

> pg_restore already knows to tell you to use psql if it sees an SQL file
> as input. Having something similar for pg_dump would be really useful.

Agreed.

We could additionally write out a hint whenever a directory is fed to
psql -f that psql can't be used to read directory type dumps.

Greetings,

Andres Freund

--
Andres Freund http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

On 09/18/2014 05:58 PM, Andres Freund wrote:
> I don't think we need to make any discinction between psql -f mydb.dump,
> psql < mydb.dump, and whatever | psql. Just check, when noninteractively
> reading the first line in mainloop.c:MainLoop(), whether it starts with
> the magic header. That'd also trigger the warning on \i pg_restore_file,
> but that's hardly a problem.

Done, patch attached.

If psql sees that the first line begins with PGDMP it'll emit:

The input is a PostgreSQL custom-format dump. Use the pg_restore
command-line client to restore this dump to a database.

then discard the rest of the current input source.

>> pg_restore already knows to tell you to use psql if it sees an SQL file
>> as input. Having something similar for pg_dump would be really useful.
>
> Agreed.
>
> We could additionally write out a hint whenever a directory is fed to
> psql -f that psql can't be used to read directory type dumps.

Unlike the confusion between pg_restore and psql for custom file format
dumps I haven't seen people getting this one muddled. Perhaps directory
format dumps are just a bit more niche, or perhaps it's just more
obvious that:

psql:sometump:0: could not read from input file: Is a directory

... means psql is the wrong tool.

Still, separate patch attached. psql will now emit:

psql:blah:0: Input path is a directory. Use pg_restore to restore
directory-format database dumps.

I'm less sure that this is a worthwhile improvement than the check for
PGDMP and custom format dumps though.

--
Craig Ringer http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

Hi,

Regarding Loading Custom Format Dump:
===
When we supply plain sql file to pg_restore, we get following error:
$ ./install/bin/pg_restore a.sql
pg_restore: [archiver] input file does not appear to be a valid archive

So I would expect similar kind of message when we provide non-plain sql
file to psql. Something like:
"input file does not appear to be a valid sql script file
(use pg_restore instead)"

I have added additional details in parenthesis as we correctly identified
it as a custom dump file and user wanted it to restore.

However I do not see any issue with the patch.

Regarding Directory Error:
===
I strongly against the proposal. This patch changing error message to
something like this:
"psql:blah:0: Input path is a directory. Use pg_restore to restore
directory-format database dumps."

So even though I accidentally provide a directory instead of a sql script
file when I have NO intention of restoring a dump, above message looks
weired. Instead current message looks perfectly fine here. i.e.
"could not read from input file: Is a directory"

psql always expect a file and NOT directory. Also it is not necessarily
working on restoring a dump.

Thanks

On Fri, Oct 17, 2014 at 9:41 AM, Craig Ringer <craig(at)2ndquadrant(dot)com> wrote:

> On 09/18/2014 05:58 PM, Andres Freund wrote:
> > I don't think we need to make any discinction between psql -f mydb.dump,
> > psql < mydb.dump, and whatever | psql. Just check, when noninteractively
> > reading the first line in mainloop.c:MainLoop(), whether it starts with
> > the magic header. That'd also trigger the warning on \i pg_restore_file,
> > but that's hardly a problem.
>
> Done, patch attached.
>
> If psql sees that the first line begins with PGDMP it'll emit:
>
> The input is a PostgreSQL custom-format dump. Use the pg_restore
> command-line client to restore this dump to a database.
>
> then discard the rest of the current input source.
>
> >> pg_restore already knows to tell you to use psql if it sees an SQL file
> >> as input. Having something similar for pg_dump would be really useful.
> >
> > Agreed.
> >
> > We could additionally write out a hint whenever a directory is fed to
> > psql -f that psql can't be used to read directory type dumps.
>
> Unlike the confusion between pg_restore and psql for custom file format
> dumps I haven't seen people getting this one muddled. Perhaps directory
> format dumps are just a bit more niche, or perhaps it's just more
> obvious that:
>
> psql:sometump:0: could not read from input file: Is a directory
>
> ... means psql is the wrong tool.
>
> Still, separate patch attached. psql will now emit:
>
> psql:blah:0: Input path is a directory. Use pg_restore to restore
> directory-format database dumps.
>
> I'm less sure that this is a worthwhile improvement than the check for
> PGDMP and custom format dumps though.
>
> --
> Craig Ringer http://www.2ndQuadrant.com/
> PostgreSQL Development, 24x7 Support, Training & Services
>
>
> --
> Sent via pgsql-hackers mailing list (pgsql-hackers(at)postgresql(dot)org)
> To make changes to your subscription:
> http://www.postgresql.org/mailpref/pgsql-hackers
>
>

--
Jeevan B Chalke
Principal Software Engineer, Product Development
EnterpriseDB Corporation
The Enterprise PostgreSQL Company

Phone: +91 20 30589500

Website: www.enterprisedb.com
EnterpriseDB Blog: http://blogs.enterprisedb.com/
Follow us on Twitter: http://www.twitter.com/enterprisedb

This e-mail message (and any attachment) is intended for the use of the
individual or entity to whom it is addressed. This message contains
information from EnterpriseDB Corporation that may be privileged,
confidential, or exempt from disclosure under applicable law. If you are
not the intended recipient or authorized to receive this for the intended
recipient, any use, dissemination, distribution, retention, archiving, or
copying of this communication is strictly prohibited. If you have received
this e-mail in error, please notify the sender immediately by reply e-mail
and delete this message.

Jeevan Chalke wrote:

> Regarding Directory Error:
> ===
> I strongly against the proposal. This patch changing error message to
> something like this:
> "psql:blah:0: Input path is a directory. Use pg_restore to restore
> directory-format database dumps."
>
> So even though I accidentally provide a directory instead of a sql script
> file when I have NO intention of restoring a dump, above message looks
> weired. Instead current message looks perfectly fine here. i.e.
> "could not read from input file: Is a directory"
>
> psql always expect a file and NOT directory. Also it is not necessarily
> working on restoring a dump.

Yeah, this patch is a lot more debatable than the other one. I have
pushed the first one without changing the error message.

--
Álvaro Herrera http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

On 2014-10-24 07:18:55 -0300, Alvaro Herrera wrote:
> Jeevan Chalke wrote:
>
> > Regarding Directory Error:
> > ===
> > I strongly against the proposal. This patch changing error message to
> > something like this:
> > "psql:blah:0: Input path is a directory. Use pg_restore to restore
> > directory-format database dumps."
> >
> > So even though I accidentally provide a directory instead of a sql script
> > file when I have NO intention of restoring a dump, above message looks
> > weired. Instead current message looks perfectly fine here. i.e.
> > "could not read from input file: Is a directory"
> >
> > psql always expect a file and NOT directory. Also it is not necessarily
> > working on restoring a dump.
>
> Yeah, this patch is a lot more debatable than the other one. I have
> pushed the first one without changing the error message.

We could just test for toc.dat and then emit the warning...

Greetings,

Andres Freund

--
Andres Freund http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

On Fri, Oct 24, 2014 at 7:23 PM, Andres Freund <andres(at)2ndquadrant(dot)com> wrote:
> On 2014-10-24 07:18:55 -0300, Alvaro Herrera wrote:
>> Yeah, this patch is a lot more debatable than the other one. I have
>> pushed the first one without changing the error message.
>
> We could just test for toc.dat and then emit the warning...
One patch has been committed, and the second is debatable. Hence
marking this entry as returned with feedback in the CF app.
--
Michael