Re: [PATCHES] to_date() validation

On Fri, Aug 29, 2008 at 7:39 PM, Brendan Jurd <direvus@gmail.com> wrote:
> Hi all,
>
> Well, it's been a long time coming, but I've finally got a patch to
> improve the error handling of to_date() and to_timestamp(), as
> previously discussed on -hackers. [1][2]

BTW -patches is obsolete just submit pathces to -hackers.

Im just following this:
http://wiki.postgresql.org/wiki/Reviewing_a_Patch so lets get started.

Submission review: Everything looks good. Tests seem reasonable patch
applies etc.

Usability review: I think both linked threads in the parent mail give
sufficient justification.

Feature test: Everything seems to work as advertised. However before
via sscanf() most limited the max length of the input. Before they
were just silently ignored now they get added to the result:

patched:
# SELECT to_timestamp('11111', 'HHMM');
to_timestamp
------------------------------
0009-03-19 11:00:00-06:59:56

8.3.3:
# SELECT to_timestamp('11111', 'HHMM');
to_timestamp
---------------------------
0001-11-01 11:00:00-07 BC

Performance review: simple pgbench of to_timestamp did not show any
noticeable differences

Coding review: seemed fine to me, code matched surrounding style,
comments made sense etc

Code review: one minor nit
*** a/src/backend/utils/adt/formatting.c
--- b/src/backend/utils/adt/formatting.c
***************
*** 781,787 **** static const KeyWord DCH_keywords[] = {
{"y", 1, DCH_Y, TRUE, FROM_CHAR_DATE_GREGORIAN},

/* last */
! {NULL, 0, 0, 0}
};

/* ----------
--- 781,787 ----
{"y", 1, DCH_Y, TRUE, FROM_CHAR_DATE_GREGORIAN},

/* last */
! {NULL, 0, 0, 0, 0}
};

/* ----------
***************

--
Sent via pgsql-patches mailing list (pgsql-patches@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-patches