pg.ddx.io pgsql-hackers@postgresql.org mailing list archive help / color / mirror / Atom feed From: amul sul <sul_amul@yahoo.co.in>
To: Artur Zakirov <a.zakirov@postgrespro.ru>
To: Robert Haas <robertmhaas@gmail.com>
Cc: Pavel Stehule <pavel.stehule@gmail.com>
Cc: Tom Lane <tgl@sss.pgh.pa.us>
Cc: David G. Johnston <david.g.johnston@gmail.com>
Cc: Alex Ignatov <a.ignatov@postgrespro.ru>
Cc: Bruce Momjian <bruce@momjian.us>
Cc: PostgreSQL-development <pgsql-hackers@postgresql.org>
Subject: Re: Bug in to_timestamp().
Date: Tue, 23 Aug 2016 05:28:37 +0000 (UTC)
Message-ID: <234265385.16298751.1471930117094.JavaMail.yahoo@mail.yahoo.com> (raw )
In-Reply-To: <60cc3122-0950-e455-21fe-de89882e285d@postgrespro.ru >
References: <1873520224.1784572.1465833145330.JavaMail.yahoo.ref@mail.yahoo.com >
<884791796.213185.1466667686999.JavaMail.yahoo@mail.yahoo.com >
<20160623133001.GF21246@momjian.us >
<1877e60a-a970-18b5-453c-e342f96150e6@postgrespro.ru >
<CAKFQuwZKtS0Ad=a1E9_mkdYwLMq+JA3YjmhCvjJ1JF+3BU8a0g@mail.gmail.com >
<d469b177-fd4d-5a5b-769e-620957ec1a64@postgrespro.ru >
<CAKFQuwb2gp8n2ai_agyJod5BTYxL7ek0AjB9zifDSceK6QntnQ@mail.gmail.com >
<CA+TgmoaWTgbtd2RTwVvySMVoOcHmWq66rCCxtVywOOX3FJy4tQ@mail.gmail.com >
<31915.1466703655@sss.pgh.pa.us >
<CA+TgmoaFQZeqmAo3CdivNu70coB=9eM5OroKYtNcTuMaC1kuUQ@mail.gmail.com >
<32684.1466704957@sss.pgh.pa.us >
<734e50cd-c563-4afd-d1c8-80f244b3265e@postgrespro.ru >
<CAFj8pRDGxe=cdcZ7pur=2WeW9O3MQCZTBEAOJfQ7OLQJaYXDvw@mail.gmail.com >
<b2a39359-3282-b402-f4a3-057aae500ee7@postgrespro.ru >
<1285870380.13225431.1471272994354.JavaMail.yahoo@mail.yahoo.com >
<CA+TgmoZEWyszzwDGFZ2i1psDgHrdPeZE5Hvs570Q1KfkCfqwgw@mail.gmail.com >
<d7ffcce8-491f-7969-71a6-73272ad31dfd@postgrespro.ru >
<60cc3122-0950-e455-21fe-de89882e285d@postgrespro.ru >
List-Unsubscribe: <mailto:majordomo@postgresql.org?body=unsub%20pgsql-hackers>
Hi Artur Zakirov,
Please see following review comment for "0001-to-timestamp-format-checking-v2.patch" & share your thought:
#1.
15 + <literal>to_timestamp('2000----JUN', 'YYYY MON')</literal>
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.
#2.
102 + /* Previous character was a quote */
103 + else if (in_text)
104 + {
105 + if (*str == '"')
106 + {
107 + str++;
108 + in_text = false;
109 + }
110 + else if (*str == '\\')
111 + {
112 + str++;
113 + in_backslash = true;
114 + }
115 + else
116 + {
117 + n->type = NODE_TYPE_CHAR;
118 + n->character = *str;
119 + n->key = NULL;
120 + n->suffix = 0;
121 + n++;
122 + str++;
123 + }
124 + continue;
125 + }
126 +
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?
#3.
296 - /* Ignore spaces before fields when not in FX (fixed width) mode */
297 + /* Ignore spaces before fields when not in FX (fixed * width) mode */
298 if (!fx_mode && n->key->id != DCH_FX)
299 {
300 - while (*s != '\0' && isspace((unsigned char) *s))
301 + while (*s != '\0' && (isspace((unsigned char) *s)))
302 s++;
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.
>I attached second patch "0002-to-timestamp-validation-v2.patch". With it
>PostgreSQL perform additional checks for date and time. But as I wrote
>there is another patch in the thread "to_date_valid()" wich differs from
>this patch.
@community : I am not sure what to do with this patch, should we keep it as separate enhancement?
Regards,
Amul Sul
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
view thread (138+ messages) latest in thread
Message-ID: <234265385.16298751.1471930117094.JavaMail.yahoo@mail.yahoo.com>
Permalink: ../234265385.16298751.1471930117094.JavaMail.yahoo@mail.yahoo.com/
Also on: postgresql.org/message-id/234265385.16298751.1471930117094.JavaMail.yahoo@mail.yahoo.com
copy link · copy postgr.es
reply Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: pgsql-hackers@postgresql.org
Cc: sul_amul@yahoo.co.in, a.zakirov@postgrespro.ru, robertmhaas@gmail.com, pavel.stehule@gmail.com, tgl@sss.pgh.pa.us, david.g.johnston@gmail.com, a.ignatov@postgrespro.ru, bruce@momjian.us
Subject: Re: Bug in to_timestamp().
In-Reply-To: <234265385.16298751.1471930117094.JavaMail.yahoo@mail.yahoo.com>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox