Skip to content
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

using pool and native #1061

Merged
merged 2 commits into from
Jun 24, 2016
Merged

using pool and native #1061

merged 2 commits into from
Jun 24, 2016

Conversation

brianc
Copy link
Owner

@brianc brianc commented Jun 24, 2016

Hi, the syntax for using pg native is:

require('pg').native.

What if I want to use this with

require('pg').Pool;

is there a way to have Pool work in native ?

@vitaly-t
Copy link
Contributor

Have you tried? 😄

@Brakkar
Copy link
Author

Brakkar commented Jun 23, 2016

let Pg = require('pg').Pool.native ?

@vitaly-t
Copy link
Contributor

let Pg = require('pg').native.Pool

@Brakkar
Copy link
Author

Brakkar commented Jun 23, 2016

Was close, thanks

@Brakkar Brakkar closed this Jun 23, 2016
@brianc
Copy link
Owner

brianc commented Jun 23, 2016

Whoah I am surprised that worked actually! Haha. I am gonna need to make that API nicer or at least documented so I'm gonna reopen this until I get it documented right. 😊

@brianc brianc reopened this Jun 23, 2016
@vitaly-t
Copy link
Contributor

A suggestion, it is quite important for developers to understand that when switching over to the new pool, and continue being able to restore a database connection they have to change from pg.on('error', cb) to pool.on('error', cb), or else it will break the app when the server disconnects.

@brianc brianc force-pushed the 1061-using-pool-and-native branch from 452ff5d to b1642f5 Compare June 24, 2016 05:45
@brianc
Copy link
Owner

brianc commented Jun 24, 2016

@Brakkar Your question actually helped me uncover a bug with the pool. I also spent some time updating the readme to make it clearer how to consume the pool, both here as well as on pg-pool itself! Thanks!

@vitaly-t you're right about that...I'll add some more documentation on error handling to pg-pool!

@brianc brianc merged commit 812277f into master Jun 24, 2016
@vitaly-t
Copy link
Contributor

@brianc just so since you seem to have some time now to improve your libraries, I wanted to point at things that would make a very worthwhile improvement...

@Brakkar
Copy link
Author

Brakkar commented Jun 24, 2016

@brianc thanks for update, documentation is much better indeed. Glad I could help.

I put var pool = new pg.Pool(config); in exported function like this:

export let processSqlPgQuery = function( sqlQuery: string, theCallBack ) { 
    let dbPool = new pg.Pool( settings.dbConfig )  // <- HERE 
    dbPool.connect( function( err, client, done ) {
        if ( err ) {
            return console.error( 'error fetching client from pool', err );
        }
        client.query( sqlQuery, function( err, result ) {
            done()
            if ( err ) {
                return console.error( 'error running query', err );
            }
            theCallBack( undefined, result )  // Execution du callback
        })
    })
}

Is this bad practice ? Will pool get reinitialized each time processSqlPgQuery is called ? Should I put let dbPool = new pg.Pool( settings.dbConfig ) in top of the file ?

@brianc
Copy link
Owner

brianc commented Jun 24, 2016

@Brakkar - yeah that's bad practice. If you instantiate more than 1 pool during the lifetime of your application you're likely leaking resources & creating an unbounded number of clients to postgres.

I added some instructions around that here https://github.com/brianc/node-pg-pool#a-note-on-instances

@brianc
Copy link
Owner

brianc commented Jun 24, 2016

@vitaly-t pg-query-stream will likely never support native bindings. libpq doesn't expose streaming primitives in a way that libuv or node can easily consume, and the questionable level of performance gain which may or may not present itself is definitely not worth the massive amount of time it will take. If you wanna use query streams, you gotta use the JavaScript implementation.

As far as making native bindings have the same Result and Notification interface - I'm happy to accept a tested pull request for either of those things - I provided the main framework & tools to enable native bindings, but small interface differences I'd like to see the community fix if possible, they're not high on my list of priorities. 😄

@Brakkar
Copy link
Author

Brakkar commented Jun 24, 2016

Thanks Bryan.
Just to be sure:
If I put in de db module global space:

const dbPool = new pg.Pool( settings.dbConfig )

  1. even if I import this module file in multiple module, this will not reinitialize the pool each time the file is imported: the object is created only on 1st file load ?

  2. on those other file I can access exported dbPool.connect(..... this will just connect to the pool and not reinitialize it ?

@brianc
Copy link
Owner

brianc commented Jun 24, 2016

Correctamundo. Just make sure you don't call new pg.Pool more than once
& you are good to go!

On Fri, Jun 24, 2016 at 11:22 AM, Benj notifications@github.com wrote:

Thanks Bryan.
Just to be sure:
If I put in de db module global space:

const dbPool = new pg.Pool( settings.dbConfig )

  1. even if I import this module file in multiple module, this will not
    reinitialize the pool each time the file is imported: the object is created
    only on 1st file load.

  2. on those other file I can access exported dbPool.connect(..... this
    will just connect to the pool and not reinitialize it.


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#1061 (comment),
or mute the thread
https://github.com/notifications/unsubscribe/AADDoZt_LXiU7igWTSkO9jxwzundke4eks5qPAQ1gaJpZM4I8vHb
.

@vitaly-t
Copy link
Contributor

This gets awfully more complicated for any reusable library.

Where previously we could just use one global pool, now we have to figure out how to associate a pool with a database/connection config.

When a client wants to connect, it passes in a connection config. Now the library has to decide - do I need to create a new pool object for this config, or do I need to reuse an existing one?

I can only guess that perhaps it would have to parse the connection first (which previously wasn't required), and then look at such parameters as host, port, user, password, ssl?

For example, it shouldn't create multiple pools when those parameters are the same, but application_name name changes for different sub-modules.

As I said, it gets quite complicated. I have spent many hours at this point trying upgrade pg-promise to the new pool, but still running into problems, now with this reusable pool logic. Whenever I try to reuse an existing pool object, something goes wrong.

@vitaly-t
Copy link
Contributor

vitaly-t commented Jun 24, 2016

@brianc what we need now, is a pool factory, something that could take a connection string/object, and either return a new pool object or an existing one, plus an indication of whether it is a new pool.

The factory would parse the connection to figure out whether it represents a new physical connection to be created, or if there is already a pool that represents such a physical connection.

Perhaps the main reason why developers always wanted to have a separate pool is to have one pool per physical channel. But with the current library we are missing one important piece - to be able to tell when a connection string/object represents a unique physical channel. Yeah, we need a pool factory.

@brianc
Copy link
Owner

brianc commented Jun 24, 2016

With a pool factory we'd be in the same bad spot we've been in for years
with a global singleton being modified by different modules. The right way
forward is dependent modules such as pg-promise and ORMs need to accept an
instance of a pre-created and configured pool instead of a query string or
raw postgres config options. This way they declare their dependency on the
pool through their public interface and allow the consumer of the library
to configure their own pool and supply it to library.

Basically dependency injection instead of a globally mutateable singleton.

I realize this causes a public API change for other modules, but it's the
right way forward IMO for users and ultimately will make apps more
deterministic and easy to reason about.

On Friday, June 24, 2016, Vitaly Tomilov notifications@github.com wrote:

@brianc https://github.com/brianc what we need, is pool factory,
something that could take a connection string/object, and either return a
new pool object or an existing one.


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#1061 (comment),
or mute the thread
https://github.com/notifications/unsubscribe/AADDoW3rW4eLAVDAW-mTBeSdB-WMgM2Tks5qPAlNgaJpZM4I8vHb
.

@vitaly-t
Copy link
Contributor

But then where would you place the logic that can offer a separate pool per database server? This is what developers really want.

@brianc
Copy link
Owner

brianc commented Jun 24, 2016

I would imagine a developer having separate database servers would configure two pools:

var pool1 = new pg.Pool({
  database: 'db-1'
})

var pool2 = new pg.Pool({
  database: 'db-2'
})

var db1 = new PGPromise(pool1)
var db2 = new PGPromise(pool2)

Something like that - then they're able to bootstrap, configure, tweak
whatever each pool how they'd like, supply it to your lib, and then use the
two instances of pg-promise depending on which database they'd like to talk
to.

@brianc brianc deleted the 1061-using-pool-and-native branch June 8, 2017 03:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants