-
Notifications
You must be signed in to change notification settings - Fork 2.1k
Initial pass at PostgreSQL session management #1163
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
base: main
Are you sure you want to change the base?
Initial pass at PostgreSQL session management #1163
Conversation
Wanted to get the code up here in case there was some discussion on how to do things. Will look into the documentation tomorrow. |
Got a little stuck on the I took a stab at documentation. Open to feedback on structuring. Overall I wanted the postgres session to be in extensions because psycopg can be somewhat heavy on certain platforms. But I wasn't sure if the docs should go with extensions or the other memory docs. |
ff857ed
to
be28fbc
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@artificial-aidan lmk if this is ready for review and i can take a pass!
Should be ready now. Have been running it in our testing for about a week now. |
be28fbc
to
4498798
Compare
9f0d7a1
to
78aa642
Compare
Tests fixed. Missed the test changes I had made locally. |
ef1a02d
to
7d392a1
Compare
def __init__( | ||
self, | ||
session_id: str, | ||
pool: AsyncConnectionPool, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why you use a hole connection pool just for one session management? One connection will be enough.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In my use case this is running in a service with many concurrent sessions. A shared connection pool is used between them. That's why you can initialize it from a connection string, or using an existing pool
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay, that is your way, it is not bad.
* Add postgres_session.py * Implements session management using PostgreSQL * Accepts a pool as a default argument * Additional class method for creation from a connection string * Add optional-dependency for psycopg Signed-off-by: Aidan Jensen <aidan@artificial.com>
Signed-off-by: Aidan Jensen <aidan@artificial.com>
Signed-off-by: Aidan Jensen <aidan@artificial.com>
* Fix warning from opening a pool in the constructor: https://www.psycopg.org/psycopg3/docs/news_pool.html#psycopg-pool-3-2-2 * Add integration tests that can be run locally * Fix get_items with limit Signed-off-by: Aidan Jensen <aidan@artificial.com>
Signed-off-by: Aidan Jensen <aidan@artificial.com>
7d392a1
to
4eb7af4
Compare
Summary
Test plan
Tests were written mocking the postgres calls. This was tested locally against a docker postgres database, with a variety of input messages.
Issue number
Related to #745
Checks
make lint
andmake format