Date: Tue, 23 Aug 2016 05:28:37 +0000 (UTC)
From: amul sul <sul_amul@yahoo.co.in>
Reply-To: amul sul <sul_amul@yahoo.co.in>
To: Artur Zakirov <a.zakirov@postgrespro.ru>,
	Robert Haas <robertmhaas@gmail.com>
Cc: Pavel Stehule <pavel.stehule@gmail.com>, Tom Lane <tgl@sss.pgh.pa.us>,
	"David G. Johnston" <david.g.johnston@gmail.com>,
	Alex Ignatov <a.ignatov@postgrespro.ru>,
	Bruce Momjian <bruce@momjian.us>,
	PostgreSQL-development <pgsql-hackers@postgresql.org>
Message-ID: <234265385.16298751.1471930117094.JavaMail.yahoo@mail.yahoo.com>
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>
Subject: Re: Bug in to_timestamp().
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit
Content-Length: 2728
Precedence: bulk
Sender: pgsql-hackers-owner@postgresql.org

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