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

fix(Postgres Node): Re-use connection pool across executions #12346

Open
wants to merge 8 commits into
base: master
Choose a base branch
from

Conversation

despairblue
Copy link
Contributor

@despairblue despairblue commented Dec 23, 2024

Summary

This add a connection pool manager similar to the ssh SSHClientsManager for re-using ssh connections.

It's a singleton that lives in the nodes-base package. Adi and I initially tried to make this available through the context (node execution functions) and also have this available to the credentials and have them manage the connections, but this led to changes to the interfaces in the workflow package that would have culminated in a single huge refactor in this PR. Instead we chose this simpler approach that can later be integrated into the context and be made available to the credentials instead of the nodes by further refactors.

Next step is to use the manager for more nodes, e.g. MSSQL and MySQL. This will also fix an issue that at least one cloud customer has with connections to MSSQL being blocked because we connect and disconnect too often when they execute the MSSQL node in a tight loop.

Related Linear tickets, Github issues, and Community forum posts

todo: add help ticket link

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)

Copy link

codecov bot commented Dec 23, 2024

@n8n-assistant n8n-assistant bot added n8n team Authored by the n8n team node/improvement New feature or request labels Dec 23, 2024
@despairblue despairblue changed the title fix(Postgres Node): re-use connection pool across executions fix(Postgres Node): Re-use connection pool across executions Dec 23, 2024
@despairblue despairblue marked this pull request as ready for review December 24, 2024 10:56
Copy link
Member

@netroy netroy left a comment

Choose a reason for hiding this comment

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

besides some minor comments, LGTM 🎉.

packages/nodes-base/utils/connection-pool-manager.ts Outdated Show resolved Hide resolved
packages/nodes-base/utils/connection-pool-manager.ts Outdated Show resolved Hide resolved
Copy link

cypress bot commented Dec 24, 2024

n8n    Run #8467

Run Properties:  status check passed Passed #8467  •  git commit 63865918dc: 🌳 🖥️ browsers:node18.12.0-chrome107 🤖 despairblue 🗃️ e2e/*
Project n8n
Branch Review connection-pooling
Run status status check passed Passed #8467
Run duration 04m 43s
Commit git commit 63865918dc: 🌳 🖥️ browsers:node18.12.0-chrome107 🤖 despairblue 🗃️ e2e/*
Committer कारतोफ्फेलस्क्रिप्ट™
View all properties for this run ↗︎

Test results
Tests that failed  Failures 0
Tests that were flaky  Flaky 1
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 483
View all changes introduced in this branch ↗︎

Copy link
Contributor

✅ All Cypress E2E specs passed

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
n8n team Authored by the n8n team node/improvement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants