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
