Lighthouse has a new layout. Prefer the old one? Return to the old layout, and switch back any time from the link at the top of each page.

This project is archived and is in readonly mode.

[PATCH] ActiveRecord timestamp conversions fail for some cases

#3693

The microsecond handling in ActiveRecord::ConnectionAdapters::Column#fast_string_to_time and ActiveRecord::ConnectionAdapters::Column#microseconds fail for some values.

In slightly more than 1% of all possible 6-digit cases, writing a timestamp to a database column and then reading it back in results in a different value being returned to the program.

So, for instance, saving the timestamp

2010-01-12 12:34:56.125014

and then loading it again from the database yields

2010-01-12 12:34:56.125013

The problem occurs when the value read is converted from string form to a Ruby timestamp, so it is largely database independent (the exception being drivers that override the methods or databases that don't support timestamps at all).

The underlying problem is the use of to_i to convert from floats to ints inside the affected methods. As you know, to_i simply truncates the result and in some cases this causes rounding errors introduced by inherent inaccuracies in the multiplication operations and decimal representation to bubble up and affect the least significant digit.

Here's a simple test that illustrates the problem:

converted = ActiveRecord::ConnectionAdapters::Column.send('fast_string_to_time', "2010-01-12 12:34:56.125014")
    assert_equal 125014, converted.usec

This test case (and a similar one for #microseconds) fail on plain vanilla Rails 2.3.5.

I guess the best solution would be to change the ISO_DATETIME regex used to extract the microsecond-part from timestamps to not include the decimal point at all and then avoid the to_f and subsequent floating point multiplication completely inside the failing methods. However, these regexes are defined as constants on ActiveRecord::ConnectionAdapters::Column::Format and therefore publicly available, so the impact of changing these is difficult to ascertain.

A simpler solution is to use round() instead of to_i to convert from the intermediate floating point result to int. This works (I have verified that the precision is sufficient for all possible 6-digit cases) but is about 15% slower than the current method. A small price to pay for correctness, in my opinion.

I have attached a tiny patch (against 2.3.5) that switches the code to using round() and a test case that verifies that the method works for a few problematic cases that fail without the patch.

Reported by Jacob Lauemøller · January 14th, 2010 @ 03:16 PM

State: committed
Milestone: none
Assigned to: Pratik Pratik
Importance: none

Activity

  1. Chris Hapgood
    Chris Hapgood

    I spent an entirely inappropriate amount of time digging into this issue...

    Here are some observations:

    The #microseconds method LOOKS clumsy, but it's actually quite optimized. Turns out the the sec_fraction component is a Rational less than one. So.... converting as quickly as possible to something other than Rational (Float, in this case) is actually the right thing to do. I tried dozens of alternatives and benchmarked them all and could not beat the current implementation -even after changing the #to_i to #round. Arguably, the modulo division step is superfluous -but only if you always pass in rationals less than one.

    On the other hand, The regex parsing of time strings in #fast_string_to_time is both clunky and incorrect for some values (as noted and fixed by Jacob). I've taken the first approach hinted at by Jacob and used a more direct regex to return only integers. The result is a method that is faster (roughly 5%), accurate (no more truncation issues) and arguably cleaner (no more double conversion). It should be completely backwards compatible as well -I've defined a new constant.

    The benchmarks are here: http://gist.github.com/278185

    I've attached a new patch with extensive tests.

    January 15th, 2010 @ 11:53 PM

  2. Michael Koziarski
    Michael Koziarski
    • State changed from new to committed

    Applied to master in 717a294

    January 16th, 2010 @ 02:23 AM

  3. Levent
    Levent

    Pratik seemed to have reverted this commit with 0ab3063 so I guess the issue still stands in 2.3.x and 3.x

    commit 0ab30637dd5bc7536c5accd66b45ce0263134a14
    Author: Pratik Naik <pratiknaik@gmail.com>
    Date:   Sun Jan 17 03:04:11 2010 +0530
    
        Revert "Fix #microseconds conversion and #fast_string_to_time"
        
        This reverts commit 717a2941e15b32d07cc456bb0d81742ecfc5b4a3. Bunch of failures when running postgresql tests.
    

    September 13th, 2010 @ 01:30 PM

  4. Chris Hapgood
    Chris Hapgood
    • Assigned user set to Pratik

    Pratik, do you have any insight as to why this failed on postgresql?

    October 11th, 2010 @ 06:16 PM

  5. Andrea Campi
    Andrea Campi
    • Tag changed from rails 2.3.5 timestamp bug patch to 2.3.5, patch, timestamp

    October 16th, 2010 @ 11:57 PM