Re: psycopg2: parsing timestamps with odd time zones

On Wed, May 19, 2010 at 4:04 PM, Karsten Hilbert
<[email protected]> wrote:
> On Wed, May 19, 2010 at 03:41:53PM +0100, Daniele Varrazzo wrote:

>> I don't know exactly the type of strings TSTZ_W_SECS expects (and the
>> object it returns): it would be nice to have a few of them in the test
>> suite, both for testing and documentation.
>
> If you happen to have a locale which contains seconds within
> the timezone definition you will find that Python cannot
> deal with that. If this typecaster is registered those
> seconds will be truncated away and the original DATETIME
> typecaster is used again on that.
>
> This makes the result wrong, though, by up to 59 seconds,
> unless the seconds where "00".
>
>> What stops us to have a default typecaster doing the right thing in
>> first place?
>
> It does, it's just that Python cannot deal with the right thing.

Yes, I've read the imitation in the docs ("the value returned must be
a timedelta object specifying a whole number of minutes in the range
-1439 to 1439 inclusive"), so it seems these timezones can't be
treated by Python specs. I've also verified that pytz deals with the
issue by rounding (not truncating) to the minute
(http://pytz.sourceforge.net/#issues-limitations).

I've also checked that it's us raising the exception in
typecast_PYDATETIME_cast, not the python library.

So what we do is to raise an error where we can't make the things with
the maximum precision. Thus the workflow for using psycopg is
something like:

1. an user starts using the library

2. everything works file

3. at a certain point, her program deals with an exotic input, such as
the 1930 India trains time table. We raise an exception because we
can't deal with the seconds in the tz, and we would make an error of
20 seconds

4. the diligent user realizes that she has to register TSTZ_W_SECS,
because the doc says "If you encounter 'unable to parse time' on a
perfectly valid timestamp you likely want to try this type caster"

5. when the condition in 3 happens again, we don't raise any
exception, but we make an error of 20 seconds.

So: why don't we do the error in first place, by rounding to the
nearest minute in typecast_PYDATETIME_cast, saving the user the hassle
of doing 4. and to incur in the exception at all, maybe in production
instead of in testing? I'd like to do the best thing, but if pytz
doesn't use seconds in the tz either, I guess we can't do more. We
could round instead of truncate, so the error would be +/- 30 secs
instead of 60 and we'd be consistent with pytz.

> As for testing try setting your timezone to Asia/Colombo or
> Asia/Calcutta and go back in time far enough.

Yup, thanks. I was also having some tests with:

test=> set timezone='Asia/Calcutta';
SET
test=> select now() - '80 years'::interval;
              ?column?
------------------------------------
 1930-05-19 20:27:34.46063+05:53:20
(1 row)

that gives a sample of the problematic tz.

So, I'm generally against introducing silent errors, but:

1. because Python can't do better, and this is acknowledged by the
best Python tz library around, and
2. because the current solution is to introduce the error anyway, just
*after* the user has been bitten by the exception at least once,
possibly in an inappropriate moment,

I am for fixing typecast_PYDATETIME_cast() to round the tz to the
minute in first place and make register_tstz_w_secs() a no-op
function.

If this suggestion is rejected, I propose to raise a ValueError
subclass (e.g. TzWithSecondsError) so that TSTZ_W_SECS could deal with
it without having to parse the error message. Because we raise the
exception after having parsed the timestamp, we could store the
seconds off in the exceptions, so that TSTZ_W_SECS wouldn't need
re-parsing and could easily round the tz to the the nearest minute.

Comments?

-- Daniele