Обсуждение: Should CSV parsing be stricter about mid-field quotes?

On Thu, 11 May 2023 at 10:04, Joel Jacobson <joel@compiler.org> wrote:
>
> The parser currently accepts quoting within an unquoted field. This can lead to
> data misinterpretation when the quote is part of the field data (e.g.,
> for inches, like in the example).

I think you're thinking about it differently than the parser. I think
the parser is treating this the way, say, the shell treats quotes.
That is, it sees a quoted "I bought this for my 6" followed by an
unquoted "a laptop but it didn't fit my 8" followed by a quoted "
tablet".

So for example, in that world you might only quote commas and newlines
so you might print something like

1,2,I bought this for my "6"" laptop
" but it "didn't" fit my "8""" laptop

The actual CSV spec https://datatracker.ietf.org/doc/html/rfc4180 only
allows fully quoted or fully unquoted fields and there can only be
escaped double-doublequote characters in quoted fields and no
doublequote characters in unquoted fields.

But it also says

      Due to lack of a single specification, there are considerable
      differences among implementations.  Implementors should "be
      conservative in what you do, be liberal in what you accept from
      others" (RFC 793 [8]) when processing CSV files.  An attempt at a
      common definition can be found in Section 2.


So the real question is are there tools out there that generate
entries like this and what are their intentions?

> I think we should throw a parsing error for unescaped mid-field quotes,
> and add a COPY option like ALLOW_MIDFIELD_QUOTES for cases where mid-field
> quotes are necessary. The error message could suggest this option when it
> encounters an unescaped mid-field quote.
>
> I think the convenience of not having to use an extra option doesn't outweigh
> the risk of undetected data integrity issues.

It's also a pretty annoying experience to get a message saying "error,
turn this option on to not get an error". I get what you're saying
too, which is more of a risk depends on whether turning off the error
is really the right thing most of the time or is just causing data to
be read incorrectly.



-- 
greg

Maybe this is unexpected by you, but it's not by me. What other sane interpretation of that data could there be? And what CSV producer outputs such horrible content? As you've noted, ours certainly does not. Our rules are clear: quotes within quotes must be escaped (default escape is by doubling the quote char). Allowing partial fields to be quoted was a deliberate decision when CSV parsing was implemented, because examples have been seen in the wild.

So I don't think our behaviour is broken or needs fixing. As mentioned by Greg, this is an example of the adage about being liberal in what you accept.

cheers

andrew

--
Andrew Dunstan
EDB: https://www.enterprisedb.com

Maybe this is unexpected by you, but it's not by me. What other sane interpretation of that data could there be? And what CSV producer outputs such horrible content? As you've noted, ours certainly does not. Our rules are clear: quotes within quotes must be escaped (default escape is by doubling the quote char). Allowing partial fields to be quoted was a deliberate decision when CSV parsing was implemented, because examples have been seen in the wild.

So I don't think our behaviour is broken or needs fixing. As mentioned by Greg, this is an example of the adage about being liberal in what you accept.

Maybe this is unexpected by you, but it's not by me. What other sane interpretation of that data could there be? And what CSV producer outputs such horrible content? As you've noted, ours certainly does not. Our rules are clear: quotes within quotes must be escaped (default escape is by doubling the quote char). Allowing partial fields to be quoted was a deliberate decision when CSV parsing was implemented, because examples have been seen in the wild.

So I don't think our behaviour is broken or needs fixing. As mentioned by Greg, this is an example of the adage about being liberal in what you accept.

I'm pretty reluctant to change something that's been working as designed for almost 20 years, and about which we have hitherto had zero complaints that I recall.

I could see an argument for a STRICT mode which would disallow partially quoted fields, although I'd like some evidence that we're dealing with a real problem here. Is there really a CSV producer that produces output like that you showed in your example? And if so has anyone objected to them about the insanity of that?

cheers

andrew

--
Andrew Dunstan
EDB: https://www.enterprisedb.com

