-
Notifications
You must be signed in to change notification settings - Fork 11
configure django's database connections #1191
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: dev
Are you sure you want to change the base?
Conversation
WalkthroughAdds PostgreSQL connection reuse and health-check parameters plus connection and statement timeout options to the AWS RDS database settings in configuration. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
d2c71eb to
ed6e9e8
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.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
tcf_core/settings/base.py(1 hunks)
🔇 Additional comments (2)
tcf_core/settings/base.py (2)
114-114: Validate the 30-second query timeout against actual workload.The PR description mentions these settings were "Prompted AI for connection settings needed" without specifics on load testing or query profiling. A 30-second query timeout may be too generous for typical web requests (causing slow user experience) or too restrictive for legitimate batch operations.
Monitor query execution times in production and verify:
- What percentage of queries currently exceed 30 seconds?
- Are there legitimate long-running queries that need higher limits?
- What's the 95th/99th percentile query duration?
Consider making timeout values configurable via environment variables:
"OPTIONS": { - "connect_timeout": 10, # Timeout for establishing connection - "options": "-c statement_timeout=30000", # 30 second query timeout + "connect_timeout": env.int("DB_CONNECT_TIMEOUT", default=10), + "options": f"-c statement_timeout={env.int('DB_STATEMENT_TIMEOUT_MS', default=30000)}", },This allows tuning without code changes and different values per environment.
108-108: No action needed — CONN_MAX_AGE setting is appropriate for your deployment.Your codebase uses Gunicorn with persistent WSGI processes (as specified in Procfile), which is exactly the scenario where a 60-second connection lifetime is suitable. AWS RDS benefits from maintaining persistent connection pooling with health checks enabled (
CONN_HEALTH_CHECKS: True), and your configuration properly handles this with appropriate connection timeout settings.
| # Connection pooling - reuse connections between requests | ||
| "CONN_MAX_AGE": 60, # Keep connections alive for 60 seconds | ||
| # Health checks (Django 4.1+) - verify connection is still valid | ||
| "CONN_HEALTH_CHECKS": True, | ||
| # Database connection options | ||
| "OPTIONS": { | ||
| "connect_timeout": 10, # Timeout for establishing connection | ||
| "options": "-c statement_timeout=30000", # 30 second query timeout | ||
| }, |
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.
Apply connection settings to dev environment for consistency.
These database connection settings are only applied to the AWS RDS (production) configuration but not to the dev configuration (lines 59-68). This creates an environment inconsistency that could mask production issues during development.
Apply this diff to add similar settings to the dev database configuration:
DATABASES = {
"default": {
"NAME": env.str("DB_NAME"),
"ENGINE": "django.db.backends.postgresql_psycopg2",
"USER": env.str("DB_USER"),
"PASSWORD": env.str("DB_PASSWORD"),
"HOST": env.str("DB_HOST"),
"PORT": env.int("DB_PORT"),
+ "CONN_MAX_AGE": 60,
+ "CONN_HEALTH_CHECKS": True,
+ "OPTIONS": {
+ "connect_timeout": 10,
+ "options": "-c statement_timeout=30000",
+ },
}
}Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In tcf_core/settings/base.py around lines 107-115 and the dev DB block at lines
~59-68, the connection pooling and options (CONN_MAX_AGE, CONN_HEALTH_CHECKS,
and OPTIONS with connect_timeout and options="-c statement_timeout=30000") are
only applied to production; add the same keys to the dev DATABASES['default']
configuration so dev uses identical connection settings. Update the dev DB dict
to include "CONN_MAX_AGE": 60, "CONN_HEALTH_CHECKS": True, and an "OPTIONS" dict
containing "connect_timeout": 10 and "options": "-c statement_timeout=30000" (or
merge these using an existing shared settings dict if present) and keep/update
comments to reflect the intent.
GitHub Issues addressed
What I did
Screenshots
Testing
Questions/Discussions/Notes
Summary by CodeRabbit