Re: [HACKERS] [PATCH] COPY vs \copy HINT

[Tom Lane]

Craig Ringer  writes:
> On 7 September 2016 at 04:19, Christoph Berg  wrote:
>> I like your new version, it's crisp and transports the right message.

> OK, updated with Tom's tweaked version of Christoph's wording per
> discussion. Thanks all.

Pushed with minor stylistic adjustment (narrowing the scope of
save_errno).

regards, tom lane


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

Re: [HACKERS] [PATCH] COPY vs \copy HINT

[Craig Ringer]

On 7 September 2016 at 04:19, Christoph Berg  wrote:

> I like your new version, it's crisp and transports the right message.

OK, updated with Tom's tweaked version of Christoph's wording per
discussion. Thanks all.

-- 
 Craig Ringer   http://www.2ndQuadrant.com/
 PostgreSQL Development, 24x7 Support, Training & Services
From 17bdc19e15f21ce84eee76dee9f4f86e868a8430 Mon Sep 17 00:00:00 2001
From: Craig Ringer 
Date: Fri, 12 Aug 2016 15:42:12 +0800
Subject: [PATCH] Emit a HINT when COPY can't find a file

Users often get confused between COPY and \copy and try to use client-side
paths with COPY. The server of course cannot find the file.

Emit a HINT in the most common cases to help users out.

Craig Ringer, Tom Lane and Christoph Berg
---
 src/backend/commands/copy.c | 20 +---
 1 file changed, 17 insertions(+), 3 deletions(-)

diff --git a/src/backend/commands/copy.c b/src/backend/commands/copy.c
index 5947e72..e7118df 100644
--- a/src/backend/commands/copy.c
+++ b/src/backend/commands/copy.c
@@ -1751,6 +1751,7 @@ BeginCopyTo(Relation rel,
 		{
 			mode_t		oumask; /* Pre-existing umask value */
 			struct stat st;
+			int			save_errno;
 
 			/*
 			 * Prevent write to relative path ... too easy to shoot oneself in
@@ -1759,16 +1760,21 @@ BeginCopyTo(Relation rel,
 			if (!is_absolute_path(filename))
 ereport(ERROR,
 		(errcode(ERRCODE_INVALID_NAME),
-	  errmsg("relative path not allowed for COPY to file")));
+		 errmsg("relative path not allowed for COPY to file")));
 
 			oumask = umask(S_IWGRP | S_IWOTH);
 			cstate->copy_file = AllocateFile(cstate->filename, PG_BINARY_W);
+			save_errno = errno;
 			umask(oumask);
+
 			if (cstate->copy_file == NULL)
 ereport(ERROR,
 		(errcode_for_file_access(),
 		 errmsg("could not open file \"%s\" for writing: %m",
-cstate->filename)));
+cstate->filename),
+		 ((save_errno == ENOENT || save_errno == EACCES) ?
+		  errhint("COPY TO instructs the PostgreSQL server process to write a file. "
+  "You may want a client-side facility such as psql's \\copy.") : 0)));
 
 			if (fstat(fileno(cstate->copy_file), ))
 ereport(ERROR,
@@ -2786,13 +2792,21 @@ BeginCopyFrom(Relation rel,
 		else
 		{
 			struct stat st;
+			int			save_errno;
 
 			cstate->copy_file = AllocateFile(cstate->filename, PG_BINARY_R);
+
+			/* in case something in ereport changes errno: */
+			save_errno = errno;
+
 			if (cstate->copy_file == NULL)
 ereport(ERROR,
 		(errcode_for_file_access(),
 		 errmsg("could not open file \"%s\" for reading: %m",
-cstate->filename)));
+cstate->filename),
+		 ((save_errno == ENOENT || save_errno == EACCES) ?
+		  errhint("COPY FROM instructs the PostgreSQL server process to read a file. "
+  "You may want a client-side facility such as psql's \\copy.") : 0)));
 
 			if (fstat(fileno(cstate->copy_file), ))
 ereport(ERROR,
-- 
2.5.5


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

Re: [HACKERS] [PATCH] COPY vs \copy HINT

[Christoph Berg]

Re: Tom Lane 2016-09-06 <17637.1473192...@sss.pgh.pa.us>
> Christoph's idea isn't bad.  We could tweak it to:
> 
>   COPY TO instructs the PostgreSQL server process to write a file.
> 
>   COPY FROM instructs the PostgreSQL server process to read a file.
> 
> This seems to cover both the wrong-machine and wrong-process-identity
> cases, and as a bonus it might help if you've had a brain fade about
> which COPY direction is write vs. read.
> 
> (I think we're all in agreement that the second sentence of the hint
> is fine, so leaving that out of it.)
> 
> Comments?

I like your new version, it's crisp and transports the right message.

Christoph


signature.asc
Description: PGP signature

Re: [HACKERS] [PATCH] COPY vs \copy HINT

[Tom Lane]

Craig Ringer  writes:
> Tom, any preference here?
> I'm probably inclined to go for your original wording and accept that
> it's just too hard to hint at the client/server process split in a
> single short message.

I think my original wording is pretty hopeless for the single-machine
case: "COPY copies to a file on the PostgreSQL server, not on the client".
If the server and client are the same machine, that's content-free.

Christoph's idea isn't bad.  We could tweak it to:

COPY TO instructs the PostgreSQL server process to write a file.

COPY FROM instructs the PostgreSQL server process to read a file.

This seems to cover both the wrong-machine and wrong-process-identity
cases, and as a bonus it might help if you've had a brain fade about
which COPY direction is write vs. read.

(I think we're all in agreement that the second sentence of the hint
is fine, so leaving that out of it.)

Comments?

regards, tom lane


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

Re: [HACKERS] [PATCH] COPY vs \copy HINT

[Craig Ringer]

On 5 September 2016 at 16:32, Christoph Berg  wrote:

> The part about the server permissions might be useful to hint at.

> What about
>
>  "COPY TO instructs the PostgreSQL server to write to a file on the 
> server. "
>  "You may want a client-side facility such as psql's \\copy."
>
>  "COPY FROM instructs the PostgreSQL server to read from a file on the 
> server. "
>  "You may want a client-side facility such as psql's \\copy."
>
> This stresses more explicitly that the file is opened by the server
> process. (I'm not particularly happy that it says "server" in there
> twice, but couldn't think of anything else.)

Tom, any preference here?

I'm probably inclined to go for your original wording and accept that
it's just too hard to hint at the client/server process split in a
single short message.

-- 
 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

Re: [HACKERS] [PATCH] COPY vs \copy HINT

[Christoph Berg]

Re: Craig Ringer 2016-09-05 

> To cover the same-host case we could try something like:
> 
>COPY runs on the PostgreSQL server, using the PostgreSQL server's
> directories and permissions, it doesn't run on the client.
> 
> ... but I think that's actually less helpful for the users who'll need this.

The part about the server permissions might be useful to hint at.

> > I don't suppose there's any easy way for COPY to distinguish local
> > from remote connections
> 
> Not that I see, since "local" can be unix socket or tcp to localhost.
> Not cleanly anyway.
> 
> I don't think it matters. Many hosts have enough paths in common that
> in practice the hint on EACCES will be useful anyway. It'd be nice to
> work in something about running with the permissions of the PostgreSQL
> server, but I don't see a way to do that without making it all more
> complex.
> 
> I don't think it's worth tons of time anyway. This will be better than
> what we have, lets do it.

It's probably not helpful trying to distinguish local and remote
connections, the set of possible problems is diverse and this is just
one of them.

> I'm fairly happy with the wording you came up with:
> 
> "COPY copies to a file on the PostgreSQL server, not on the client. "
> "You may want a client-side facility such as psql's \\copy."

What about

 "COPY TO instructs the PostgreSQL server to write to a file on the server. 
"
 "You may want a client-side facility such as psql's \\copy."

 "COPY FROM instructs the PostgreSQL server to read from a file on the 
server. "
 "You may want a client-side facility such as psql's \\copy."

This stresses more explicitly that the file is opened by the server
process. (I'm not particularly happy that it says "server" in there
twice, but couldn't think of anything else.)

Christoph


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

Re: [HACKERS] [PATCH] COPY vs \copy HINT

[Craig Ringer]

On 5 September 2016 at 09:05, Craig Ringer  wrote:

>I've attached an update that does so and
> warns on EACCES too.

... this time, with required parens.

-- 
 Craig Ringer   http://www.2ndQuadrant.com/
 PostgreSQL Development, 24x7 Support, Training & Services
From d4455da00411daae846eadbd7f7d9bae2fb61b49 Mon Sep 17 00:00:00 2001
From: Craig Ringer 
Date: Fri, 12 Aug 2016 15:42:12 +0800
Subject: [PATCH] Emit a HINT when COPY can't find a file

Users often get confused between COPY and \copy and try to use client-side
paths with COPY. The server of course cannot find the file.

Emit a HINT in the most common cases to help users out.

Craig Ringer, review by Tom Lane and Christoph Berg
---
 src/backend/commands/copy.c | 20 +---
 1 file changed, 17 insertions(+), 3 deletions(-)

diff --git a/src/backend/commands/copy.c b/src/backend/commands/copy.c
index 5947e72..8d5cb4f 100644
--- a/src/backend/commands/copy.c
+++ b/src/backend/commands/copy.c
@@ -1751,6 +1751,7 @@ BeginCopyTo(Relation rel,
 		{
 			mode_t		oumask; /* Pre-existing umask value */
 			struct stat st;
+			int			save_errno;
 
 			/*
 			 * Prevent write to relative path ... too easy to shoot oneself in
@@ -1759,16 +1760,21 @@ BeginCopyTo(Relation rel,
 			if (!is_absolute_path(filename))
 ereport(ERROR,
 		(errcode(ERRCODE_INVALID_NAME),
-	  errmsg("relative path not allowed for COPY to file")));
+		 errmsg("relative path not allowed for COPY to file")));
 
 			oumask = umask(S_IWGRP | S_IWOTH);
 			cstate->copy_file = AllocateFile(cstate->filename, PG_BINARY_W);
+			save_errno = errno;
 			umask(oumask);
+
 			if (cstate->copy_file == NULL)
 ereport(ERROR,
 		(errcode_for_file_access(),
 		 errmsg("could not open file \"%s\" for writing: %m",
-cstate->filename)));
+cstate->filename),
+		 ((save_errno == ENOENT || save_errno == EACCES) ?
+		  errhint("COPY copies to a file on the PostgreSQL server, not on the client. "
+  "You may want a client-side facility such as psql's \\copy.") : 0)));
 
 			if (fstat(fileno(cstate->copy_file), ))
 ereport(ERROR,
@@ -2786,13 +2792,21 @@ BeginCopyFrom(Relation rel,
 		else
 		{
 			struct stat st;
+			int			save_errno;
 
 			cstate->copy_file = AllocateFile(cstate->filename, PG_BINARY_R);
+
+			/* in case something in ereport changes errno: */
+			save_errno = errno;
+
 			if (cstate->copy_file == NULL)
 ereport(ERROR,
 		(errcode_for_file_access(),
 		 errmsg("could not open file \"%s\" for reading: %m",
-cstate->filename)));
+cstate->filename),
+		 ((save_errno == ENOENT || save_errno == EACCES) ?
+		  errhint("COPY copies from a file on the PostgreSQL server, not on the client. "
+  "You may want a client-side facility such as psql's \\copy.") : 0)));
 
 			if (fstat(fileno(cstate->copy_file), ))
 ereport(ERROR,
-- 
2.5.5


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

Re: [HACKERS] [PATCH] COPY vs \copy HINT

[Craig Ringer]

On 4 September 2016 at 23:33, Tom Lane  wrote:

> So my consciousness was raised just now by an example of exactly this
> scenario over in pgsql-novice.  What I forgot was that the client may
> in fact be on the same machine as the server, in which case EACCES
> is pretty much exactly what you'd expect.

Yep. Also in cases with common paths, like /root or whatever.

> So we probably do want to
> hint for that case, but the hint wording I previously suggested no
> longer seems like le mot juste ... it needs to cover the idea that
> the client and server are different processes on the same machine.

Yeah, may be. *We* know that in this case "client" and "server" still
applies since client and server can be on the same host, but the
people who needs this hint may not understand that. Though there's
only so much we can do or should try to do in a HINT.

I think the most important bit is pointing them at \copy, so it's still useful.

To cover the same-host case we could try something like:

   COPY runs on the PostgreSQL server, using the PostgreSQL server's
directories and permissions, it doesn't run on the client.

... but I think that's actually less helpful for the users who'll need this.

We could say "COPY runs as the PostgreSQL server" but that's the kind
of hint that mostly helps people who already understand it and don't
actually the hint.

(BTW, whoever came up with "EACCES" needs to go spend time with the
creat() system call somewhere dark and smelly).

> I don't suppose there's any easy way for COPY to distinguish local
> from remote connections

Not that I see, since "local" can be unix socket or tcp to localhost.
Not cleanly anyway.

I don't think it matters. Many hosts have enough paths in common that
in practice the hint on EACCES will be useful anyway. It'd be nice to
work in something about running with the permissions of the PostgreSQL
server, but I don't see a way to do that without making it all more
complex.

I don't think it's worth tons of time anyway. This will be better than
what we have, lets do it.


I'm fairly happy with the wording you came up with:

"COPY copies to a file on the PostgreSQL server, not on the client. "
"You may want a client-side facility such as psql's \\copy."

and am inclined to suggest going ahead with the existing wording. I
agree that removing the part for "relative path not allowed for COPY
to file" is reasonable, so I've attached an update that does so and
warns on EACCES too.

-- 
 Craig Ringer   http://www.2ndQuadrant.com/
 PostgreSQL Development, 24x7 Support, Training & Services
From a30266cdbf1febd033ad2ba127341e1da4677dae Mon Sep 17 00:00:00 2001
From: Craig Ringer 
Date: Fri, 12 Aug 2016 15:42:12 +0800
Subject: [PATCH] Emit a HINT when COPY can't find a file

Users often get confused between COPY and \copy and try to use client-side
paths with COPY. The server of course cannot find the file.

Emit a HINT in the most common cases to help users out.

Craig Ringer, review by Tom Lane and Christoph Berg
---
 src/backend/commands/copy.c | 20 +---
 1 file changed, 17 insertions(+), 3 deletions(-)

diff --git a/src/backend/commands/copy.c b/src/backend/commands/copy.c
index 5947e72..341c058 100644
--- a/src/backend/commands/copy.c
+++ b/src/backend/commands/copy.c
@@ -1751,6 +1751,7 @@ BeginCopyTo(Relation rel,
 		{
 			mode_t		oumask; /* Pre-existing umask value */
 			struct stat st;
+			int			save_errno;
 
 			/*
 			 * Prevent write to relative path ... too easy to shoot oneself in
@@ -1759,16 +1760,21 @@ BeginCopyTo(Relation rel,
 			if (!is_absolute_path(filename))
 ereport(ERROR,
 		(errcode(ERRCODE_INVALID_NAME),
-	  errmsg("relative path not allowed for COPY to file")));
+		 errmsg("relative path not allowed for COPY to file")));
 
 			oumask = umask(S_IWGRP | S_IWOTH);
 			cstate->copy_file = AllocateFile(cstate->filename, PG_BINARY_W);
+			save_errno = errno;
 			umask(oumask);
+
 			if (cstate->copy_file == NULL)
 ereport(ERROR,
 		(errcode_for_file_access(),
 		 errmsg("could not open file \"%s\" for writing: %m",
-cstate->filename)));
+cstate->filename),
+		 (save_errno == ENOENT || save_errno == EACCES ?
+		  errhint("COPY copies to a file on the PostgreSQL server, not on the client. "
+  "You may want a client-side facility such as psql's \\copy.") : 0)));
 
 			if (fstat(fileno(cstate->copy_file), ))
 ereport(ERROR,
@@ -2786,13 +2792,21 @@ BeginCopyFrom(Relation rel,
 		else
 		{
 			struct stat st;
+			int			save_errno;
 
 			cstate->copy_file = AllocateFile(cstate->filename, PG_BINARY_R);
+
+			/* in case something in ereport changes errno: */
+			save_errno = errno;
+
 			if (cstate->copy_file == NULL)
 ereport(ERROR,
 		(errcode_for_file_access(),
 		 errmsg("could not open file \"%s\" for reading: %m",
-cstate->filename)));
+cstate->filename),
+		 (save_errno 

Re: [HACKERS] [PATCH] COPY vs \copy HINT

[Tom Lane]

I wrote:
> Craig Ringer  writes:
>> I thought about that but figured it didn't really matter too much,
>> when thinking about examples like
>> # COPY batch_demo FROM '/root/secret.csv' WITH (FORMAT CSV);
>> ERROR:  could not open file "/root/secret.csv" for reading: Permission denied
>> or whatever, where the user doesn't understand why they can't read the
>> file given that their local client has permission to do so.

> Meh.  Hinting in this case could be helpful only if the user in fact had
> identical directory structures on client and server and a data file in the
> right place on both.

So my consciousness was raised just now by an example of exactly this
scenario over in pgsql-novice.  What I forgot was that the client may
in fact be on the same machine as the server, in which case EACCES
is pretty much exactly what you'd expect.  So we probably do want to
hint for that case, but the hint wording I previously suggested no
longer seems like le mot juste ... it needs to cover the idea that
the client and server are different processes on the same machine.

I don't suppose there's any easy way for COPY to distinguish local
from remote connections --- if it could, that might influence both
the errnos to hint for and the phrasing of the hint.

regards, tom lane


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

Re: [HACKERS] [PATCH] COPY vs \copy HINT

[Tom Lane]

Craig Ringer  writes:
> On 2 September 2016 at 04:28, Tom Lane  wrote:
>> 1. I don't really think the HINT is appropriate for the not-absolute-path
>> case.

> Why? If the user runs
> # COPY sometable FROM 'localfile.csv' WITH (FORMAT CSV);
> ERROR:  could not open file "localfile.csv" for reading: No such file
> or directory
> they're going to be just as confused by this error as by:
> # COPY batch_demo FROM '/tmp/localfile.csv' WITH (FORMAT CSV);
> ERROR:  could not open file "/tmp/localfile.csv" for reading: No such
> file or directory
> so I don't really see the rationale for this change.

I think you misinterpreted what I was complaining about; it's not the
FROM case but the TO case, specifically this addition:

 if (!is_absolute_path(filename))
 ereport(ERROR,
 (errcode(ERRCODE_INVALID_NAME),
-  errmsg("relative path not allowed for COPY to file")));
+ errmsg("relative path not allowed for COPY to file"),
+ errhint("COPY copies to a file on the PostgreSQL 
server, not on the client. "
+ "You may want a client-side facility such as 
psql's \\copy.")));

The reason I'm not happy about that is that the hint is completely
unrelated to the error condition, and I think that such a hint is likely
to be more confusing than helpful.  What's likely to happen if we leave
it alone is that someone who's confused about client vs. server will do

=# COPY sometable TO 'localfile.csv' WITH (FORMAT CSV);
ERROR:  relative path not allowed for COPY to file

[ user mutters something about hopeless pedants and writes an absolute
path: ]

=# COPY sometable TO '/home/joe/localfile.csv' WITH (FORMAT CSV);
ERROR:  could not open file "/home/joe/localfile.csv" for writing: No such file 
or directory

At this point, it's appropriate to give the HINT, but I think doing it
for the first case is just confusing.  There's an opportunity cost to
hints like this, which is that if they are in fact wildly unrelated
to the real problem, they are more likely to send the user down a blind
alley than lead his thoughts to the right answer.  What you want to do
is essentially cutting one step out of the process by jumping to the
conclusion that if the user gave a relative path, it's because he's
confused about client vs. server, and I don't see how that follows.

>> 2. I don't think it's appropriate for all possible cases of AllocateFile
>> failure either, eg surely not for EACCES or similar situations where we
>> did find a file.  Maybe print it only for ENOENT?

> I thought about that but figured it didn't really matter too much,
> when thinking about examples like
> # COPY batch_demo FROM '/root/secret.csv' WITH (FORMAT CSV);
> ERROR:  could not open file "/root/secret.csv" for reading: Permission denied
> or whatever, where the user doesn't understand why they can't read the
> file given that their local client has permission to do so.

Meh.  Hinting in this case could be helpful only if the user in fact had
identical directory structures on client and server and a data file in the
right place on both.  That doesn't seem terribly likely, and again I argue
that the downsides of giving an irrelevant hint outweigh the occasional
benefit when it happens to be right.  I mean, taking this position just
one step further, we should annotate every COPY data error with this same
hint, arguing that maybe the user has the right data on one machine and
slightly wrong data on the other machine.  Once in a blue moon that would
indeed be the answer, but most of the time it would be wrong, and users
would soon start getting annoyed with the hint --- maybe to the point
where they automatically ignore that hint even when it's right.

In short, I like hints as long as they're carefully targeted, but if
they're not they are likely to be seen as a net negative by users.
I think giving this hint for the ENOENT case is probably fine, but
I'm much less convinced about other cases.

One additional case that I could believe is useful is whatever error
code Windows gives when you specify a drive letter that's not in use.
(Maybe that's ENOENT but I bet not.)

regards, tom lane


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

Re: [HACKERS] [PATCH] COPY vs \copy HINT

[Craig Ringer]

On 2 September 2016 at 17:05, Christoph Berg  wrote:
> Re: Craig Ringer 2016-09-02 
> 
>> I thought about that but figured it didn't really matter too much,
>> when thinking about examples like
>>
>> # COPY batch_demo FROM '/root/secret.csv' WITH (FORMAT CSV);
>> ERROR:  could not open file "/root/secret.csv" for reading: Permission denied
>>
>> or whatever, where the user doesn't understand why they can't read the
>> file given that their local client has permission to do so.
>>
>> I don't feel strongly about this and think that the error on ENOENT is
>> by far the most important, so I'll adjust it per your recommendation.
>
> Couldn't you just add EACCESS to the check as well?

Yeah, probably. Those are really the only two failures I think it's
worth reporting for.


-- 
 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

Re: [HACKERS] [PATCH] COPY vs \copy HINT

[Christoph Berg]

Re: Craig Ringer 2016-09-02 

> I thought about that but figured it didn't really matter too much,
> when thinking about examples like
> 
> # COPY batch_demo FROM '/root/secret.csv' WITH (FORMAT CSV);
> ERROR:  could not open file "/root/secret.csv" for reading: Permission denied
> 
> or whatever, where the user doesn't understand why they can't read the
> file given that their local client has permission to do so.
> 
> I don't feel strongly about this and think that the error on ENOENT is
> by far the most important, so I'll adjust it per your recommendation.

Couldn't you just add EACCESS to the check as well?

> > 3. As for the wording, maybe you could do it like this:
> >
> > HINT:  COPY copies to[from] a file on the PostgreSQL server, not on the
> > client.  You may want a client-side facility such as psql's \copy.
> >
> > That avoids trying to invent a name for other implementations.
> 
> I like that wording a lot more, thanks. Adopted.

Same here, thanks!

Christoph


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

Re: [HACKERS] [PATCH] COPY vs \copy HINT

[Craig Ringer]

On 2 September 2016 at 04:28, Tom Lane  wrote:
> Craig Ringer  writes:
>> On 12 August 2016 at 16:34, Christoph Berg  wrote:
>>> Also, I vaguely get what you wanted to say with "a driver ...
>>> wrapper", but it's pretty nonsensical if one doesn't know about the
>>> protocol details. I don't have a better suggestion now, but I think it
>>> needs rephrasing.
>
>> I don't like it either, but didn't come up with anything better. The
>> problem is that every driver calls it something different.
>
> A few thoughts on this patch:

Thanks for the review. I'll leave the first change pending your
comments, the others are made, though I'm not completely sure we
should restrict it to ENOENT.

> 1. I don't really think the HINT is appropriate for the not-absolute-path
> case.

Why? If the user runs

# COPY sometable FROM 'localfile.csv' WITH (FORMAT CSV);
ERROR:  could not open file "localfile.csv" for reading: No such file
or directory

they're going to be just as confused by this error as by:

# COPY batch_demo FROM '/tmp/localfile.csv' WITH (FORMAT CSV);
ERROR:  could not open file "/tmp/localfile.csv" for reading: No such
file or directory

so I don't really see the rationale for this change.

> 2. I don't think it's appropriate for all possible cases of AllocateFile
> failure either, eg surely not for EACCES or similar situations where we
> did find a file.  Maybe print it only for ENOENT?  (See for example
> report_newlocale_failure() for technique.)

I thought about that but figured it didn't really matter too much,
when thinking about examples like

# COPY batch_demo FROM '/root/secret.csv' WITH (FORMAT CSV);
ERROR:  could not open file "/root/secret.csv" for reading: Permission denied

or whatever, where the user doesn't understand why they can't read the
file given that their local client has permission to do so.

I don't feel strongly about this and think that the error on ENOENT is
by far the most important, so I'll adjust it per your recommendation.

> 3. As for the wording, maybe you could do it like this:
>
> HINT:  COPY copies to[from] a file on the PostgreSQL server, not on the
> client.  You may want a client-side facility such as psql's \copy.
>
> That avoids trying to invent a name for other implementations.

I like that wording a lot more, thanks. Adopted.

-- 
 Craig Ringer   http://www.2ndQuadrant.com/
 PostgreSQL Development, 24x7 Support, Training & Services
From aa4f1fc810c8617d78dec75adad9951faf929909 Mon Sep 17 00:00:00 2001
From: Craig Ringer 
Date: Fri, 12 Aug 2016 15:42:12 +0800
Subject: [PATCH] Emit a HINT when COPY can't find a file

Users often get confused between COPY and \copy and try to use client-side
paths with COPY. The server of course cannot find the file.

Emit a HINT in the most common cases to help users out.

Craig Ringer, review by Tom Lane and Christoph Berg
---
 src/backend/commands/copy.c | 22 +++---
 1 file changed, 19 insertions(+), 3 deletions(-)

diff --git a/src/backend/commands/copy.c b/src/backend/commands/copy.c
index f45b330..fe44bd9 100644
--- a/src/backend/commands/copy.c
+++ b/src/backend/commands/copy.c
@@ -1753,6 +1753,7 @@ BeginCopyTo(Relation rel,
 		{
 			mode_t		oumask; /* Pre-existing umask value */
 			struct stat st;
+			int			save_errno;
 
 			/*
 			 * Prevent write to relative path ... too easy to shoot oneself in
@@ -1761,16 +1762,23 @@ BeginCopyTo(Relation rel,
 			if (!is_absolute_path(filename))
 ereport(ERROR,
 		(errcode(ERRCODE_INVALID_NAME),
-	  errmsg("relative path not allowed for COPY to file")));
+		 errmsg("relative path not allowed for COPY to file"),
+		 errhint("COPY copies to a file on the PostgreSQL server, not on the client. "
+ "You may want a client-side facility such as psql's \\copy.")));
 
 			oumask = umask(S_IWGRP | S_IWOTH);
 			cstate->copy_file = AllocateFile(cstate->filename, PG_BINARY_W);
+			save_errno = errno;
 			umask(oumask);
+
 			if (cstate->copy_file == NULL)
 ereport(ERROR,
 		(errcode_for_file_access(),
 		 errmsg("could not open file \"%s\" for writing: %m",
-cstate->filename)));
+cstate->filename),
+		 (save_errno == ENOENT ?
+		  errhint("COPY copies to a file on the PostgreSQL server, not on the client. "
+  "You may want a client-side facility such as psql's \\copy.") : 0)));
 
 			if (fstat(fileno(cstate->copy_file), ))
 ereport(ERROR,
@@ -2790,13 +2798,21 @@ BeginCopyFrom(Relation rel,
 		else
 		{
 			struct stat st;
+			int			save_errno;
 
 			cstate->copy_file = AllocateFile(cstate->filename, PG_BINARY_R);
+
+			/* in case something in ereport changes errno: */
+			save_errno = errno;
+
 			if (cstate->copy_file == NULL)
 ereport(ERROR,
 		(errcode_for_file_access(),
 		 errmsg("could not open file \"%s\" for reading: %m",
-cstate->filename)));
+cstate->filename),
+		 

Re: [HACKERS] [PATCH] COPY vs \copy HINT

[Tom Lane]

Craig Ringer  writes:
> On 12 August 2016 at 16:34, Christoph Berg  wrote:
>> Also, I vaguely get what you wanted to say with "a driver ...
>> wrapper", but it's pretty nonsensical if one doesn't know about the
>> protocol details. I don't have a better suggestion now, but I think it
>> needs rephrasing.

> I don't like it either, but didn't come up with anything better. The
> problem is that every driver calls it something different.

A few thoughts on this patch:

1. I don't really think the HINT is appropriate for the not-absolute-path
case.

2. I don't think it's appropriate for all possible cases of AllocateFile
failure either, eg surely not for EACCES or similar situations where we
did find a file.  Maybe print it only for ENOENT?  (See for example
report_newlocale_failure() for technique.)

3. As for the wording, maybe you could do it like this:

HINT:  COPY copies to[from] a file on the PostgreSQL server, not on the
client.  You may want a client-side facility such as psql's \copy.

That avoids trying to invent a name for other implementations.

regards, tom lane


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

Re: [HACKERS] [PATCH] COPY vs \copy HINT

[Craig Ringer]

On 12 August 2016 at 16:34, Christoph Berg  wrote:


> > postgres=# COPY x TO '/root/nopermissions';
> > ERROR:  could not open file "/root/nopermissions" for writing: Permission
> > denied
> > HINT:  Paths for COPY are on the PostgreSQL server, not the client. You
> may
> > want psql's \copy or a driver COPY ... FROM STDIN wrapper
>
> TO STDOUT.
>

Ahem, whoops. It's the simple things sometimes...

Attached amended.


> Also, I vaguely get what you wanted to say with "a driver ...
> wrapper", but it's pretty nonsensical if one doesn't know about the
> protocol details. I don't have a better suggestion now, but I think it
> needs rephrasing.


I don't like it either, but didn't come up with anything better. The
problem is that every driver calls it something different.

-- 
 Craig Ringer   http://www.2ndQuadrant.com/
 PostgreSQL Development, 24x7 Support, Training & Services
From e713a8a7c5373a137d417d0001b11e296b841f5a Mon Sep 17 00:00:00 2001
From: Craig Ringer 
Date: Fri, 12 Aug 2016 15:42:12 +0800
Subject: [PATCH] Emit a HINT when COPY can't find a file

Users often get confused between COPY and \copy and try to use client-side
paths with COPY. The server of course cannot find the file.

Emit a HINT in the most common cases to help users out.
---
 src/backend/commands/copy.c | 12 +---
 1 file changed, 9 insertions(+), 3 deletions(-)

diff --git a/src/backend/commands/copy.c b/src/backend/commands/copy.c
index f45b330..c902e8b 100644
--- a/src/backend/commands/copy.c
+++ b/src/backend/commands/copy.c
@@ -1761,7 +1761,9 @@ BeginCopyTo(Relation rel,
 			if (!is_absolute_path(filename))
 ereport(ERROR,
 		(errcode(ERRCODE_INVALID_NAME),
-	  errmsg("relative path not allowed for COPY to file")));
+	  errmsg("relative path not allowed for COPY to file"),
+	  errhint("Paths for COPY are on the PostgreSQL server, not the client. "
+		  "You may want psql's \\copy or a driver COPY ... TO STDOUT wrapper")));
 
 			oumask = umask(S_IWGRP | S_IWOTH);
 			cstate->copy_file = AllocateFile(cstate->filename, PG_BINARY_W);
@@ -1770,7 +1772,9 @@ BeginCopyTo(Relation rel,
 ereport(ERROR,
 		(errcode_for_file_access(),
 		 errmsg("could not open file \"%s\" for writing: %m",
-cstate->filename)));
+cstate->filename),
+		 errhint("Paths for COPY are on the PostgreSQL server, not the client. "
+		 		 "You may want psql's \\copy or a driver COPY ... TO STDOUT wrapper")));
 
 			if (fstat(fileno(cstate->copy_file), ))
 ereport(ERROR,
@@ -2796,7 +2800,9 @@ BeginCopyFrom(Relation rel,
 ereport(ERROR,
 		(errcode_for_file_access(),
 		 errmsg("could not open file \"%s\" for reading: %m",
-cstate->filename)));
+cstate->filename),
+		 errhint("Paths for COPY are on the PostgreSQL server, not the client. "
+		 		 "You may want psql's \\copy or a driver COPY ... FROM STDIN wrapper")));
 
 			if (fstat(fileno(cstate->copy_file), ))
 ereport(ERROR,
-- 
2.5.5


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

Re: [HACKERS] [PATCH] COPY vs \copy HINT

[Christoph Berg]

Re: Craig Ringer 2016-08-12 

> I think we should emit a HINT here, something like:
> 
> ERROR:  could not open file "D:\CBS_woningcijfers_2014.csv" for reading: No
> such file or directory'
> HINT:  Paths for COPY are on the PostgreSQL server, not the client. You may
> want psql's \copy or a driver COPY ... FROM STDIN wrapper

+1 on the idea.

> postgres=# COPY x TO '/root/nopermissions';
> ERROR:  could not open file "/root/nopermissions" for writing: Permission
> denied
> HINT:  Paths for COPY are on the PostgreSQL server, not the client. You may
> want psql's \copy or a driver COPY ... FROM STDIN wrapper

TO STDOUT.

Also, I vaguely get what you wanted to say with "a driver ...
wrapper", but it's pretty nonsensical if one doesn't know about the
protocol details. I don't have a better suggestion now, but I think it
needs rephrasing.

Christoph


signature.asc
Description: PGP signature