tighten input to float4/float8/oid

Neil Conway <neilc@samurai.com> writes:

- emit a warning when there is trailing whitespace in the input to the
oid type, per suggestion from Tom (BTW, I couldn't see anything about
this in the standard, although I may have been looking in the wrong
place; in any case, rejecting trailing whitespace seems clearly
correct behavior to me)

I think this is wrong. We silently accept leading whitespace in
(IIRC) all the numeric datatypes, and I believe we should accept
trailing whitespace too. The SQL spec does not speak anywhere that
I can see to the external representation of datatypes, but it has
plenty to say about the behavior of casts, and it seems reasonable
to me to consider the spec's description of casting to and from
character-string data as normative for I/O. (Especially since we
tend to use the same code for both purposes.) In SQL92 section
6.10 <cast specification> I find

3) If TD is exact numeric, then
...
b) If SD is character string, then SV is replaced by SV with any
leading or trailing <space>s removed.
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

Case:

i) If SV does not comprise a <signed numeric literal> as
defined by the rules for <literal> in Subclause 5.3,
"<literal>", then an exception condition is raised: data
exception-invalid character value for cast.

ii) Otherwise, let LT be that <signed numeric literal>. The
<cast specification> is equivalent to

CAST ( LT AS TD )

This looks to me like it's perfectly clear that the input can have
leading or trailing spaces, but not leading or trailing non-space
junk.

The spec means <space> to indicate the space character only, so
allowing other whitespace characters (as the existing code with
isspace() does) is perhaps a bit arguable. I think it's reasonable
to allow any whitespace, though.

+                  errdetail("this input will be rejected in "
+                            "a future release of PostgreSQL")));

Minor stylistic gripe here: errdetail and errhint messages are
supposed to be complete sentences --- this is a deliberate departure
from the style for primary errmsg messages. So the above should be

+                  errdetail("This input will be rejected in "
+                            "a future release of PostgreSQL.")));

regards, tom lane

Tom Lane wrote:

I think this is wrong. We silently accept leading whitespace in
(IIRC) all the numeric datatypes, and I believe we should accept
trailing whitespace too.

(Sorry, I had misremembered your suggestion -- you had earlier said
that the spec probably allows for leading and trailing whitespace.)

I think there's a case to be made that we shouldn't accept either
leading or trailing whitespace, but it seems too late to go down that
path: it isn't worth breaking backward compatibility over, for one thing.

So if we're going to continue to accept leading whitespace, it seems
only reasonable to consistently accept trailing whitespace as well.

Minor stylistic gripe here: errdetail and errhint messages are
supposed to be complete sentences

Thanks for catching that, I'll fix it before applying.

I'll apply the patch this evening without the trailing whitespace
change; if there's a consensus that allowing trailing whitespace is
the best course, I'll post a separate patch for that shortly.

-Neil

Neil Conway <neilc@samurai.com> writes:

I think there's a case to be made that we shouldn't accept either
leading or trailing whitespace, but it seems too late to go down that
path: it isn't worth breaking backward compatibility over, for one thing.

To do that we'd have to use separate code for I/O and casting. As
an example, this current behavior is unquestionably contrary to the
spec text I cited:

regression=# select ' 42'::text::int;
int4
------
42
(1 row)

regression=# select ' 42 '::text::int;
ERROR: invalid input syntax for integer: " 42 "
regression=#

regards, tom lane