Fix occasional microsecond conversion inaccuracy #7352

Merged
merged 1 commit into from Aug 17, 2012

Conversation

Projects
None yet
7 participants
Contributor

aripollak commented Aug 14, 2012

This patch was originally submitted by Logan Bowers in https://rails.lighthouseapp.com/projects/8994/tickets/4498-rails-loses-precision-when-deserializing-timestamps-from-postgresql, but I've shortened the test case significantly and I can reliably reproduce it.

In psql, run this:

CREATE TABLE tmps (id serial primary key, atime timestamp with time zone );

Then open up rails console and run this:

class Tmp < ActiveRecord::Base; end

time = Time.at(1344803062, 129346).utc
t = Tmp.create!({atime: time}, without_protection: true)
puts "Original time: " + time.strftime('%s.%N')
puts "Retrieved time: " + t.reload.atime.strftime('%s.%N')
puts "Raw time from DB: " + t.atime_before_type_cast

I get the following results without the patch:

1.9.3p194 :005 > class Tmp < ActiveRecord::Base; end
1.9.3p194 :007 >   time = Time.at(1344803062, 129346).utc
 => 2012-08-12 20:24:22 UTC 
1.9.3p194 :008 > t = Tmp.create!({atime: time}, without_protection: true)
   (0.1ms)  BEGIN
  SQL (0.8ms)  INSERT INTO "tmps" ("atime") VALUES ('2012-08-12 20:24:22.129346') RETURNING "id"
   (2.6ms)  COMMIT
 => #<Tmp id: 3, atime: "2012-08-12 20:24:22"> 
1.9.3p194 :009 > puts "Original time: " + time.strftime('%s.%N')
Original time: 1344803062.129346000
1.9.3p194 :010 > puts "Retrieved time: " + t.reload.atime.strftime('%s.%N')
Retrieved time: 1344788662.129345000
1.9.3p194 :011 > puts "Raw time from DB: " + t.atime_before_type_cast
Raw time from DB: 2012-08-12 20:24:22.129346+00

Note that the DB says there are .129346 seconds, but Ruby says it's .129345 instead. With the patch, the times are all in sync.
This diff should also apply cleanly to 3.2.

CC: @tenderlove, @rafaelfranca

Member

sikachu commented Aug 14, 2012

/cc @tenderlove @jonleighton. Can you guys review this patch?

jonleighton was assigned Aug 14, 2012

Contributor

aripollak commented Aug 15, 2012

Also, in case it wasn't obvious, the ramifications of this bug are rather important:

1.9.3-p194 :006 > time == t.atime
 => false 
1.9.3-p194 :011 > Tmp.where('atime <= ?', t.atime)
 => [] 
Member

steveklabnik commented Aug 15, 2012

This already needs a rebase :/

@aripollak aripollak Fix occasional microsecond conversion inaccuracy
ActiveRecord::ConnectionAdapters::Column#microseconds did an unnecessary
conversion to from Rational to float when calculating the integer number
of microseconds. Some terminating decimal numbers in base10 are
repeating decimal numbers in base2 (the format of float), and
occasionally this causes a rounding error.
Patch & explanation originally from Logan Bowers.
53ca22f
Contributor

aripollak commented Aug 15, 2012

Rebased. If it's easier, I can just rip out the changelog modification.

Owner

rafaelfranca commented Aug 15, 2012

Seems great!

@aripollak do you think this is related with #6986, #6975 and #7119?

Contributor

aripollak commented Aug 15, 2012

Thanks!
As far as I can tell, those issues actually seem to be dealing with the
opposite problem of MySQL not saving microseconds.
On Aug 15, 2012 6:24 PM, "Rafael Mendonça França" notifications@github.com
wrote:

Seems great!

@aripollak https://github.com/aripollak do you think this is related
with #6986 #6986, #6975https://github.com/rails/rails/issues/6975and
#7119 #7119?


Reply to this email directly or view it on GitHubhttps://github.com/rails/rails/pull/7352#issuecomment-7771229.

Owner

rafaelfranca commented Aug 15, 2012

Right! Thank you for the explanation.

@rafaelfranca rafaelfranca added a commit that referenced this pull request Aug 17, 2012

@rafaelfranca rafaelfranca Merge pull request #7352 from aripollak/microsecond-timestamp
Fix occasional microsecond conversion inaccuracy
8f4ee48

@rafaelfranca rafaelfranca merged commit 8f4ee48 into rails:master Aug 17, 2012

@rafaelfranca rafaelfranca added a commit that referenced this pull request Aug 17, 2012

@rafaelfranca rafaelfranca Merge pull request #7352 from aripollak/microsecond-timestamp
Fix occasional microsecond conversion inaccuracy
Conflicts:
	activerecord/CHANGELOG.md
d6dbd7f

Possibly, is this not fast_string_to_date but fast_string_to_time?

You're right, fixed in d23c761, thanks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment