Re: Bug in to_timestamp().

From: Artur Zakirov <a(dot)zakirov(at)postgrespro(dot)ru>
To: Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>
Cc: Artur Zakirov <a(dot)zakirov(at)postgrespro(dot)ru>, amul sul <sulamul(at)gmail(dot)com>, Pavel Stehule <pavel(dot)stehule(at)gmail(dot)com>, Robert Haas <robertmhaas(at)gmail(dot)com>, "David G(dot) Johnston" <david(dot)g(dot)johnston(at)gmail(dot)com>, Alex Ignatov <a(dot)ignatov(at)postgrespro(dot)ru>, Bruce Momjian <bruce(at)momjian(dot)us>, amul sul <sul_amul(at)yahoo(dot)co(dot)in>, PostgreSQL-development <pgsql-hackers(at)postgresql(dot)org>
Subject: Re: Bug in to_timestamp().
Date: 2016-11-06 15:26:57
Message-ID: CAKNkYnxCqwC+aXZFXent27SU5=nonR6JPxJDxfnf9YiXW71PKA@mail.gmail.com
Views: Raw Message | Whole Thread | Download mbox
Thread:
Lists: pgsql-hackers

Thank you for your comments,

2016-11-04 20:36 GMT+02:00 Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>:
>
> Artur Zakirov <a(dot)zakirov(at)postgrespro(dot)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
>

>
> 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.

Previous versions of the patch doesn't try to follow all Oracle
behaviors. It tries to fix Amul Sul's behaviors. Because I was
confused by dealing with spaces and separators by Oracle
to_timestamp() and there is not information about it in the Oracle
documentation:
https://docs.oracle.com/cd/B19306_01/server.102/b14200/sql_elements004.htm#g195443

I've thought better about it now and fixed the patch. Now parser
removes spaces after and before fields and insists that count of
separators in the input string should match count of spaces or
separators in the formatting string (but in formatting string we can
have more separators than in input string).

>
> 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.

Fixed.

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

Fixed.

>
> * 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?
>

Hm, truly. Fixed it.

I've attached the fixed patch.

--
Sincerely,
Artur Zakirov
Postgres Professional: http://www.postgrespro.com
Russian Postgres Company

Attachment Content-Type Size
0001-to-timestamp-format-checking-v7.patch text/x-patch 17.4 KB

In response to

Responses

Browse pgsql-hackers by date

  From Date Subject
Next Message Artur Zakirov 2016-11-06 17:48:18 Re: [HACKERS] Re: [PATCH] Tab completion for ALTER TYPE … RENAME VALUE …
Previous Message Michael Paquier 2016-11-06 12:12:24 Re: BUG #13755: pgwin32_is_service not checking if SECURITY_SERVICE_SID is disabled