Re: [HACKERS] [PATCH] COPY vs \copy HINT
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
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), &st))
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), &st))
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
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
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
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
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
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), &st))
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), &st))
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
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), &st))
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 ?
+
Re: [HACKERS] [PATCH] COPY vs \copy HINT
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
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
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
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
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), &st))
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),
+ (save_errno == ENOENT ?
+ errhint("COPY copies from a file on the PostgreSQL ser
Re: [HACKERS] [PATCH] COPY vs \copy HINT
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
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), &st))
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), &st))
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
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
[HACKERS] [PATCH] COPY vs \copy HINT
Hi all
I see this sort of question quite a bit:
http://stackoverflow.com/q/38903811/398670
where the user wonders why
COPY gemeenten
FROM 'D:\CBS_woningcijfers_2014.csv'
DELIMITER ';' CSV
fails with
ERROR: could not open file "D:\CBS_woningcijfers_2014.csv" for reading: No
such file or directory'
and as usual, it's because the path is on their local host not the Pg
server.
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
as a usability improvement. Trivial patch attached.
I'm not sure how to avoid the need to translate the string multiple times,
or if the tooling will automatically flatten them. I haven't bothered with
the stat() failure or isdir cases, since they seem less likely to be the
cause of users being confused between client and server.
Sample output:
postgres=# COPY x FROM '/tmp/somepath';
ERROR: could not open file "/tmp/somepath" 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
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
postgres=# COPY x TO 'relpath';
ERROR: relative path not allowed for COPY to file
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
--
Craig Ringer http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services
From 8e9df8a8874e3ae7446c0e6ebbf4da75889d6027 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..69d2047 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 ... FROM STDIN 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 ... FROM STDIN wrapper")));
if (fstat(fileno(cstate->copy_file), &st))
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), &st))
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
