From: | Arthur Zakirov <a(dot)zakirov(at)postgrespro(dot)ru> |
---|---|
To: | Dmitry Dolgov <9erthalion6(at)gmail(dot)com> |
Cc: | Thomas Munro <thomas(dot)munro(at)enterprisedb(dot)com>, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>, 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: [HACKERS] Bug in to_timestamp(). |
Date: | 2018-02-01 10:24:51 |
Message-ID: | 20180201102449.GA29082@zakirov.localdomain |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | pgsql-hackers |
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
Attachment | Content-Type | Size |
---|---|---|
0001-to-timestamp-format-checking-v10.patch | text/plain | 10.1 KB |
From | Date | Subject | |
---|---|---|---|
Next Message | Rajkumar Raghuwanshi | 2018-02-01 10:29:06 | Query running for very long time (server hanged) with parallel append |
Previous Message | Konstantin Knizhnik | 2018-02-01 10:24:21 | Re: Built-in connection pooling |