Postgres fixes: Concurrency issue, connection leak, and fixes to guard CRUD operations #83
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…en starting with more than one worker using postgres client)
CalebCourier
reviewed
Feb 6, 2025
CalebCourier
reviewed
Feb 6, 2025
compose.yml
Outdated
| @@ -9,7 +9,7 @@ services: | |||
| volumes: | |||
| - ./postgres:/data/postgres | |||
| ports: | |||
| - "5432:5432" | |||
| - "5932:5432" | |||
There was a problem hiding this comment.
Is this change intentional?
There was a problem hiding this comment.
good catch shouldn't be checked in
CalebCourier
reviewed
Feb 6, 2025
Comment on lines
+45
to
+49
| PGPORT: 5432 | ||
| PGDATABASE: postgres | ||
| PGHOST: postgres | ||
| PGUSER: ${PGUSER:-postgres} | ||
| PGPASSWORD: ${PGPASSWORD:-changeme} |
There was a problem hiding this comment.
I'm starting to think we should just have two different Dockfiles and compose files to run with PG vs config.py
CalebCourier
reviewed
Feb 6, 2025
Comment on lines
+80
to
+82
| # Support for providing env vars as uvicorn does not support supplying args to create_app | ||
| # - Usage: GR_CONFIG_FILE_PATH=config.py GR_ENV_FILE=.env PORT=8080 uvicorn --factory 'guardrails_api.app:create_app' --host 0.0.0.0 --port $PORT --workers 2 --timeout-keep-alive 90 | ||
| # - Usage: gunicorn -k uvicorn.workers.UvicornWorker --bind 0.0.0.0:$PORT --timeout=90 --workers=2 "guardrails_api.app:create_app(None, None, $PORT)" |
There was a problem hiding this comment.
That's rough. We'll have to address this in the OSS's cli as well since it calls this.
CalebCourier
reviewed
Feb 6, 2025
Comment on lines
+100
to
+115
| with engine.begin() as connection: | ||
| lock_acquired = connection.execute(text(f"SELECT pg_try_advisory_lock({lock_id});")).scalar() | ||
| if lock_acquired: | ||
| self.run_initialization(connection) | ||
| # Release the lock after initialization is complete | ||
| connection.execute(text(f"SELECT pg_advisory_unlock({lock_id});")) | ||
|
|
||
| def run_initialization(self, connection): | ||
| # Perform the actual initialization tasks | ||
| from guardrails_api.models import GuardItem, GuardItemAudit # noqa | ||
| Base.metadata.create_all(bind=self.engine) | ||
|
|
||
| # Execute custom SQL extensions and triggers | ||
| connection.execute(text(INIT_EXTENSIONS)) | ||
| connection.execute(text(AUDIT_FUNCTION)) | ||
| connection.execute(text(AUDIT_TRIGGER)) |
There was a problem hiding this comment.
Good patch. We probably just need a real migration tool in the long run since sql-alchemy doesn't do updates.
There was a problem hiding this comment.
totally I believe they can be kicked off programatically as well so we can probably add this to lifecycle hooks
CalebCourier
previously approved these changes
Feb 6, 2025
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Pull Request Type
Link to Notion Task or Github Issue
Summary of Feature(s)
Summary of Bug Fix(es)
gunicornneed to useuvicornworkers (WSGI / ASGI incompatibility) due to the FastAPI switchdb = next(self.get_db())was essentially causing connections to proliferate due to not being properly closed, so opted for contextmanager approachdb = next(self.get_db())was causing some changes not to commit properly so deletions / updates weren't working properly due to inconsistent states during db.commitsPrevious Behaviour
UDATE/DELETEAPI CRUD operations were not workingNew Behaviour
UDATE/DELETEAPI CRUD operations workingOther details
util_methods, these are utility functions which don't create db connections but rather take db sessions as arguments and perform auxiliary actions for the non util functions which are called directly by API controllers and do start db sessionsDependencies