Re: Bug in to_timestamp().

Thank you for your comments,

2016-11-04 20:36 GMT+02:00 Tom Lane <tgl@sss.pgh.pa.us>:
>
> 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
>

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