Re: [HACKERS] Bug in to_timestamp().

On Wed, Jan 31, 2018 at 05:53:29PM +0100, Dmitry Dolgov wrote:
> Thanks for working on that! I haven't followed this thread before, and after a
> quick review I have few side questions.

Thank you for your comments!

> Why not write `is_separator_char` using `isprint`, `isalpha`, `isdigit` from
> ctype.h? Something like:
> 
>     return isprint(*str) && !isalpha(*str) && !isdigit(*str)
> 
> From what I see in the source code they do exactly the same and tests are
> successfully passing with this change.

Fixed. The patch uses those functions now. I made is_separator_char() as a
IS_SEPARATOR_CHAR() macro.

> What do you think about providing two slightly different messages for these two
> situations:
> 
>     if (n->type == NODE_TYPE_SPACE && !isspace((unsigned char) *s))
>         ereport(ERROR,
>                 (errcode(ERRCODE_INVALID_DATETIME_FORMAT),
>                  errmsg("unexpected character \"%.*s\", expected \"%s\"",
>                         pg_mblen(s), s, n->character),
>                  errhint("In FX mode, punctuation in the input string "
>                          "must exactly match the format string.")));
>     /*
>      * In FX mode we insist that separator character from the format
>      * string matches separator character from the input string.
>      */
>     else if (n->type == NODE_TYPE_SEPARATOR && *n->character != *s)
>         ereport(ERROR,
>                 (errcode(ERRCODE_INVALID_DATETIME_FORMAT),
>                  errmsg("unexpected character \"%.*s\", expected \"%s\"",
>                         pg_mblen(s), s, n->character),
>                  errhint("In FX mode, punctuation in the input string "
>                          "must exactly match the format string.")));
> 
> E.g. "unexpected space character" and "unexpected separator character". The
> difference is quite subtle, but I think a bit of context would never hurt.

I fixed those messages, but in a different manner. I think that an
unexpected character is unknown and neither space nor separator. And
better to say that was expected space/separator character.

Attached fixed patch.

-- 
Arthur Zakirov
Postgres Professional: http://www.postgrespro.com
Russian Postgres Company