Andrew Dunstan <andrew@dunslane.net> writes:
> I could see an argument for a STRICT mode which would disallow partially 
> quoted fields, although I'd like some evidence that we're dealing with a 
> real problem here. Is there really a CSV producer that produces output 
> like that you showed in your example? And if so has anyone objected to 
> them about the insanity of that?

I think you'd want not just "some evidence" but "compelling evidence".
Any such option is going to add cycles into the low-level input parser
for COPY, which we know is a hot spot and we've expended plenty of
sweat on.  Adding a speed penalty that will be paid by the 99.99%
of users who don't have an issue here is going to be a hard sell.

            regards, tom lane

On Sat, 13 May 2023 at 09:46, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>
> Andrew Dunstan <andrew@dunslane.net> writes:
> > I could see an argument for a STRICT mode which would disallow partially
> > quoted fields, although I'd like some evidence that we're dealing with a
> > real problem here. Is there really a CSV producer that produces output
> > like that you showed in your example? And if so has anyone objected to
> > them about the insanity of that?
>
> I think you'd want not just "some evidence" but "compelling evidence".
> Any such option is going to add cycles into the low-level input parser
> for COPY, which we know is a hot spot and we've expended plenty of
> sweat on.  Adding a speed penalty that will be paid by the 99.99%
> of users who don't have an issue here is going to be a hard sell.

Well I'm not sure that follows. Joel specifically claimed that an
implementation that didn't accept inputs like this would actually be
simpler and that might mean it would actually be faster.

And I don't think you have to look very hard for inputs like this --
plenty of people generate CSV files from simple templates or script
outputs that don't understand escaping quotation marks at all. Outputs
like that will be fine as long as there's no doublequotes in the
inputs but then one day someone will enter a doublequote in a form
somewhere and blammo.

So I guess the real question is whether accepting inputs with
unescapted quotes and interpreting them the way we do is really the
best interpretation. Is the user best served by a) assuming they
intended to quote part of the field and not quote part of it b) assume
they failed to escape the quotation mark or c) assume something's gone
wrong and the input is entirely untrustworthy.

-- 
greg

On Sat, 13 May 2023 at 09:46, Tom Lane <tgl@sss.pgh.pa.us> wrote:

Andrew Dunstan <andrew@dunslane.net> writes:

I could see an argument for a STRICT mode which would disallow partially
quoted fields, although I'd like some evidence that we're dealing with a
real problem here. Is there really a CSV producer that produces output
like that you showed in your example? And if so has anyone objected to
them about the insanity of that?

I think you'd want not just "some evidence" but "compelling evidence".
Any such option is going to add cycles into the low-level input parser
for COPY, which we know is a hot spot and we've expended plenty of
sweat on.  Adding a speed penalty that will be paid by the 99.99%
of users who don't have an issue here is going to be a hard sell.

Well I'm not sure that follows. Joel specifically claimed that an
implementation that didn't accept inputs like this would actually be
simpler and that might mean it would actually be faster.

And I don't think you have to look very hard for inputs like this --
plenty of people generate CSV files from simple templates or script
outputs that don't understand escaping quotation marks at all. Outputs
like that will be fine as long as there's no doublequotes in the
inputs but then one day someone will enter a doublequote in a form
somewhere and blammo.

The procedure described is plain wrong, and I don't have too much sympathy for people who implement it. Parsing CSV files might be a mild PITN, but constructing them is pretty darn simple. Something like this perl fragment should do it:

do {
  s/"/""/g;
  $_ = qq{"$_"} if /[",\r\n]/;
} foreach @fields;
print join(',',@fields),"\n";

And if people do follow the method you describe then their input with unescaped quotes will be rejected 999 times out of 1000. It's only cases where the field happens to have an even number of embedded quotes, like Joel's somewhat contrived example, that the input will be accepted.

So I guess the real question is whether accepting inputs with
unescapted quotes and interpreting them the way we do is really the
best interpretation. Is the user best served by a) assuming they
intended to quote part of the field and not quote part of it b) assume
they failed to escape the quotation mark or c) assume something's gone
wrong and the input is entirely untrustworthy.

