Skip to content

Conversation

abdibaker
Copy link
Contributor

Description

This PR fixes PostgreSQL compatibility issues that were causing CI pipeline failures. The main issue was that the SQL panel's profiling feature was attempting to use MySQL-specific commands on PostgreSQL databases, resulting in errors like "unrecognized configuration parameter profiling".

Additionally, the CI configuration had missing environment variables for PostgreSQL test jobs and a malformed lint job configuration.

Key Changes:

  • Added vendor detection to SQL panel profiling to prevent MySQL-specific commands on other databases
  • Fixed CI PostgreSQL job environment variables (DB_NAME, DB_USER, DB_PASSWORD)
  • Fixed malformed lint job strategy configuration
  • Added comprehensive tests for PostgreSQL compatibility

Root Cause: The SQLSelectForm.profile() method was executing MySQL-specific SET PROFILING commands and querying information_schema.profiling without checking the database vendor, causing PostgreSQL CI jobs to fail.

Solution: Added vendor detection that raises a clear ValueError for non-MySQL databases, preventing the MySQL-specific code from executing on PostgreSQL/SQLite.

Fixes #2185

Checklist:

  • I have added the relevant tests for this change.
  • I have added an item to the Pending section of docs/changes.rst.

Fixes django-commons#2185

- Add vendor detection to SQL panel profiling to prevent MySQL-specific
  commands on PostgreSQL
- Fix CI PostgreSQL job environment variables
- Fix malformed lint job strategy configuration
- Add comprehensive PostgreSQL compatibility tests

Changes:
- debug_toolbar/panels/sql/forms.py: Add vendor check in profile() method
- .github/workflows/test.yml: Add DB_* env vars for PostgreSQL, fix lint job
- tests/test_postgresql_compatibility.py: New test file for PostgreSQL fixes
- docs/changes.rst: Document PostgreSQL compatibility improvements
Copy link
Member

@tim-schilling tim-schilling left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the idea of this change is a quality one, but I think we should move the logic up into the form layer. Try subclassing SQLSelectForm with some custom validation to do this check. That will protect this in the sql_profile and keep our instrumentation logic cleaner.

We should also prevent the profile button from appearing when using an incompatible vendor.

Lastly, we may want to check Django's own backend features to see if this is something we can base off Django's own checks. There's no sense in both repositories maintaining a boolean constant about whether or not this feature is supported in MySQL.

Comment on lines +21 to +22
if connection.vendor == "mysql":
self.skipTest("This test is for non-MySQL databases")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be moved up as a decorator. Search for other usages of connection.vendor and tests that are skipped.

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.

PostgreSQL compatibility issues in CI and SQL profiling
2 participants