Re: [HACKERS] Bug in to_timestamp().

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

In response to

Browse pgsql-hackers by date

  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