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