From: Tom Lane <tgl@sss.pgh.pa.us>
To: Artur Zakirov <a.zakirov@postgrespro.ru>
cc: amul sul <sulamul@gmail.com>, Pavel Stehule <pavel.stehule@gmail.com>,
        Robert Haas <robertmhaas@gmail.com>,
        "David G. Johnston" <david.g.johnston@gmail.com>,
        Alex Ignatov <a.ignatov@postgrespro.ru>,
        Bruce Momjian <bruce@momjian.us>, amul sul <sul_amul@yahoo.co.in>,
        PostgreSQL-development <pgsql-hackers@postgresql.org>
Subject: Re: Bug in to_timestamp().
In-reply-to: 
 <CAKNkYny-0YJZzz=a9E==UBuiEoMAFV=n6tik7oLbDVtahiW7tA@mail.gmail.com>
References: 
 <1873520224.1784572.1465833145330.JavaMail.yahoo.ref@mail.yahoo.com>
 <1873520224.1784572.1465833145330.JavaMail.yahoo@mail.yahoo.com>
 <f64563b3-5c3c-6f28-3f73-41b2159417bc@postgrespro.ru>
 <884791796.213185.1466667686999.JavaMail.yahoo@mail.yahoo.com>
 <20160623133001.GF21246@momjian.us>
 <1877e60a-a970-18b5-453c-e342f96150e6@postgrespro.ru>
 <CAKFQuwZKtS0Ad=a1E9_mkdYwLMq+JA3YjmhCvjJ1JF+3BU8a0g@mail.gmail.com>
 <d469b177-fd4d-5a5b-769e-620957ec1a64@postgrespro.ru>
 <CAKFQuwb2gp8n2ai_agyJod5BTYxL7ek0AjB9zifDSceK6QntnQ@mail.gmail.com>
 <CA+TgmoaWTgbtd2RTwVvySMVoOcHmWq66rCCxtVywOOX3FJy4tQ@mail.gmail.com>
 <31915.1466703655@sss.pgh.pa.us>
 <CA+TgmoaFQZeqmAo3CdivNu70coB=9eM5OroKYtNcTuMaC1kuUQ@mail.gmail.com>
 <32684.1466704957@sss.pgh.pa.us>
 <734e50cd-c563-4afd-d1c8-80f244b3265e@postgrespro.ru>
 <CAFj8pRDGxe=cdcZ7pur=2WeW9O3MQCZTBEAOJfQ7OLQJaYXDvw@mail.gmail.com>
 <b2a39359-3282-b402-f4a3-057aae500ee7@postgrespro.ru>
 <29142.1475097531@sss.pgh.pa.us> <CAAJ_b96+G2F_FYvEmwb9b+0TQd9L!
 aMeWNFVhZSMxXCqHO1m8KQ@mail.gmail.com>
 <CAKNkYny-0YJZzz=a9E==UBuiEoMAFV=n6tik7oLbDVtahiW7tA@mail.gmail.com>
Comments: In-reply-to Artur Zakirov <a.zakirov@postgrespro.ru>
	message dated "Thu, 29 Sep 2016 14:53:34 +0500"
MIME-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-ID: <8743.1478284597.1@sss.pgh.pa.us>
Date: Fri, 04 Nov 2016 14:36:37 -0400
Message-ID: <8744.1478284597@sss.pgh.pa.us>
Precedence: bulk
Sender: pgsql-hackers-owner@postgresql.org

Artur Zakirov <a.zakirov@postgrespro.ru> writes:
> I attached new version of the patch, which fix is_char_separator()
> declaration too.

I did some experimenting using
http://rextester.com/l/oracle_online_compiler

It appears that Oracle will consider a single space in the pattern
to match zero or more spaces in the input, as all of these produce
the expected result:

SELECT to_timestamp('2000JUN', 'YYYY MON') FROM dual
SELECT to_timestamp('2000 JUN', 'YYYY MON') FROM dual
SELECT to_timestamp('2000   JUN', 'YYYY MON') FROM dual

Also, a space in the pattern will match a single separator character
in the input, but not multiple separators:

SELECT to_timestamp('2000-JUN', 'YYYY MON') FROM dual
-- ok
SELECT to_timestamp('2000--JUN', 'YYYY MON') FROM dual
ORA-01843: not a valid month

And you can have whitespace along with that single separator:

SELECT to_timestamp('2000 + JUN', 'YYYY MON') FROM dual
-- ok
SELECT to_timestamp('2000 +   JUN', 'YYYY MON') FROM dual
-- ok
SELECT to_timestamp('2000 ++  JUN', 'YYYY MON') FROM dual
ORA-01843: not a valid month

You can have leading whitespace, but not leading separators:

SELECT to_timestamp('   2000 JUN', 'YYYY MON') FROM dual
-- ok
SELECT to_timestamp('/2000 JUN', 'YYYY MON') FROM dual
ORA-01841: (full) year must be between -4713 and +9999, and not be 0

These all work:

SELECT to_timestamp('2000 JUN', 'YYYY/MON') FROM dual
SELECT to_timestamp('2000JUN', 'YYYY/MON') FROM dual
SELECT to_timestamp('2000/JUN', 'YYYY/MON') FROM dual
SELECT to_timestamp('2000-JUN', 'YYYY/MON') FROM dual

but not

SELECT to_timestamp('2000//JUN', 'YYYY/MON') FROM dual
ORA-01843: not a valid month
SELECT to_timestamp('2000--JUN', 'YYYY/MON') FROM dual
ORA-01843: not a valid month

which makes it look a lot like Oracle treats separator characters in the
pattern the same as spaces (but I haven't checked their documentation to
confirm that).

The proposed patch doesn't seem to me to be trying to follow 
these Oracle behaviors, but I think there is very little reason for
changing any of this stuff unless we move it closer to Oracle.

Some other nitpicking:

* I think the is-separator function would be better coded like

static bool
is_separator_char(const char *str)
{
    /* ASCII printable character, but not letter or digit */
    return (*str > 0x20 && *str < 0x7F &&
            !(*str >= 'A' && *str <= 'Z') &&
            !(*str >= 'a' && *str <= 'z') &&
            !(*str >= '0' && *str <= '9'));
}

The previous way is neither readable nor remarkably efficient, and it
knows much more about the ASCII character set than it needs to.

* Don't forget the cast to unsigned char when using isspace() or other
<ctype.h> functions.

* I do not see the reason for throwing an error here:

+		/* Previous character was a backslash */
+		if (in_backslash)
+		{
+			/* After backslash should go non-space character */
+			if (isspace(*str))
+				ereport(ERROR,
+						(errcode(ERRCODE_SYNTAX_ERROR),
+						 errmsg("invalid escape sequence")));
+			in_backslash = false;

Why shouldn't backslash-space be a valid quoting sequence?

I'll set this back to Waiting on Author.

			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