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

refactor(Postgres Node): Backport connection pooling to postgres v1 #12484

Merged
merged 6 commits into from
Jan 16, 2025

Conversation

despairblue
Copy link
Contributor

@despairblue despairblue commented Jan 7, 2025

Summary

This uses the pool manager for postgres v1, but additionally it also fixes the credential test for postgres which before would shut down the shared pool and lead to Connection pool of the database object has been destroyed. on any subsequent execution.

Taken from the pg-promise docs:

Object db represents the Database protocol with lazy connection, i.e. only the actual query methods acquire and release the connection automatically. Therefore, you should create only one global/shared db object per connection details.

Thus it's not necessary to release connections manually. They are managed by pg-promise.

You can verify this using these queries and constructing a workflow that executes them once a second:

Lists all open connections per db, grouped by state.

SELECT 
    datname,
    usename,
    state,
    COUNT(*) 
FROM pg_stat_activity 
WHERE backend_type = 'client backend'
GROUP BY datname, usename, state;

Long running query to simulated load.

SELECT 
    g.i,
    pg_sleep(0.001)  -- each row sleeps for 1ms
FROM generate_series(1, 1000) g(i);

By default the pool has a size of 10. If we need more then the next execution hangs until the next connection is free.

That limit is way too low.

I set it to 10_000 to test what happens if I ran out of the 100 connections my local postgres allows:

image

Also quickly after deactivating the workflow the pool was cleaned up and all 100 connections have been available again.

I would like to set it to unlimited for now, but that's not possible:
brianc/node-postgres#1977

But for that reason I made it configurable in the credential.

image

Related Linear tickets, Github issues, and Community forum posts

Fixes #12517
NODE-2240

Review / Merge checklist

  • PR title and summary are descriptive. (conventions)
  • Docs updated or follow-up ticket created.
  • Tests included.
  • PR Labeled with release/backport (if the PR is an urgent fix that needs to be backported)

@n8n-assistant n8n-assistant bot added n8n team Authored by the n8n team node/improvement New feature or request labels Jan 7, 2025

const db = pgp(config);
await db.connect();
Copy link
Contributor Author

@despairblue despairblue Jan 7, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This would have left the connection hanging. It needs to be ended:

https://vitaly-t.github.io/pg-promise/Database.html#connect

The connection must be released in the end of the chain by calling done() on the connection object.

@@ -41,8 +39,8 @@ export async function postgresConnectionTest(
message,
};
} finally {
if (pgpClientCreated) {
pgpClientCreated.end();
Copy link
Contributor Author

@despairblue despairblue Jan 7, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This would shut down all pools in the process:
https://vitaly-t.github.io/pg-promise/module-pg-promise.html#~end

Shuts down all connection pools created in the process, so it can terminate without delay. It is available as pgp.end, after initializing the library.

Doing this would render any pool for any credential unusable until restarting n8n or waiting for the pool to be destroyed by the connection pool manager and every execution would throw this:
Connection pool of the database object has been destroyed.

@despairblue despairblue changed the title refactor(Postgres Node): backport connection pooling to postgres v1 refactor(Postgres Node): Backport connection pooling to postgres v1 Jan 7, 2025
@netroy netroy marked this pull request as ready for review January 14, 2025 15:19
netroy
netroy previously approved these changes Jan 14, 2025
Copy link
Contributor

⚠️ Some Cypress E2E specs are failing, please fix them before merging

Copy link

cypress bot commented Jan 14, 2025

n8n    Run #8772

Run Properties:  status check passed Passed #8772  •  git commit 9ce7027c46: 🌳 🖥️ browsers:node18.12.0-chrome107 🤖 despairblue 🗃️ e2e/*
Project n8n
Branch Review backport-connection-pooling-to-postgres-v1
Run status status check passed Passed #8772
Run duration 04m 55s
Commit git commit 9ce7027c46: 🌳 🖥️ browsers:node18.12.0-chrome107 🤖 despairblue 🗃️ e2e/*
Committer Danny Martini
View all properties for this run ↗︎

Test results
Tests that failed  Failures 0
Tests that were flaky  Flaky 2
Tests that did not run due to a developer annotating a test with .skip  Pending 0
Tests that did not run due to a failure in a mocha hook  Skipped 0
Tests that passed  Passing 489
View all changes introduced in this branch ↗︎

@despairblue despairblue force-pushed the backport-connection-pooling-to-postgres-v1 branch from 78ad9be to a98c60d Compare January 14, 2025 16:33
@despairblue despairblue requested a review from netroy January 14, 2025 16:33
@dana-gill dana-gill self-requested a review January 14, 2025 16:37
Copy link
Contributor

@dana-gill dana-gill left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

tested locally and it works 🥳 lgtm!

Copy link
Contributor

⚠️ Some Cypress E2E specs are failing, please fix them before merging

@HermesMacedo
Copy link

Any news? release 1.75.0 has not been resolved.

@tomi tomi added the release/backport Changes that need to be backported to older releases. label Jan 16, 2025
@despairblue despairblue force-pushed the backport-connection-pooling-to-postgres-v1 branch from a98c60d to 9ce7027 Compare January 16, 2025 09:29
Copy link
Contributor

✅ All Cypress E2E specs passed

@despairblue despairblue merged commit 35cb10c into master Jan 16, 2025
38 checks passed
@despairblue despairblue deleted the backport-connection-pooling-to-postgres-v1 branch January 16, 2025 10:09
@despairblue
Copy link
Contributor Author

despairblue commented Jan 16, 2025

Any news? release 1.75.0 has not been resolved.

@HermesMacedo This will be part of 1.75.1 which is scheduled to be released today.

@gonzalogcontacto
Copy link

Many people are waiting the fix to be deployed 🥲 Thanks for that fix @despairblue 🙌🏻

@janober
Copy link
Member

janober commented Jan 17, 2025

Got released with n8n@1.74.2

@g-roliveira
Copy link

The problem still persists for me in version 1.74.2 and 1.75.1

@HermesMacedo
Copy link

HermesMacedo commented Jan 17, 2025

@janober The problem still persists for me in version 1.75.1

It responds on the first interaction, but the error occurs on subsequent interactions.

error-chatmemoru-error

{
"errorMessage": "Cannot use a pool after calling end on the pool",
"errorDescription": "Cannot use a pool after calling end on the pool",
"errorDetails": {},
"n8nDetails": {
"time": "17/01/2025, 10:09:11",
"n8nVersion": "1.75.1 (Self Hosted)",
"binaryDataMode": "default",
"cause": {}
}
}

@zberg007
Copy link

Same here on both versions. Cloud hosted. All subsequent interactions fail.

@netroy
Copy link
Member

netroy commented Jan 17, 2025

Please let's move the conversation to the issue #12517 . That way we don't have to keep posting the same message in multiple places.
#12517 (comment)

@n8n-io n8n-io locked as off-topic and limited conversation to collaborators Jan 17, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
n8n team Authored by the n8n team node/improvement New feature or request release/backport Changes that need to be backported to older releases. Released
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1.74.0 - PostgreSQL - Cannot use a pool after calling end on the pool
9 participants