On Sun, 2006-01-29 at 17:03 -0800, David Fetter wrote:
+ /* Disallow the forbidden_delimiter strings */
+ if (strcspn(cstate->delim, BADCHARS) != 1)
+ elog(ERROR, "COPY delimiter cannot be %#02x",
+ *cstate->delim);
+

The comment is still wrong: referencing "forbidden_delimiter" makes it
sound like there is something named forbidden_delimiter, but there is
not (at least in the patch as submitted).

The patch should also use ereport rather than elog, because this error
message might reasonably be encountered by the user.

-Neil



---------------------------(end of broadcast)---------------------------
TIP 4: Have you searched our list archives?

http://archives.postgresql.org

--UlVJffcvxoiEqYs2
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline

On Sun, Jan 29, 2006 at 10:20:47PM -0500, Neil Conway wrote:
Patch with BADCHARS attached

Cheers,
D
--
David Fetter david (AT) fetter (DOT) org http://fetter.org/
phone: +1 415 235 3778

Remember to vote!

--UlVJffcvxoiEqYs2
Content-Type: text/plain; charset=us-ascii
Content-Disposition: attachment; filename="forbid_badchars_copy.diff"

Index: src/backend/commands/copy.c
================================================== =================
RCS file: /projects/cvsroot/pgsql/src/backend/commands/copy.c,v
retrieving revision 1.257
diff -c -r1.257 copy.c
*** src/backend/commands/copy.c 28 Dec 2005 03:25:32 -0000 1.257
--- src/backend/commands/copy.c 30 Jan 2006 06:44:10 -0000
***************
*** 51,56 ****
--- 51,57 ----

#define ISOCTAL(c) (((c) >= '0') && ((c) <= '7'))
#define OCTVALUE(c) ((c) - '0')
+ #define BADCHARS "\r\n\\"

/*
* Represents the different source/dest cases we need to worry about at
***************
*** 856,861 ****
--- 857,869 ----
(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
errmsg("COPY delimiter must be a single character")));

+ /* Disallow BADCHARS characters */
+ if (strcspn(cstate->delim, BADCHARS) != 1)
+ ereport(ERROR,
+ (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
+ errmsg("COPY delimiter cannot be \"%#02x\"",
+ *cstate->delim)));
+
/* Check header */
if (!cstate->csv_mode && cstate->header_line)
ereport(ERROR,

--UlVJffcvxoiEqYs2
Content-Type: text/plain
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
MIME-Version: 1.0


---------------------------(end of broadcast)---------------------------
TIP 3: Have you checked our extensive FAQ?

http://www.postgresql.org/docs/faq

--UlVJffcvxoiEqYs2--

David Fetter wrote:

Is ERRCODE_FEATURE_NOT_SUPPORTED the right errcode? This isn't a
missing feature; we are performing a sanity check here. We can
reasonably expect never to support CR, LF or \ as the text delimiter.
Maybe ERRCODE_INVALID_PARAMETER_VALUE ? Or maybe we need a new one.

Also, I would probably make the format %#.02x so the result would look
like 0x0d (for a CR).

(I bet David never thought there would so much fuss over a handful of
lines of code)

cheers

andrew

---------------------------(end of broadcast)---------------------------
TIP 5: don't forget to increase your free space map settings

On Mon, Jan 30, 2006 at 08:21:34AM -0500, Andrew Dunstan wrote:
I guess that depends on whether we ever plan to allow people to set
the output record separator to something other than CR?LF.

Actually, I'm happy to see it's getting QA. COPY is something that
has Consequences™ if anything goes wrong with it, so I'd rather do
best efforts up front to get it right.

Cheers,
D
--
David Fetter david (AT) fetter (DOT) org http://fetter.org/
phone: +1 415 235 3778

Remember to vote!

---------------------------(end of broadcast)---------------------------
TIP 9: In versions below 8.0, the planner will ignore your desire to
choose an index scan if your joining column's datatypes do not
match

I guess that depends on whether we ever plan to allow people to set
the output record separator to something other than CR?LF.

Agreed. Right now it is invalid and there are no plans to support other
values for end-of-line. I will make the change when the patch is
applied.

--
Bruce Momjian | http://candle.pha.pa.us
pgman (AT) candle (DOT) pha.pa.us | (610) 359-1001
+ If your life is a hard drive, | 13 Roberts Road
+ Christ can be your backup. | Newtown Square, Pennsylvania 19073

---------------------------(end of broadcast)---------------------------
TIP 6: explain analyze is your friend

Uh, couldn't the delimiter be a backslash in CVS mode?

+ #define BADCHARS "\r\n\\"

Also, should we disable DELIMITER and NULL from sharing characters?

---------------------------------------------------------------------------

David Fetter wrote:
[ Attachment, skipping... ]

--
Bruce Momjian | http://candle.pha.pa.us
pgman (AT) candle (DOT) pha.pa.us | (610) 359-1001
+ If your life is a hard drive, | 13 Roberts Road
+ Christ can be your backup. | Newtown Square, Pennsylvania 19073

---------------------------(end of broadcast)---------------------------
TIP 6: explain analyze is your friend

On Tue, Jan 31, 2006 at 08:03:41PM -0500, Bruce Momjian wrote:
I don't think so. Folks?

Anyhow, if there are different sets, I could do something like:

#define BADCHARS "\r\n\\"
#define BADCHARS_CSV "\r\n"

and then check for csv_mode, etc.

That's on about line 916, post-patch:

/* Don't allow the delimiter to appear in the null string. */
if (strchr(cstate->null_print, cstate->delim[0]) != NULL)
ereport(ERROR,
(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
errmsg("COPY delimiter must not appear in the NULL specification")));

I suppose that a different error code might be The Right Thing™ here.

Cheers,
D
--
David Fetter david (AT) fetter (DOT) org http://fetter.org/
phone: +1 415 235 3778

Remember to vote!

---------------------------(end of broadcast)---------------------------
TIP 4: Have you searched our list archives?

http://archives.postgresql.org

On Tue, Jan 31, 2006 at 09:50:26PM -0600, Andrew Dunstan wrote:
<aol>So do I.</aol>

That's an excellent candidate, or maybe ERRCODE_INVALID_PARAMETER_VALUE.
My vote is for ERRCODE_D00D_WTF

Maybe we need an error code for mutually incompatible param values.

Cheers,
D
--
David Fetter david (AT) fetter (DOT) org http://fetter.org/
phone: +1 415 235 3778

Remember to vote!

---------------------------(end of broadcast)---------------------------
TIP 1: if posting/reading through Usenet, please send an appropriate
subscribe-nomail command to majordomo (AT) postgresql (DOT) org so that your
message can get through to the mailing list cleanly

David Fetter said:
Using backslash as a delimiter in CSV would be odd, to say the least. As an
escape char it is occasionally used, but not as a delimiter in my
experience. Maybe we should apply the "be liberal in what you accept" rule,
but I think this would be stretching it.

ERRCODE_WHAT WERE_YOU_THINKING ?

cheers

andrew



---------------------------(end of broadcast)---------------------------
TIP 6: explain analyze is your friend