readme suggests done() which is incorrect

[phillip-haydon]

On the readme, it says to call done();

Following this, 3 times I managed to exhaust all database connections. The link under the example suggests done(client); which correctly closes the connection.

I think this is a possible documentation error.

---

Basically we need the connection timeout handler to live in a separate process to make the lambda function freezable. That should solve the termination timeout issue (in theory).

How lambda handles re-use is described here in detail:
https://aws.amazon.com/blogs/compute/container-reuse-in-lambda/
Pay attention to the last section in particular.

[brianc]

where exactly in the readme is there an error? calling done() without
passing it any arguments is the correct way to return clients to the
connection pool. if you are exhausting your pool it is likely you are
forgetting to call done somewhere?

On Tuesday, February 2, 2016, Phillip Haydon notifications@github.com
wrote:

On the readme, it says to call done();

Following this, 3 times I managed to exhaust all database connections. The
link under the example suggests done(client); which correctly closes the
connection.

I think this is a possible documentation error.

—
Reply to this email directly or view it on GitHub
#930.

[phillip-haydon]

I only have 2 queries total in node, and both queries are the same shown in the activity, no other code queries those 2 tables.

Both were only using done();, the only thing I changed was pass in the client, done(client); and the problem went away. I can see it in the activity, before change if I touched the url, the connections would pile up and never leave. After the change they show and disappear pretty much immediately.

[brianc]

If you pass a truthy value to done() it will close the client. You might not want to do that because you aren't really using a pool if you close the client every time you run a query. Can you share a code sample so I can look at it? Otherwise I can't really help...I just double-checked and the code in the readme is 100% correct in the way its calling done.

[phillip-haydon]

Yup sure.

This runs in an AWS Lambda, it checks to see if the token exists, then calls a bunch of web services (removed) then inserts the result.

This is called using run-waterfall library.

https://www.npmjs.com/package/run-waterfall

        function(args, callback) {
            pg.connect(connectionString, function (err, client, done) {
                console.log('Querying the Token to check user is authenticated');
                client.query('SELECT * FROM token WHERE token = $1', [args.token], function(err, result) {
                    done(client);

                    if (err || result.rows.length === 0) {
                        return callback(new Error("Token is invalid."));
                    }

                    return callback(null, args);
                });
            });
        },
        // Snipped a bunch of 3rd party requests for data

        function(args, callback) {
            pg.connect(connectionString, function (err, client, done) {
                console.log('Insert the suggestions');
                client.query('INSERT INTO mt_doc_album ("data") VALUES ($1)', [JSON.stringify(args.suggestions)], function(err, result) {
                    done(client);

                    if (err) {
                        return callback(err);
                    }

                    return callback(null, args);
                });
            });
        },

[brianc]

Yeah calling done(client) up there is gonna disconnect the client immediately after the query. You can do that but you lose the advantage of using a pool at all. The reason to use a pool is because the overhead of establishing a connection to postgres initially is 20x as much work as dispatching a single query. I'd remove the client argument and just call done() - it wont close the connections to postgres right away (by default there's a 1 minute timeout) but it wont open more than pg.defaults.poolSize connections either - it will reuse them. The default pool size is 20.

One thing I can see is I'm not sure exactly how aws lamda works. If every instance of a lambda script spins up its own worker process then each worker process will have its own pool, so you can end up with number-of-workers * 20 connections to postgres which might be too many. Depending on your database resources you will eventually exhaust connections - in my experience I try to keep connections to under 200. In this situation you can either reduce the pool size for each worker: pg.defaults.poolSize = 2 which will get you past the hump. Regardless of poolSize if you have enough requests simultaneously to lambda you will exhaust your connections - bascially if your lamda process group size is unbounded you have to have a way to funnel your queries through some more bounded endpoint....there is also pgbouncer but I've had bug reports on it from time to time - YMMV.

Hope this was helpful.

[phillip-haydon]

Ahh ok.

Then what is happening would be correct, to disconnect the client immediately.

Lambda's are small containers that are deployed and your piece of code is executed against, exists for short period of time. Once that piece of code is executed the process is killed. There's nothing to share the client with to pool a connection.

So if I was using express I would just call done(); which would close the current connection but keep it in the client for pooling.

Thanks for clearing that up ❤️

[brianc]

awesome! so glad we got that figured out! extra glad because i was gonna
start fiddling with lamda soon. 😄

On Tuesday, February 2, 2016, Phillip Haydon notifications@github.com
wrote:

Closed #930 #930.

—
Reply to this email directly or view it on GitHub
#930 (comment).

[lukas-gitl]

Thanks, this helped me a lot!

@phillip-haydon @brianc According to the AWS documentation you would expect the pool to persist between different lambda runs (not guaranteed). And then you wouldn't have to destroy the client.

I suspect this has something to do with the timeout of the reap function. It somehow prevents the thread from sleeping even though the lambda context has been closed (??). Very weird. Here is a related thread on the aws forums: https://forums.aws.amazon.com/thread.jspa?threadID=216000

I'm wondering if we can do something in this library. Can we solve idle connection killing differently than with a timeout? Something that doesn't block the lambda function?

[lukas-gitl]

After thinking about this more, I'm re-opening this as a feature request:
Basically we need the connection timeout handler to live in a separate process to make the lambda function freezable. That should solve the termination timeout issue (in theory).

How lambda handles re-use is described here in detail:
https://aws.amazon.com/blogs/compute/container-reuse-in-lambda/
Pay attention to the last section in particular.

Edit: I can't seem to edit this issue. Would you mind @phillip-haydon? Thanks!

[phillip-haydon]

We shall await @brianc's statement on this.

[lukas-gitl]

I think I finally figured out what is happening:
https://aws.amazon.com/blogs/compute/node-js-4-3-2-runtime-now-available-on-lambda/
Take a look at "Node callbacks". Let me test this and confirm that this actually solves the issue!

[lukas-gitl]

Yep, setting context.callbackWaitsForEmptyEventLoop = false; seems to resolve this.

Can you confirm @brianc ?

[Vanuan]

This should be documented.
I'm using pool because there was hard to find info about providing a connection string when using raw pg client.