Skip to content

Conversation

@metala
Copy link

@metala metala commented Jul 30, 2021

Rationale.
In order to mitigate the issue of long running connections that increase database memory usage over time I have introduced lifetime to all connections of the pool. I need your feedback on the patch.

@iby
Copy link
Contributor

iby commented Aug 3, 2021

@metala Hey there! Just curious if this attempts to solve #163 and removes hanging connections after completed queries?

@metala
Copy link
Author

metala commented Aug 3, 2021

@iby Actually, it's about an OOM issue I hit with PostgreSQL as one of the services was making a lot of specific queries and crashing the database server. It might work for that case as well, but the connection lifetime is set using time duration and not number of requests.

The issue is about how Linux's fork() and memory CoW work. It is better explained here: https://italux.medium.com/postgresql-out-of-memory-3fc1105446d

@porsager
Copy link
Owner

porsager commented Aug 3, 2021

Yeah, #163 didn't really have an issue did it? what are you having issues with @iby ? (maybe open a new issue)

@metala metala marked this pull request as ready for review September 4, 2021 22:20
@metala
Copy link
Author

metala commented Sep 4, 2021

I am trying to run all the tests, but the test runner is broken on my setup. I have just ensured that my new tests are running fine.

My test setup:

$ docker run -it -e 'POSTGRES_USER=metala' -e 'POSTGRES_DATABASE=postgres_js_test' -e 'POSTGRES_HOST_AUTH_METHOD=trust' -v '/var/run/postgresql/:/var/run/postgresql/' postgres:13
$ yarn test

Edit. I have some new thoughts.

  1. Maybe the slot should be released only when the connection has ended. Thus we will not exceed pool max connections.
  2. Rename release to gracefullyEnd and it's flag from released to endingGracefully.

Let me know what you think about that.

@metala
Copy link
Author

metala commented Sep 19, 2021

We are switching DB, so I don't think I would be able to maintain my change here, thus I will just close the MR.

@metala metala closed this Sep 19, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants