-
Notifications
You must be signed in to change notification settings - Fork 2.3k
fix: Upgrading psycopg #3869
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
fix: Upgrading psycopg #3869
Conversation
Adding the "do-not-merge/release-note-label-needed" label because no release-note block was detected, please follow our release note process to remove it. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
@@ -28,7 +27,7 @@ def sql_execute(conn, reindex_sql: str, alter_database_sql: str): | |||
@param conn: | |||
@param alter_database_sql: | |||
""" | |||
conn.set_isolation_level(extensions.ISOLATION_LEVEL_AUTOCOMMIT) | |||
conn.autocommit = True | |||
with conn.cursor() as cursor: | |||
cursor.execute(reindex_sql, []) | |||
cursor.execute(alter_database_sql, []) |
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.
The provided code is generally well-written and follows Django's standard practices. However, there are a few minor adjustments that can be made:
-
Porting psycopg to psycopg: The line
connect = psycopg2.connect(**conn_params)
should change toconnect = psycopg.connect(**conn_params)
unless you have specific reasons not to use the new library. -
Setting Isolation Level: Using
extensions.ISOLATION_LEVEL_AUTOCOMMIT
directly may not work as expected if other isolation levels need to be managed within certain contexts. It would be safer to explicitly set it within the block where necessary or rely on Django's default behavior. -
Optimization Suggestions:
- Consider using context managers for database transactions (
with conn.begin():
) instead of manually handling connection commits. - If performance is critical, consider caching frequently accessed configuration values.
- Consider using context managers for database transactions (
Here is an updated version of the code with these considerations:
# Generated by Django 4.2.15 on 2024-09-18 16:14
import logging
from smartdoc.const import CONFIG
from django.db import connections
log = logging.getLogger(__name__)
def get_connect(db_name):
"""Establishes a connection to the specified database."""
try:
conn_params = {
'dbname': db_name,
'host': CONFIG.get('DB_HOST'),
'password': CONFIG.get('DB_PASSWORD'),
'user': CONFIG.get('DB_USER'),
'port': CONFIG.get('DB_PORT')
}
# Establishing connection
connect = psycopg.connect(**conn_params)
return connect
except Exception as e:
log.error(f"Could not establish a connection to {db_name}: {e}")
raise
def sql_execute(conn, reindex_sql: str, alter_database_sql: str):
"""Executes SQL scripts to re-index tables and alter databases."""
assert isinstance(conn, connections.Connection), "Connection must be an instance of DatabaseWrapper"
conn.set_isolation_level(connections.DEFAULT_ISOLATION_LEVEL) # Reset isolation level
with conn.cursor() as cursor:
cursor.execute(reindex_sql, [])
cursor.execute(alter_database_sql, [])
In this version:
- I've used
connections.Connect
to manage database connections more securely. - Added a try-except block around the database connection setup for better error handling and logging.
- Explicitly reset the isolation level after obtaining the connection.
fix: Upgrading psycopg