Skip to content

V1.0 #301

Merged
merged 6 commits into from Apr 4, 2013

6 participants

@brianc
Owner
brianc commented Mar 11, 2013

This pull request introduces a few breaking changes.

1) Floats will be returned from node-postgres as JavaScript strings instead of being parsed.

Using parseFloat can lead to very tricky and hard to catch rounding & conversion errors within JavaScript. Data loss is the last thing node-postgres should do. If you'd like you can override the float type parsers yourself to either use parseFloat like v0.x or another JavaScript large number library which is better suited to handle larger numeric values.

2) The client pool callback now requires 3 parameters as outlined in the documentation here:

https://github.com/brianc/node-postgres/wiki/pg

if you install pg@1.0 or greater and you do not call the done() callback in pg.connect you will very quickly exhaust the client pool

3) Client#pauseDrain() and Client#resumeDrain() are removed because the pool will no longer automatically return clients when they emit the drain event. Please note the drain event will still be emitted, that did not change. Only now node-postgres itself will not respond to it. Since it doesn't respond to it there's no reason you need to stop it from emitting.

@strk
strk commented Mar 14, 2013

Will you create a 0.14 branch before merging this ?

@brianc
Owner
brianc commented Mar 14, 2013

yah I will, but I don't plan on maintaining a previous version or "break fix" branch except w/ critical fixes.

@strk
strk commented Mar 14, 2013
@brianc
Owner
brianc commented Mar 14, 2013

yah definitely will merge those today.

Also, the API breakage is pretty small. It's really just the pool requiring a 3rd parameter when checking out a client and parseFloat not being the default. You can definitely still over-ride the default type parsing and revert it back to using parseFloat if that worked in your apps. That should only take a few lines during app initialization to revert to old behavior.

As for pauseDrain and resumeDrain being removed....those were a travesty to begin with. 😊

@jmarca
jmarca commented Mar 22, 2013

Can you post an example of how to use another library to do the numeric conversion? I found https://github.com/alexbardas/bignumber.js, https://github.com/MikeMcl/bignumber.js, and https://github.com/justmoon/node-bignum.

One fix of course is to turn off parsing and get numbers as strings, then wire one of these libraries (or some other version) to convert numbers from string after they are returned. A cleaner solution would be to write a converter and pass it in somehow. That is what I am asking for documentation on how to do. I don't understand what the register stuff is doing here and I can't find an option setting to insert my own even if I could figure out what is going on there.

@spollack
spollack commented Apr 2, 2013

@brianc: i agree that an example of how to hook parseFloat or similar back up would be great.

related to this, since i am implicitly loading pg via gesundheit/any-db i'm going to need to figure out how to get that custom pg post-initialization code to run...

@brianc
Owner
brianc commented Apr 2, 2013
@brianc brianc merged commit 1d65417 into master Apr 4, 2013

1 check failed

Details default The Travis build could not complete due to an error
@strk
strk commented Apr 4, 2013

I'm also interested in the example about reverting to old behavior, it'll be important for everyone upgrading.

@laurentdebricon

I would like to turn on the old behavior

var pg = require('pg');
pg.defaults.parseFloat = true

It doesn't seem to work. Any ideas ?

@mren mren referenced this pull request in snd/mesa Apr 10, 2013
Closed

node-pg requires call to done() method #7

@strk
strk commented Apr 11, 2013

@laurentdebricon require('pg').types.setTypeParser() something (not documented on the wiki, it seems)

@strk
strk commented Apr 11, 2013

@brianc still I don't really understand the rationale for dropping support for numbers.
Beside you left support for parsing _float4 _float8 and _numeric (the array types) but drop the single (non-array) type...

@spollack

@brianc I agree with @strk that i don't understand the rationale for dropping support for numbers.

I would imagine that most users of the pg module are going to need to manipulate numeric data, and so having some facility for this (be that mapping to native types, or using a higher level abstraction, or both) built in to the library is important.

(btw, I totally get that postgres and javascript types do not map to each other perfectly. its not just potential loss of precision on floats; its also bigints that may overflow the javascript number, and i'm sure a variety of other examples.)

@brianc
Owner
brianc commented Apr 11, 2013
  1. for reference on why default to parsing floats was removed please see: #271 /cc @shtylman @freewil basically converting from postgres float (or double) to javascript float corrupts your data and you will be none-the-wiser. It's unacceptable as an out of the box default because of the size and impact that footgun has. If you yourself know you're dealing with floats parseFloat can handle you can "opt-in" to use the old behavior. (see point 3)

  2. Whoops, I missed the array parsers. I'll remove those soon.

  3. Today I'm going to build a module you can npm install and use to restore the old behavior to postgres. It will be called pg-parse-float. If you install this and require it anywhere it will patch into postgres and restore the old behavior. This module can also be used as a reference point for providing the various other 3rd party big-number library implementations.

  4. It's easier to detect when ints are going to be stomped by postgres and it's also a lot less common, so that default wasn't changed to strings. I think technically it would be nice to return everything as either a string or null with type information and have other modules includable to provide custom parsers, but that would be horrifically large change at this point, and while "cleaner" wouldn't be more usable. The parseFloat had to go because it was actually broken. If you insert a value and get a different value back...what good at all is your datastore?

@strk
strk commented Apr 11, 2013
@spollack

@brianc thank you for the detailed response, much appreciated. and definitely looking forward to the pg-parse-float module.

sorry i'm being slow here, but i'm still missing something here as to why this is a giant footgun (great visual image image btw ;-). I ran the test case (https://gist.github.com/shtylman/4757910) referenced in #271, and the results i get back (node 0.8.19) are:
1.79769313486231579
1.7976931348623157
Which doesn't strike me as horrific data loss. particularly if the postgres datatype was real/float4, which only has 6 digits of precision.

@brianc
Owner
brianc commented Apr 11, 2013

You can actually put float values in as strings due to the way the postgres protocol works:

https://github.com/brianc/node-pg-parse-float/blob/master/test.js#L17

@strk it's easier to detect stomped ints because there is Number.MAX_VALUE...except this code has been removed...it used to be here: https://github.com/brianc/node-postgres/blob/master/lib/types/textParsers.js#L159 I should probably add that back in. It would just spit out a warning to the console.

@laurentdebricon et al - I created this module you should be able to use to restore the old behavior in 1 line of code. You can also use it as a reference for making other custom type parsers (i.e. JSON)

https://github.com/brianc/node-pg-parse-float

@defunctzombie
@defunctzombie
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.