Re: Bug in to_timestamp(). - Mailing list pgsql-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
incorrectoutput 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
tothe patch and eventually extra burden to review the code. Similarly hunk in the patch @ line # 313 - 410, nothing to
dowith 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