to_date() validation

From: "Brendan Jurd" <direvus(at)gmail(dot)com>
To: pgsql-patches(at)postgresql(dot)org
Subject: to_date() validation
Date: 2008-08-30 01:39:04
Message-ID: 37ed240d0808291839t21e19956mdfbdc80aeb1b3c19@mail.gmail.com
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: pgsql-hackers pgsql-patches

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]

The patch is against HEAD, and compiles cleanly and passes all
regression tests on gentoo amd64.

I did some testing to see whether the extra error checks had an
adverse effect on performance. It turns out that my patch actually
performs materially *better*, somehow. About 14.5% better over 1M
calls to to_date in a single statement. Go figure (C compiler
optimisations are completely arcane to me).

What the patch does
====

With this patch I've tried to make DCH_from_char a bit smarter with
regard to parsing and validation of the user input. The goal here is
that if the user makes a mistake and enters something invalid,
to_date() will throw an meaningful error instead of just acting like
everything is okay and then producing an absurd answer.

I've modified the code to error out usefully in a few different scenarios:

* Non-integer value for an integer field
* Out-of-range value for an integer field
* Not enough characters in the input string to provide for all the
format fields
* Two conflicting values given for a field
* Illegal mixture of date conventions (ISO week date and Gregorian)

I've included new regression tests to check that these errors are
occurring when appropriate.

How I went about it
====

In order to detect an illegal mixture of Gregorian and ISO week date
conventions, the code needs to know which fomatting fields are
Gregorian, which are ISO WD and which are date-convention-neutral. To
get this working I added a new enum called FromCharDateMode, and added
a field to both KeyWord and TmFromChar to hold the date mode. In
KeyWord, it tells you which convention the keyword belongs to (YYYY is
Gregorian, IYYY is ISO WD and HH24 has nothing to do with dates). In
TmFromChar it keeps track of which convention is currently in use.

Because TmFromChar now tracks the date mode independently, it is not
necessary to maintain separate fields for the ISO WD values, so
'iyear' and 'iw' are removed.

In order to get some consistent treatment on pulling integers out of a
string and stuffing them into a TmFromChar, I had to refactor
DCH_from_char somewhat. The function is basically a giant switch
statement, with branches for each of the formatting fields. In HEAD
these branches are highly duplicative. The integer fields all perform
a sscanf() on the source string, putting the result straight into the
TmFromChar struct, and then skipping over the number of characters
consumed by sscanf(). I moved this logic into functions:

* from_char_parse_int() uses strtol() rather than sscanf() to parse
an integer from the string, and produces meaningful errors if
something is wrong with the input,
* from_char_set_int() saves an integer into a TmFromChar field, but
throws an error if the field has already been set to some other
non-zero value.

What the patch doesn't do
====

It doesn't consider to_number() at all. I don't have any interest in
the number formatting stuff.

There is plenty of room to educate do_to_timestamp() further about
dates; it's still very naive. Currently you can do something like
to_date('2008-50', 'YYYY-MM') and it won't even bat an eyelid. It'll
just advance the date by 50 months. I suppose you could consider that
a bug or a feature, depending on your point of view. Personally, I
think of it as a bug. If you're giving a month value outside of 1-12
to to_date(), chances are good that it's a user error in the query,
not a deliberate attempt to perform some date arithmetic.

do_to_timestamp() also doesn't try to detect logical mistakes like
'YYYY-MM-DD DDD' where the DDD part conflicts with the MM-DD part. I
have some thoughts about how I would improve on that, but I think it's
best left for a separate patch.

The patch has been added to the September commitfest. Thanks for your time.

Cheers,
BJ

[1] http://archives.postgresql.org/pgsql-hackers/2007-02/msg00915.php
[2] http://archives.postgresql.org/message-id/37ed240d0707170747p4f5c26ffx63fff2b5750c62e5@mail.gmail.com

Attachment Content-Type Size
to-date-validation-1.diff.gz application/x-gzip 9.4 KB

Responses

Browse pgsql-hackers by date

  From Date Subject
Next Message Tom Lane 2008-08-30 02:53:18 Re: Is it really such a good thing for newNode() to be a macro?
Previous Message Alex Hunsaker 2008-08-29 22:46:58 Re: Is it really such a good thing for newNode() to be a macro?

Browse pgsql-patches by date

  From Date Subject
Next Message David Rowley 2008-08-30 23:21:11 TODO item: Implement Boyer-Moore searching (First time hacker)
Previous Message Karl Schnaitter 2008-08-29 22:00:04 fixing bug in combocid.c