On 2 September 2016 at 04:28, Tom Lane <t...@sss.pgh.pa.us> wrote:
> Craig Ringer <cr...@2ndquadrant.com> writes:
>> On 12 August 2016 at 16:34, Christoph Berg <m...@debian.org> 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 <cr...@2ndquadrant.com>
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 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
