vitaly-t / pg-promise

PostgreSQL interface for Node.js
https://vitaly-t.github.io/pg-promise
MIT License
3.47k stars 217 forks source link

Question re: WARNING: Creating a duplicate database object for the same connection #175

Closed CalebEverett closed 3 years ago

CalebEverett commented 8 years ago

Is the right way to set this up in an Express app to create a separate db module per the example and then require that in each set of routes? I've done that, but am still getting the warnings. Do I need a separate database object for each set of routes or can you recommend otherwise? Thanks,

vitaly-t commented 8 years ago

You should create a single database object per connection details, and then reuse it.

From the start-up instructions:

You should create only one global/shared db object per connection details.

vitaly-t commented 8 years ago

See related questions:

Or the whole category: http://stackoverflow.com/questions/tagged/pg-promise

vitaly-t commented 8 years ago

Oh, and complete forgot: WARNING: Creating a duplicate database object for the same connection.

That should do it :smile:

CalebEverett commented 8 years ago

I was going off of those, but missed a module I was pulling into one of my route files. Thanks again

vitaly-t commented 8 years ago

@CalebEverett

See the changes brought with v.5.1.0

thenikso commented 7 years ago

I've a followup on this. I'm using pg-promise both in my app and on an integration test wrapper that initialize the test database to then import and call the app.

I get this warning which I believe it's not a problem and I can safely ignore it. However do you have any further best practice regarding this kind of scenarios?

vitaly-t commented 7 years ago

So, your intergration test and the application are using separate configurators for the database, while connecting to the same database. Is it really necessary? Why not place it in a shared module? That would get rid of the warning :wink:

Otherwise, in special cases you can disable all the warnings by using option noWarnings.

MitMaro commented 7 years ago

We have a distributed microservice like system, that sometimes runs several independant services within the same node process, with an unknown set of database connections. This meant that explicitly using a shared module wasn't a viable solution. Instead we ended up creating a factory that would use a hash of the configuration to return a cached instance. Looks something like this:

const pgPromise = require('pg-promise')(/*initialization options*/);
const cache = new Map();
module.exports = function  databaseConfig) {
    const dbKey = JSON.stringify(databaseConfig, Object.keys(databaseConfig).sort());
    if (cache.has(dbKey)) {
        return cache.get(dbKey);
    }
    const instance = pgPromise(databaseConfig);
    cache.set(dbKey, instance);
    return instance;
};

edit: Fixed a mistake I made while breaking out the example. Thanks @vitaly-t for pointing it out.

vitaly-t commented 7 years ago

@MitMaro is this module supposed to return an initialized pgp variable or a new db instance?

If it is the former, it is fine, if it is the latter - it won't work, because the library's initialization call is missing, i.e. var pgp = require('pg-promise')(/*initialization options*/), not just var pgp = require('pg-promise').

By the look of it is, it is the latter, that's why I asked.

And if you do have a special use-case, then as I commented earlier, you can use option noWarnings :wink:

var pgp = require('pg-promise')({
   noWarnings: true
});
var db = pgp(connectionDetails);
MitMaro commented 7 years ago

I made a mistake when I pulled out the example from the codebase. It's a database connection, not the pg-promise instance. Our actual code is a bit more complex. I've edited the example.

I was initially suppressing the warning but wanted to use a shared connection pool when possible. I didn't think to check to see if pg-promise reused an existing connection pool on the same connection parameters, but my guess is that it does not.

vitaly-t commented 7 years ago

pg-promise uses just one connection pool, as per node-postgres v5.1 used underneath.

MitMaro commented 7 years ago

Interesting. I think I found a bug in either pg-promise or node-postgres then. I will do up a test case and report it.

benoittgt commented 7 years ago

Hello

Thanks for pg-promise ! Love using it.

I have very simple code (that run on an AWS lambda) but I get THE warning :

undefined WARNING: Creating a duplicate database object for the same connection. at exports.handler (/var/task/index.js:20:14)

Here is my code :

'use strict';
const config = require('./config_from_env');
const connParams = `pg://${config.user}${config.password}@${config.host}/${config.database}`;
const pgp = require('pg-promise')();

const moveTravels = function(travelType) {
  return `
      BEGIN TRANSACTION ISOLATION LEVEL REPEATABLE READ;
      -- 2 query...
      COMMIT;`;
};

exports.handler = function(event, context) {
  const db = pgp(connParams);

  return db.tx(function (t) {
    return t.batch([
      t.none(moveTravels('arrivals')),
      t.none(moveTravels('departures'))
    ]);
  })
    .then(function () {
      return context.succeed(`Successfully moved`);
    })
    .catch(function (error) {
      return context.fail(`Failed to run queries : ${JSON.stringify(error)}`);
    });
};

Why I have the warning ? I know it's friday but I don't see duplicated database object.

vitaly-t commented 7 years ago

this is designed to be called more than once:

exports.handler = function(event, context) {
  const db = pgp(connParams);

which results in a repeated creation of the same db object more than once, hence the warning.

Also, on another note, your code shouldn't have things like:

  return `
      BEGIN TRANSACTION ISOLATION LEVEL REPEATABLE READ;
      -- 2 query...
      COMMIT;`;

The library can do such things automatically, while guaranteeing correctness.

benoittgt commented 7 years ago

Thanks a lot with your comment I just discover : http://vitaly-t.github.io/pg-promise/txMode.TransactionMode.html

this is designed to be called more than once: But this handler is called only once (in my case every 4 min)

Thanks again

vitaly-t commented 7 years ago

in my case every 4 min

that's not only once :)

vitaly-t commented 7 years ago

@benoittgt see this: http://stackoverflow.com/questions/34382796/where-should-i-initialize-pg-promise

vitaly-t commented 7 years ago

@lakesare what is this about?

RomeHein commented 6 years ago

Hello! I'm currently trying to implement authentification on my API. I was wondering how could I manage different user connections to the db? This is how I currently create the connection:

const pgp = require('pg-promise')();

let instance = null;

module.exports = class Connector {

  constructor(user,psw) {

    if (!instance || (user && psw)) {
      instance = this;

      this.config = {
        user: user || process.env.boUser,
        database:process.env.boDatabase, 
        password: psw || process.env.boUserPassword, 
        host:process.env.boHost, 
        port:process.env.boPort,
      };

      this.db = pgp(this.config);
    }

    return instance;
  }

}

And I use it like so (Connector is a global variable):

module.exports = class Pack {
    ...
    static all() {
        return Connector.db.tx(t => {
            return t.batch([t.manyOrNone(sql.pack.findAll)]);
        })
        .then(data => {
            if (data[0].constructor === Array && data[0].length > 0) {
                return Pack.parseArray(data[0]);
            }
            return null;
        });
    }
}

I'm a bit lost and don't get how I can create multiple connections. Let's say my API receive 10 requests from 10 different users, with different access rights on the db, how can I manage that on the API side? I feel like my pattern is not right.

vitaly-t commented 6 years ago

@RomeHein You would have to create and keep a separate db object, but that's not a great idea, because if it is the same server, the valuable IO connections would be consumed, and not reused across users.

RomeHein commented 6 years ago

Ok thanks for your quick answer! I'll move the authentification logic to the API, and keep only one user to perform the IO connections to the db. Thx again

oliversalzburg commented 5 years ago

@vitaly-t Is there a way to suppress the warning?

I'm encountering this during automated testing. I have a primary connection in application code and I'm establishing another connection with identical parameters in my test code. I manipulate the database through the test connection to set up state, then run my test which causes application code to mutate the database state through the primary connection, then assert state through the test connection.

I explicitly do not want to share the database connection between application and test context. I have no particular reason for this other than trying to keep both contexts separate from each other. I felt like that is just cleaner and a better approach, because I don't like sharing entities between test and business contexts during testing.

Please let me know your thoughts on this. Thanks

vitaly-t commented 5 years ago

@oliversalzburg Initialization option noWarnings can switch the warning off.

ild-dsv commented 5 years ago

Hi @vitaly-t,

After reading through pg-promise's official document and your explanations on StackOverflow as well, when we're working with pg-promise with Stack:

My approach: Only create data object once then import to lambda function when interactive with database (Our database is using PostgreSQL)

How to reproduce: utils/db.js

import pgPromise from 'pg-promise'
import Promise from 'bluebird'
import pgPromise from 'pg-promise'
const initOptions = {
  promiseLib: Promise,
  // noWarnings: true,
  capSQL: true,
}
const pgp = pgPromise(initOptions)

const dbObject = pgp(process.env.DATABASE_URL)

function disconnect(db) {
  return db ? db.$pool.end() : null
}

export { dbObject, disconnect }

lambaFunc.js

import { dbObject, disconnect } from './utils/db'

export const handler = async (event, context) => {
  if (event.source === 'serverless-plugin-warmup') {
    return 'Lambda is warm!'
  }

  const dbClient = dbObject

  const response = {
    statusCode: 200,
  }
 // we can do something with dbClient, however, even I don't do anything, just do like this, it also throws the warning
  disconnect(dbClient)

  return response
}

Do you have any idea about this?

Is it possible because of serverless-webpack?

vitaly-t commented 5 years ago

@ild-dsv All I can say is that somehow, somewhere you are creating a duplicate database object. I do not personally use Lambda, but I hear there is a lot of connection-related problems with it.

You can always debug it yourself - hack the library code and make it throw an error with stack upon a second call, to show where the second call is coming from.

ild-dsv commented 5 years ago

@ild-dsv, thanks for your suggestion. It's really useful for me to track this warning. Basically, I think the main error of my case, it can come from the serverless-webpack plugin. It freshly forces reinitializing our dbObject during the compiling time. I tried to use dynamic import to avoid this. However, for my case, It needs a little help with new implements from serverless-webpack.

Anyway, I really like pg-promise and also give a thumb up for your help. Thank you. @vitaly-t

LRagji commented 4 years ago

@vitaly-t Thanks for this package its really awesome, just a follow up Q on this warning part, it this warning now an anti-pattern? cause as per latest Aurora AWS RDS PG docs(some where i read) it says app shld always try to create new connections often as the connection string is load balanced pg cluster especially readers, which means i may create a new object for let say background report generation and other connection for normal app queries using the same connection string how valid is the warning in this case?

PaulKushch commented 4 years ago

Anyone maybe use next.js? I followed the proposed pattern at https://stackoverflow.com/questions/34382796/where-should-i-initialize-pg-promise but I get the same warning as you guys, "creating a duplicate connection...". What you think about the solution below (i.e. pool of 1 with serverless):

const connectionString = `postgresql://${process.env.DB_USER}:${process.env.DB_PASSWORD}@${process.env.DB_HOST}:${process.env.DB_PORT}/${process.env.DB_DB}`;
const cn = {
    capSQL: true,
};

const dbCn = {
    connectionString,
    max: 1,
};

// Use a symbol to store a global instance of a connection, and to access it from the singleton.
const DB_KEY = Symbol.for("MyApp.db");
const PGP_KEY = Symbol.for("MyApp.pgp");
const globalSymbols = Object.getOwnPropertySymbols(global);
const hasDb = globalSymbols.indexOf(DB_KEY) > -1;
if (!hasDb) {
    global[PGP_KEY] = require("pg-promise")(cn);
    global[DB_KEY] = global[PGP_KEY](dbCn);
}

// Create and freeze the singleton object so that it has an instance property.
const singleton = {};
Object.defineProperty(singleton, "db", {
    get() {
        return global[DB_KEY];
    },
});
Object.defineProperty(singleton, "pgp", {
    get() {
        return global[PGP_KEY];
    },
});
Object.freeze(singleton);

module.exports = {
    pgp: singleton.pgp,
    db: singleton.db,
};
vitaly-t commented 4 years ago

@PaulKushch The warning includes the line where the repeated initialization occurs, so start from there.

PaulKushch commented 4 years ago

@vitaly-t Yes, now the warning is not present, please see my code above. If I dont use singleton pattern, then the warning is there, with singleton pattern no warning. I use next.js... But do you think it is a reasonable solution? Best regards

vitaly-t commented 4 years ago

Th reasonable solution is the one you quoted from StackOverflow :)

PaulKushch commented 4 years ago

@vitaly-t In nextjs that solution gives warning. Like this guy mentions https://www.codeoftheprogrammer.com/2020/01/16/postgresql-from-nextjs-api-route/

vitaly-t commented 4 years ago

That's an odd one, since NodeJS loads each module just once, so it is expected to be a singleton based on that. Not sure why an extra singleton implementation is needed. That Next.js must be doing something abnormal, because I haven't seen such issue anywhere else.

PaulKushch commented 4 years ago

@vitaly-t Yes I agree, I asked at next js repo. Anyway, thanks for the great library :)

willieseabrook commented 3 years ago

I'm using nextjs and have that same problem with the warning message - that even when I use the best practice approach from stackoverflow it still gives the same warning.

FYI for anyone who finds this the nextjs discussion thread from PaulKushch is at https://github.com/vercel/next.js/discussions/18008 but as yet no answer.

@PaulKushch did you find a better solution other than the singleton pattern?

PaulKushch commented 3 years ago

@willieseabrook No I just use the one above. It does seem not totally correct though...

vitaly-t commented 3 years ago

I am re-opening this issue, just so I do not forget to have another look at it when I get a chance.

willieseabrook commented 3 years ago

Thanks for your solution @PaulKushch. For anyone with the same nextjs pd-promise problem - I can confirm Paul's approach to a workaround works - I'm using it with nextjs and no longer get the error.

vitaly-t commented 3 years ago

The following change was implemented and released in version 10.9.0 of the library...

Instead of using global const variables for internal pool repository, we are now using a Symbol on the global scope:

https://github.com/vitaly-t/pg-promise/blob/a8e94b80c0039db039e78e23c50e333688e5cd9f/lib/database-pool.js#L28

This should take care of the WARNING: Creating a duplicate... when modules are loaded in isolation.


For those who encountered the issue recently, please let us know if this update fixed it for you.

vitaly-t commented 3 years ago

In version 10.9.1 I did the same for QueryFile, which also uses a singleton to detect and report duplicate file usage.

vitaly-t commented 3 years ago

And in case this fix doesn't help, then the issue is not in this library, but definitely within another library you are using that breaks every singleton pattern in the app, by running modules multiple times. Let's see the feedback first.

vitaly-t commented 3 years ago

Here's a usable approach, using TypeScript:

// file db/index.ts

import * as pgLib from 'pg-promise';

const pgp = pgLib(/* initialization options */);

// generic singleton creator:
function createSingleton<T>(name: string, create: () => T): T {
    const s = Symbol.for(name);
    let scope = (global as any)[s];
    if (!scope) {
        scope = {...create()};
        (global as any)[s] = scope;
    }
    return scope;
}

interface IDatabaseScope {
    db: pgLib.IDatabase<any>;
    pgp: pgLib.IMain;
}

export function getDB(): IDatabaseScope {
    return createSingleton<IDatabaseScope>('my-app-db-space', () => {
        return {
            db: pgp('my-connect-string'),
            pgp
        };
    });
}

Then, in the beginning of any file that uses the database:

import {getDB} from './db';

const {db, pgp} = getDB();
vitaly-t commented 3 years ago

I have updated the popular question on StackOverflow about initialization.

If somebody still experiences an issue after all these updates, please let me know. In the meantime, I am closing this issue.

MatthewJamesBoyle commented 3 years ago

Hey I upgraded to pg-promise 10.9.1 and used your code above to create a singleton. Unfortunately, I am still seing the WARNING: Creating a duplicate database object for the same connection. error message. I'm running next 10.0.1 and here is my db code:

const pgp = require('pg-promise')();

// Get the values for these variables from configuration
const user = process.env.DB_USER
const password = process.env.DB_PASSWORD
const host = process.env.DB_HOST
const port = process.env.DB_PORT
const database = process.env.DB_NAME

const db = pgp(`postgres://${user}:${password}@${host}:${port}/${database}`);

function getSingleton(name, defaults) {
    const s = Symbol.for(name); // get global symbol
    let scope = global[s]; // get scope from the global
    if (!scope) {
        scope = {...defaults || {}}; // create singleton scope, with optional defaults
        global[s] = scope; // save our singleton in the global scope
    }
    return scope;
}

function getDB() {
    // get singleton, set db and pgp in it, if for the first time:
    return getSingleton('my-app-singleton-space', {db, pgp});
}

// module.exports = {getDB}
export default getDB;
vitaly-t commented 3 years ago

@MatthewJamesBoyle Can you create a small demo app that shows the problem? At this point I cannot do anything without a reproducible example. It shouldn't be difficult to create a small project on GitHub for this.

MatthewJamesBoyle commented 3 years ago

@vitaly-t here you go: https://github.com/MatthewJamesBoyle/pg-promise-sample run yarn and then yarn dev. If you navigate between localhost:3000 and localhost:3000/otherPage you should see the error in the dev console.

(you'll need to have a Postgres db setup with a users table, and a .env.local file with the following vars:

DB_USER
DB_PASSWORD
DB_HOST
DB_PORT
DB_NAME
vitaly-t commented 3 years ago

@MatthewJamesBoyle So basically, you took the singleton solution that I provided above, broke it in a few places, and then came back saying it doesn't work :smile:

What your code creates is not a singleton at all. Please follow the singleton pattern that was given above.

createSingleton is supposed to invoke the callback when singleton is not set, and then inside that callback is where db is supposed to be created. You reverted that logic, so your code now recreates the db whenever the module is loaded.

MatthewJamesBoyle commented 3 years ago

Here's a simple helper one can use for creating a singleton namespace:

/**
 * Retrieves or creates a singleton namespace/scope,
 * and optionally initializes it with default values.
 *
 * @param name
 * Globally-unique name for the scope.
 *
 * @param defaults
 * Optional object with default values-properties.
 *
 * @return {{}}
 * Singleton namespace/scope.
 */
function getSingleton(name, defaults) {
    const s = Symbol.for(name); // get global symbol
    let scope = global[s]; // get scope from the global
    if (!scope) {
        scope = {...defaults || {}}; // create singleton scope, with optional defaults
        global[s] = scope; // save our singleton in the global scope
    }
    return scope;
}

And inside a reusable module you can do something like this:

function getDB() {
    // get singleton, set db and pgp in it, if for the first time:
    return getSingleton('my-app-singleton-space', {db, pgp});
}

module.exports = {getDB};

@vitaly-t Not sure what you mean when you say "broke it in a few places" when I only made a single change to the code above which I copied and pasted?

The change I made was to swap:

function getDB() {
    // get singleton, set db and pgp in it, if for the first time:
    return getSingleton('my-app-singleton-space', {db, pgp});
}

to

function getDB() {
    // get singleton, set db and pgp in it, if for the first time:
    return getSingleton('my-app-singleton-space', {db, pgp}).db;
}

which I understand to be functionally equivalent?

I have updated the example repo which is now a direct copy and paste of the code you wrote above. I still the issue unfortunately.

Appreciate you looking into it!

vitaly-t commented 3 years ago

@MatthewJamesBoyle You are using the wrong example. I was referring to the last example, not what was posted earlier.

// file db/index.ts

import * as pgLib from 'pg-promise';

const pgp = pgLib(/* initialization options */);

// generic singleton creator:
function createSingleton<T>(name: string, create: () => T): T {
    const s = Symbol.for(name);
    let scope = (global as any)[s];
    if (!scope) {
        scope = {...create()};
        (global as any)[s] = scope;
    }
    return scope;
}

interface IDatabaseScope {
    db: pgLib.IDatabase<any>;
    pgp: pgLib.IMain;
}

export function getDB(): IDatabaseScope {
    return createSingleton<IDatabaseScope>('my-app-db-space', () => {
        return {
            db: pgp('my-connect-string'),
            pgp
        };
    });
}

And the question on StackOverflow uses the same.

MatthewJamesBoyle commented 3 years ago

@vitaly-t I didn't realise your previous example was invalid, might be best to remove it? My project doesn't use type script so opted for that. I will take a look at adapting that example shortly and see if it works, will report back :)