As I said earlier, I'm quite reluctant to break things that might have been working happily for people for many years, in order to accommodate people who can't do the minimum required to produce correct CSVs. I have no idea how many are relying on it, but I would be slightly surprised if the number were zero.

cheers

andrew

--
Andrew Dunstan
EDB: https://www.enterprisedb.com

You can use CSV mode pretty reliably for TSV files. The trick is to use a quoting char that shouldn't appear, such as E'\x01' as well as setting the delimiter to E'\t'. Yes, it's far from obvious.

cheers

andrew

--
Andrew Dunstan
EDB: https://www.enterprisedb.com

QUOTE NONE and DELIMITER NONE should work fine. NONE is already a keyword, so the disturbance should be minimal.

cheers

andrew

--
Andrew Dunstan
EDB: https://www.enterprisedb.com

    Joel Jacobson wrote:

> I've been using that trick myself many times in the past, but thanks to this
> deep-dive into this topic, it looks to me like TEXT would be a better format
> fit when dealing with unquoted TSV files, or?
>
> OTOH, one would then need to inspect the TSV file doesn't contain \. on an
> empty line...

Note that this is the case for valid CSV contents, since backslash-dot
on a line by itself is both an end-of-data marker for COPY FROM and a
valid CSV line.
Having this line in the data results in either an error or having the
rest of the data silently discarded, depending on the context. There
is some previous discussion about this in [1].
Since the TEXT format doesn't have this kind of problem, one solution
is to filter the data through PROGRAM with an [untrusted CSV]->TEXT
filter. This is to be preferred over direct CSV loading when
strictness or robustness are more important than convenience.


[1]
https://www.postgresql.org/message-id/10e3eff6-eb04-4b3f-aeb4-b920192b977a@manitou-mail.org

Best regards,
--
Daniel Vérité
https://postgresql.verite.pro/
Twitter: @DanielVerite

On Thu, May 18, 2023, at 18:48, Daniel Verite wrote:
> Joel Jacobson wrote:
>> OTOH, one would then need to inspect the TSV file doesn't contain \. on an
>> empty line...
>
> Note that this is the case for valid CSV contents, since backslash-dot
> on a line by itself is both an end-of-data marker for COPY FROM and a
> valid CSV line.
> Having this line in the data results in either an error or having the
> rest of the data silently discarded, depending on the context. There
> is some previous discussion about this in [1].
> Since the TEXT format doesn't have this kind of problem, one solution
> is to filter the data through PROGRAM with an [untrusted CSV]->TEXT
> filter. This is to be preferred over direct CSV loading when
> strictness or robustness are more important than convenience.
>
>
> [1]
> https://www.postgresql.org/message-id/10e3eff6-eb04-4b3f-aeb4-b920192b977a@manitou-mail.org

Thanks for sharing the old thread, very useful.
I see I've failed miserably to understand all the details of the COPY command.
 
Upon reading the thread, I'm still puzzled about one thing:

Why does \. need to have a special meaning when using COPY FROM with files?

I understand its necessity for STDIN, given that the end of input needs to be
explicitly defined.
However, for files, we have a known file size and the end-of-file can be
detected without the need for special markers.

Also, is the difference in how server-side COPY CSV is capable of dealing
with \. but apparently not the client-side \COPY CSV documented somewhere?

CREATE TABLE t (c text);
INSERT INTO t (c) VALUES ('foo'), (E'\n\\.\n'), ('bar');

-- Works OK:
COPY t TO '/tmp/t.csv' WITH CSV;
TRUNCATE t;
COPY t FROM '/tmp/t.csv' WITH CSV;

-- Doesn't work:
\COPY t TO '/tmp/t.csv' WITH CSV;
TRUNCATE t;
\COPY t FROM '/tmp/t.csv' WITH CSV;
ERROR:  unterminated CSV quoted field
CONTEXT:  COPY t, line 4: ""
\.
"

/Joel

    Joel Jacobson wrote:

> I understand its necessity for STDIN, given that the end of input needs to
> be explicitly defined.
> However, for files, we have a known file size and the end-of-file can be
> detected without the need for special markers.
>
> Also, is the difference in how server-side COPY CSV is capable of dealing
> with \. but apparently not the client-side \COPY CSV documented somewhere?

psql implements the client-side "\copy table from file..." with
COPY table FROM STDIN ...

COPY FROM file CSV somewhat differs as your example shows,
but it still mishandle \. when unquoted. For instance, consider this
file to load with COPY    t FROM '/tmp/t.csv' WITH CSV
$ cat /tmp/t.csv
line 1
\.
line 3
line 4

It results in having only "line 1" being imported.


Best regards,
--
Daniel Vérité
https://postgresql.verite.pro/
Twitter: @DanielVerite

On Fri, May 19, 2023, at 18:06, Daniel Verite wrote:
> COPY FROM file CSV somewhat differs as your example shows,
> but it still mishandle \. when unquoted. For instance, consider this
> file to load with COPY    t FROM '/tmp/t.csv' WITH CSV
> $ cat /tmp/t.csv
> line 1
> \.
> line 3
> line 4
>
> It results in having only "line 1" being imported.

Hmm, this is a problem for one of the new use-cases I brought up that would be
possible with DELIMITER NONE QUOTE NONE, i.e. to import unstructured log files,
where each raw line should be imported "as is" into a single text column.

Is there a valid reason why \. is needed for COPY FROM filename?
It seems to me it would only be necessary for the COPY FROM STDIN case,
since files have a natural end-of-file and a known file size.

/Joel

    Joel Jacobson wrote:

> Is there a valid reason why \. is needed for COPY FROM filename?
> It seems to me it would only be necessary for the COPY FROM STDIN case,
> since files have a natural end-of-file and a known file size.

Looking at CopyReadLineText() over at [1], I don't see a reason why
the unquoted \. could not be handled with COPY FROM file.
Even COPY FROM STDIN looks like it could be benefit, so that
\copy from file csv would hopefully not choke or truncate the data.
There's still the case when the CSV data is embedded in a psql script
(psql is unable to know where it ends), but for that, "don't do that"
might be a reasonable answer.


[1]
https://doxygen.postgresql.org/copyfromparse_8c.html#a90201f711221dd82d0c08deedd91e1b3



Best regards,
--
Daniel Vérité
https://postgresql.verite.pro/
Twitter: @DanielVerite

    Kirk Wolak wrote:

> We do NOT do "CSV", we mimic pg_dump.

pg_dump uses the text format (as opposed to csv), where
\. on a line by itself cannot appear in the data, so there's
no problem. The problem is limited to the csv format.


Best regards,
--
Daniel Vérité
https://postgresql.verite.pro/
Twitter: @DanielVerite

On Sat, May 20, 2023 at 09:16:30AM +0200, Joel Jacobson wrote:
> On Fri, May 19, 2023, at 18:06, Daniel Verite wrote:
> > COPY FROM file CSV somewhat differs as your example shows,
> > but it still mishandle \. when unquoted. For instance, consider this
> > file to load with COPY    t FROM '/tmp/t.csv' WITH CSV
> > $ cat /tmp/t.csv
> > line 1
> > \.
> > line 3
> > line 4
> >
> > It results in having only "line 1" being imported.
> 
> Hmm, this is a problem for one of the new use-cases I brought up that would be
> possible with DELIMITER NONE QUOTE NONE, i.e. to import unstructured log files,
> where each raw line should be imported "as is" into a single text column.
> 
> Is there a valid reason why \. is needed for COPY FROM filename?

No.

> It seems to me it would only be necessary for the COPY FROM STDIN case,
> since files have a natural end-of-file and a known file size.

Right.  Even for COPY FROM STDIN, it's not needed anymore since the removal of
protocol v2.  psql would still use it to find the end of inline COPY data,
though.  Here's another relevant thread:
https://postgr.es/m/flat/bfcd57e4-8f23-4c3e-a5db-2571d09208e2%40beta.fastmail.com