New developer TODO suggestions

Started by Craig Ringerover 11 years ago12 messages
#1Craig Ringer
craig@2ndquadrant.com

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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

#2Bruce Momjian
bruce@momjian.us
In reply to: Craig Ringer (#1)
Re: New developer TODO suggestions

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@momjian.us> http://momjian.us
EnterpriseDB http://enterprisedb.com

+ Everyone has their own god. +

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

#3Andrew Dunstan
andrew@dunslane.net
In reply to: Bruce Momjian (#2)
Re: New developer TODO suggestions

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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

#4Peter Geoghegan
pg@heroku.com
In reply to: Andrew Dunstan (#3)
Re: New developer TODO suggestions

On Wed, Jul 30, 2014 at 2:37 PM, Andrew Dunstan <andrew@dunslane.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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

#5Andrew Dunstan
andrew@dunslane.net
In reply to: Peter Geoghegan (#4)
Re: New developer TODO suggestions

On 07/30/2014 06:11 PM, Peter Geoghegan wrote:

On Wed, Jul 30, 2014 at 2:37 PM, Andrew Dunstan <andrew@dunslane.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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

#6Craig Ringer
craig@2ndquadrant.com
In reply to: Bruce Momjian (#2)
Re: New developer TODO suggestions

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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

#7Andres Freund
andres@2ndquadrant.com
In reply to: Craig Ringer (#6)
Re: New developer TODO suggestions

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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

#8Craig Ringer
craig@2ndquadrant.com
In reply to: Andres Freund (#7)
2 attachment(s)
Re: detect custom-format dumps in psql and emit a useful error

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

Attachments:

0002-If-the-input-path-to-psql-is-a-directory-mention-pg_.patchtext/x-patch; name=0002-If-the-input-path-to-psql-is-a-directory-mention-pg_.patchDownload
>From e3a6633ec2782264f3a87fbe3be079f94d89b911 Mon Sep 17 00:00:00 2001
From: Craig Ringer <craig@2ndquadrant.com>
Date: Fri, 17 Oct 2014 12:07:37 +0800
Subject: [PATCH 2/2] If the input path to psql is a directory, mention
 pg_restore in the error

This should help users who try to use psql to restore a directory-format
dump from pg_dump.
---
 src/bin/psql/input.c | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/src/bin/psql/input.c b/src/bin/psql/input.c
index 6416ab9..e332318 100644
--- a/src/bin/psql/input.c
+++ b/src/bin/psql/input.c
@@ -191,8 +191,15 @@ gets_fromFile(FILE *source)
 		{
 			if (ferror(source))
 			{
-				psql_error("could not read from input file: %s\n",
-						   strerror(errno));
+				/*
+				 * Could the user be trying to restore a directory
+				 * format dump?
+				 */
+				if (errno == EISDIR)
+					psql_error("Input path is a directory. Use pg_restore to restore directory-format database dumps.\n");
+				else
+					psql_error("could not read from input file: %s\n",
+							   strerror(errno));
 				return NULL;
 			}
 			break;
-- 
1.9.3

0001-Emit-an-error-when-psql-is-used-to-load-a-custom-for.patchtext/x-patch; name=0001-Emit-an-error-when-psql-is-used-to-load-a-custom-for.patchDownload
>From 5330eea78029f9cb689fd3c53722cb02217f47df Mon Sep 17 00:00:00 2001
From: Craig Ringer <craig@2ndquadrant.com>
Date: Fri, 17 Oct 2014 11:54:29 +0800
Subject: [PATCH 1/2] Emit an error when psql is used to load a custom-format
 dump file

Users tend to get confused between psql and pg_restore, and will
use psql to try to restore a dump from pg_dump -Fc, or use pg_restore
to try to restore an SQL format dump.

pg_restore complains if it sees an SQL format dump, but psql doesn't
complain if it sees a custom-format dump.

Fix that by emitting an error if you try to run a custom format dump:

  The input is a PostgreSQL custom-format dump. Use the pg_restore
  command-line client to restore this dump to a database.
---
 src/bin/psql/mainloop.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/src/bin/psql/mainloop.c b/src/bin/psql/mainloop.c
index 98211dc..1e057a6 100644
--- a/src/bin/psql/mainloop.c
+++ b/src/bin/psql/mainloop.c
@@ -175,6 +175,17 @@ MainLoop(FILE *source)
 		if (pset.lineno == 1 && pset.encoding == PG_UTF8 && strncmp(line, "\xef\xbb\xbf", 3) == 0)
 			memmove(line, line + 3, strlen(line + 3) + 1);
 
+		/* Detect attempts to run custom-format dumps as SQL scripts */
+		if (pset.lineno == 1 && !pset.cur_cmd_interactive && strncmp(line, "PGDMP", 5) == 0)
+		{
+			free(line);
+			puts(_("The input is a PostgreSQL custom-format dump. Use the pg_restore \n"
+				   "command-line client to restore this dump to a database.\n"));
+			fflush(stdout);
+			successResult = EXIT_FAILURE;
+			break;
+		}
+
 		/* nothing left on line? then ignore */
 		if (line[0] == '\0' && !psql_scan_in_quote(scan_state))
 		{
-- 
1.9.3

#9Jeevan Chalke
jeevan.chalke@enterprisedb.com
In reply to: Craig Ringer (#8)
Re: detect custom-format dumps in psql and emit a useful error

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@2ndquadrant.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@postgresql.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.

#10Alvaro Herrera
alvherre@2ndquadrant.com
In reply to: Jeevan Chalke (#9)
Re: detect custom-format dumps in psql and emit a useful error

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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

#11Andres Freund
andres@2ndquadrant.com
In reply to: Alvaro Herrera (#10)
Re: detect custom-format dumps in psql and emit a useful error

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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

#12Michael Paquier
michael.paquier@gmail.com
In reply to: Andres Freund (#11)
Re: detect custom-format dumps in psql and emit a useful error

On Fri, Oct 24, 2014 at 7:23 PM, Andres Freund <andres@2ndquadrant.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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers