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.

Sign up for GitHub

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

feat(core): Make Postgres connection timeout configurable #10670

Merged
merged 3 commits into from
Sep 5, 2024
Merged

feat(core): Make Postgres connection timeout configurable #10670

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
21d53d6
feat(core): Make Postgres connection timeout configurable
ivov Sep 4, 2024
e11b53a
Fix test
ivov Sep 4, 2024
60a8f20
Change to 1s
ivov Sep 4, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions packages/@n8n/config/src/configs/database.config.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -75,6 +75,10 @@ class PostgresConfig {
@Env('DB_POSTGRESDB_POOL_SIZE')
poolSize: number = 2;

/** Postgres connection timeout (ms) */
@Env('DB_POSTGRESDB_CONNECTION_TIMEOUT')
connectionTimeoutMs: number = 1000;
Copy link
Contributor

Choose a reason for hiding this comment

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

1s is a very short default value. Something like 10-30s would make more sense. Also remember to update docs

Copy link
Contributor Author

Choose a reason for hiding this comment

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

According to Adi already 5s was too long? #10670 (comment)

The docs PR is already linked above: n8n-io/n8n-docs#2452

Copy link
Member

Choose a reason for hiding this comment

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

typeorm docs say

The milliseconds before a timeout occurs during the initial connection to the postgres server.

So this is only the timeout for opening the connection. If that takes longer than 1 second, we might have many other issues in the setup. Setting this to 30s might not be a great idea.

Copy link
Contributor

Choose a reason for hiding this comment

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

typeorm passes that into connectionTimeoutMillis for pg lib's Pool. So it includes the whole TCP connection opening, TLS handshake, PG wire protocol including authentication etc. I fear users might run into bunch of issues if the default is very tight

Copy link
Member

Choose a reason for hiding this comment

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

it includes the whole TCP connection opening, TLS handshake, PG wire protocol including authentication

all of that should not take more than a second, even under heavy load. If it does, there are fundamental infrastructure issues that need to be addressed, instead of letting all of this take 30 seconds before timing out.

Copy link
Contributor

@tomi tomi Sep 5, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

You assume everyone is running n8n with colocated DB. And even in that case the connection can take longer if the DB is under load. I can't start n8n on my local machine if I'm using a PG database that is located in Azure West Germany:

[Node] Error: Connection terminated due to connection timeout
[Node]     at Connection.<anonymous> (/n8n/node_modules/.pnpm/[email protected]/node_modules/pg/lib/client.js:132:73)
[Node]     at Object.onceWrapper (node:events:633:28)
[Node]     at Connection.emit (node:events:519:28)
[Node]     at Connection.emit (node:domain:488:12)
[Node]     at Socket.<anonymous> (/n8n/node_modules/.pnpm/[email protected]/node_modules/pg/lib/connection.js:63:12)
[Node]     at Socket.emit (node:events:519:28)
[Node]     at Socket.emit (node:domain:488:12)
[Node]     at TCP.<anonymous> (node:net:338:12) undefined
[Node] [nodemon] app crashed - waiting for file changes before starting...

Not saying it needs to be 30s, but even something like 5s would be more reasonable

Copy link
Member

Choose a reason for hiding this comment

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

Well, the question is do we really want to support a scenario like this with large latencies by default, or if anyone with a setup like this should have to manually set DB_POSTGRESDB_CONNECTION_TIMEOUT to a much higher value?

IMO, we should officially only support colocated databases, to avoid having to support all the other issues that large latencies might cause.


@Nested
ssl: PostgresSSLConfig;
}
Expand Down
1 change: 1 addition & 0 deletions packages/@n8n/config/test/config.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,7 @@ describe('GlobalConfig', () => {
poolSize: 2,
port: 5432,
schema: 'public',
connectionTimeoutMs: 1000,
ssl: {
ca: '',
cert: '',
Expand Down
1 change: 1 addition & 0 deletions packages/cli/src/databases/config.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -104,6 +104,7 @@ const getPostgresConnectionOptions = (): PostgresConnectionOptions => {
schema: postgresConfig.schema,
poolSize: postgresConfig.poolSize,
migrations: postgresMigrations,
connectTimeoutMS: postgresConfig.connectionTimeoutMs,
ssl,
};
};
Expand Down
Loading