Re: [HACKERS] Bug in to_timestamp().

> On 12 January 2018 at 13:58, Arthur Zakirov <a.zakirov@postgrespro.ru> wrote:
>
> I attached new version of the patch. Now (expected output):
>

Thanks for working on that! I haven't followed this thread before, and after a
quick review I have few side questions.

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.

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.