-
-
Notifications
You must be signed in to change notification settings - Fork 1.3k
Rewrite native bindings #657
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This completes the port from the old native bindings to the new node-pg-native bindings! Time to build in support for older versions of postgres & start the pull request process.
Conflicts: lib/native/index.js package.json
Fixes a case where you connect & immediatley disconnect and _sometimes_ your notification subscription starts the reader on the libpq instance after the libpq instance is disconnected. This segfaults the app.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The native bindings needed a full rewrite. They were complex, hard to maintain, and slow.
I rewrote the core of the native bindings from scratch into a module binding to the C libpq api called libpq. On top of this I've built a modernized, higher-level API to node-libpq called pg-native. Now
pg
consumes pg-native which in turn consumes libpq. This provides a much more modularized approach to things and one I plan to continue going forward.why you should care:
pg.js
instead. This problem will be going away by making the native bindings an optional add-on.before merging:
I will use some sort of npm
beta
tag for the new version of node-postgres for a while.If you have non-mission-critical software I'd really love it if you could test out node-pg-native and help me shake out bugs over there.
I need to benchmark the new native bindings. Initial benchmarks show them to be >50% faster than the current version.
breaking changes:
COPY TO / COPY FROM
streaming support has been removed. This feature was buggy within node-postgres itself and has been replaced by the much more battle tested pg-copy-streams module. pg-native also provides native copy streams through libpq. I also plan on doing more experimentation with low-level copy operations within pg-pipe which allows me to use libpq to do some nice zero-copy buffer operations in C++.The native bindings will not support multiple statements returning different result sets in the same query. Only the last query in each command will return results. Doing 'pipelining' of multiple select statements is bad practice and not supported by the PostgreSQL backend when using prepared or parameterized queries. Multiple commands still work - and like today you will only have access to the results from the last command in the series. If you want to do something like this it wont work - use async & send two different queries:
singleRowMode
in libpq is not yet supported. It only works on PostgreSQL 9.3 anyways. If you want to stream a large batch of query results without exploding your memory space it is highly recommended you use pg-cursor or pg-query-stream. There is really no good way to support streaming rows from libpq...evensingleRowMode
has no concept of back pressure.future plans:
The documentation is becoming a bit of a mess. I need to sit down and spend some time cleaning it up and modernizing it. Thanks for your patience when you wade through it now. 😄
I want to break out the JavaScript connection class into its own module. I want to build a new JavaScript higher level module to consume the connection in the same way node-pg-native consumes node-libpq.
I plan on making the native bindings an optional install instead of installing them by default. Much in the way node-redis uses hiredis if it is installed. This will cut down on a lot of support issues I have where someone does not have access to a compiler or PostgreSQL headers and so their installation fails even though they have a perfectly usable, production-grade client installed in pure node.js.
Long term I see node-postgres being sort of a meta-package that glues together a bunch of smaller modules and provides the API it always has -- warts and all -- for maximum backwards compatibility. I know how annoying it is to have a boring, core piece of your infrastructure modules breaking compatibility for less than critical reasons & leaving you faced with the choice of either a painful, error-prone upgrade to get new functionality, or leaving you stuck on an old version for a long time. I have and will always strive to keep breaking changes to an absolute minimum. The smaller modules can be used when only some of the features are required, and their APIs can be simpler to learn and easier to evolve.