Re: Bug in to_timestamp().

From: Artur Zakirov <a(dot)zakirov(at)postgrespro(dot)ru>
To: Robert Haas <robertmhaas(at)gmail(dot)com>, amul sul <sul_amul(at)yahoo(dot)co(dot)in>
Cc: Pavel Stehule <pavel(dot)stehule(at)gmail(dot)com>, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>, "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>, PostgreSQL-development <pgsql-hackers(at)postgresql(dot)org>
Subject: Re: Bug in to_timestamp().
Date: 2016-08-24 17:42:25
Message-ID: 03e76bef-75eb-4d70-40d6-890d42f4dc8e@postgrespro.ru
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: pgsql-hackers

Sorry. I did not get last two mails from Amul. Don't know why. So I
reply to another mail.

> Documented as working case, but unfortunatly it does not :
>
> postgres=# SELECT to_timestamp('2000----JUN', 'YYYY MON');
> ERROR: invalid value "---" for "MON"
> DETAIL: The given value did not match any of the allowed values for this field.

Indeed! Fixed it. Now this query executes without error. Added this case
to tests.

> NODE_TYPE_CHAR assumption in else block seems incorrect. What if we have space after double quote? It will again have incorrect output without any error, see below:
>
> postgres=# SELECT to_timestamp('Year: 1976, Month: May, Day: 16',
> postgres(# '" Year:" YYYY, "Month:" FMMonth, "Day:" DD');
> to_timestamp
> ------------------------------
> 0006-05-16 00:00:00-07:52:58
> (1 row)
>
> I guess, we might need NODE_TYPE_SEPARATOR and NODE_TYPE_SPACE check as well?

Agree. Fixed and added to tests.

> Unnecessary hunk?
> We should not touch any code unless it necessary to implement proposed feature, otherwise it will add unnecessary diff to the patch and eventually extra burden to review the code. Similarly hunk in the patch @ line # 313 - 410, nothing to do with to_timestamp behaviour improvement, IIUC.
>
> If you think this changes need to be in, please submit separate cleanup-patch.

Fixed it. It was a typo.
About lines # 313 - 410. It is necessary to avoid bugs. I wrote aboud it
in
https://www.postgresql.org/message-id/b2a39359-3282-b402-f4a3-057aae500ee7%40postgrespro.ru
:

> - now DCH_cache_getnew() is called after parse_format(). Because now
> parse_format() can raise an error and in the next attempt
> DCH_cache_search() could return broken cache entry.

For example, you can test incorrect inputs for to_timestamp(). Try to
execute such query several times.

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

Attachment Content-Type Size
0001-to-timestamp-format-checking-v3.patch text/x-patch 18.3 KB
0002-to-timestamp-validation-v2.patch text/x-patch 6.3 KB

In response to

Responses

Browse pgsql-hackers by date

  From Date Subject
Next Message Kevin Grittner 2016-08-24 18:00:40 Re: [COMMITTERS] pgsql: Modify BufferGetPage() to prepare for "snapshot too old" feature
Previous Message Alvaro Herrera 2016-08-24 17:40:17 Re: [COMMITTERS] pgsql: Modify BufferGetPage() to prepare for "snapshot too old" feature