brianc / node-pg-types

Type parsing for node-postgres
268 stars 55 forks source link

Question: performance impact of types.arrayParser.create vs direct postgres-array.parse #73

Closed jeremychone closed 5 years ago

jeremychone commented 6 years ago

(sorry if this is the wrong venue to ask this question).

First, I am very pleased with the quality and extensibility of node-pg and the suite (coming from Java world).

Now, while writing some custom type parsers for Array, I understood that the way to do override an array type parser is like this:

Option 1)

// _int8: 1016
types.setTypeParser(1016, function (val: string) {
  return types.arrayParser.create(val, parseInt).parse();
});

Now the thing which confuses me is that types.arrayParser.create returns a new function/"scope" per record cells, which seems to be a potential performance issue. Now, there might be some JS optimization that makes it a non-issue, but I would like to have confirmation.

The other option would be to use the postgres-array directly.

Option 2)

import * as postgresArray from 'postgres-array';

types.setTypeParser(1016, function (val: string) {
  return postgresArray.parse(val); 
});

Is there any performance disadvantage of option 1 over 2, or is the way JS/V8 optimized, the new function/"scope" created per record in option 1 is not really an issue?

charmander commented 6 years ago

It’s unlikely to be a performance issue, but I’m not sure why types.arrayParser.create exists at all. (Backwards compatibility?) postgresArray.parse(val, parseInt) with a dependency on postgres-array is fine.

jeremychone commented 6 years ago

Not sure, it was exported in the "@types/pg": "^7.4.10" so I assumed it was the way to do it (and I had to do some digging). Somehow, creating a function per value parsing felt off, but I agree hard to know if it is a performance issue until it is.

But great, I will use the postgresArray.parse, feels more common.

On a related note, would be great if the pg module could export postgresArray and that the parse be in the @types/pg. Right now there is no @types/postgres-array, not deal breaker, just a little cumbersome to use.

Thanks for the fast response, and for this high quality lib. node-postgres's quality has been critical in our decision to move our "big app" dev to node from Java.

bendrucker commented 6 years ago

I've refactored this library a bit over time. Pulled out some of the larger concepts (e.g. arrays, dates) into their own module. But generally kept the API the same.

You should feel free to depend on postgres-array, that's what I was hoping for when publishing it separately (easier to build custom behavior).

As for performance, I'm hesitant to start picking at anything without benchmarks. You're probably right in this case but it's hard to say what the magnitude of impact would be. I'm open to working on that across the node/pg toolchain at some point.

jeremychone commented 6 years ago

Cool thanks. If you want I and contribute the @types/postgres-array to npm or even better, do a pull request on postgres-array with the type definition. Does not change anything for non typescript users, but removes quite a bit of pain for TS user.

bendrucker commented 6 years ago

do a pull request on postgres-array with the type definition

Please do!

jeremychone commented 6 years ago

@bendrucker Just did the pull request, sorry for the delay.