Skip to content

Conversation

@goodtune
Copy link
Collaborator

@goodtune goodtune commented Sep 1, 2025

Summary

Fixes RecursionError that occurs when using psycopg3 driver with DEBUG=True. The bug was caused by Django's SQL debug logging triggering psycopg3's mogrify operation, which opens fresh cursors that re-enter our _cursor() method, causing infinite recursion during search_path setting.

Changes

  • ContextVar Guard: Prevents recursion during search_path operations
  • Parameterized Queries: Uses set_config() with parameters instead of raw SQL formatting
  • Smart Caching: Only sets search_path when tenant configuration actually changes
  • Mogrify Optimization: Skips unnecessary mogrify calls for parameterless queries
  • Isolated Unit Tests: Mock-based tests safe for parallel execution without DB operations
  • Single Integration Test: Real database test with proper cleanup for end-to-end validation

Test Plan

  • All existing tests continue to pass
  • New isolated unit tests verify recursion prevention and caching behavior
  • Tests use mock tenants to avoid DB schema creation and constraint violations
  • Single integration test validates end-to-end functionality with real tenant
  • Tests are thread-safe and suitable for parallel CI execution
  • Compatible with both psycopg2 and psycopg3

Compatibility

  • ✅ Backward compatible - no API changes
  • ✅ Works with both psycopg2 and psycopg3
  • ✅ No configuration required
  • ✅ Transparent to consuming applications

@goodtune goodtune added the bug label Sep 1, 2025
@goodtune goodtune force-pushed the fix-psycopg3-debug-recursion-bug branch from c16ca4c to 92632f7 Compare September 1, 2025 21:56
- Add ContextVar guard to prevent recursion during search_path setting
- Switch from raw SQL to parameterized set_config() calls
- Add per-connection search path caching to avoid redundant updates
- Override last_executed_query() to skip mogrify when no parameters
- Add isolated unit tests with complete database settings for CI compatibility
- Integration test uses public schema to avoid tenant creation complexity
@goodtune goodtune force-pushed the fix-psycopg3-debug-recursion-bug branch from 92632f7 to 3b492e4 Compare September 2, 2025 07:42
- Remove unused `django.conf.settings` imports from migration files
- Remove unnecessary f-string formatting in warning messages
- Remove unused imports (`unittest`, `sys`) from test files
- Remove redundant `@unittest.skipUnless` decorator from integration test
- Clean up import statements in `test_psycopg3_recursion.py`
@goodtune goodtune requested a review from Copilot September 2, 2025 08:40
Copilot

This comment was marked as outdated.

@goodtune goodtune force-pushed the fix-psycopg3-debug-recursion-bug branch from d5ad841 to e0780c5 Compare September 2, 2025 08:50
- Replace direct Django settings modification with `@override_settings` decorator
- Improve getattr fallback pattern with explicit hasattr check and comments
- Make parameter check more explicit in `last_executed_query` method
- Ensure proper test isolation and follow Django testing best practices

Changes:
- Use `@override_settings` decorators across test classes and methods
- Add detailed comments explaining psycopg2/psycopg3 cursor attribute differences
- Replace `if not params:` with explicit `params is None or len(params) == 0`
- Remove direct settings assignments in favor of decorator pattern
- Maintain backward compatibility and test isolation
@goodtune
Copy link
Collaborator Author

goodtune commented Sep 2, 2025

All three feedback points have been addressed:

1. Settings modification in test setup - Fixed by replacing direct settings modification with proper @override_settings decorator to prevent test isolation issues.

2. getattr fallback pattern clarity - Improved with explicit comments explaining when cursor_for_search_path would have a cursor attribute (psycopg2) vs when it wouldn't (psycopg3), and replaced getattr with explicit hasattr check for better readability.

3. Parameter check explicitness - Made the condition more explicit by changing if not params to if params is None or len(params) == 0 for clearer intent.

All tests continue to pass with both psycopg2 and psycopg3 drivers after these improvements.

@goodtune goodtune requested a review from Copilot September 2, 2025 09:43
Copilot

This comment was marked as outdated.

- Extract complex boolean expression into `_should_set_search_path()` helper method
- Extract cursor compatibility logic into `_get_raw_cursor()` helper method
- Replace fragile `__bases__[0]` usage with explicit parent class references
- Improve code maintainability and testability through better method organization

Changes:
- Add `_should_set_search_path()` method with clear documentation
- Add `_get_raw_cursor()` method for psycopg2/psycopg3 compatibility
- Use `django.db.utils.load_backend()` for explicit parent class references
- Maintain test coverage and functionality across both PostgreSQL drivers
@goodtune
Copy link
Collaborator Author

goodtune commented Sep 2, 2025

Addressed the additional code quality feedback from the latest review:

1. Complex boolean expression - Extracted into well-named _should_set_search_path helper method with clear documentation explaining when search_path needs to be set.

2. Cursor compatibility logic - Extracted into _get_raw_cursor helper method to improve testability and reduce complexity in the main flow.

3. & 4. Fragile inheritance references - Replaced __bases__[0] usage with explicit parent class references using django.db.utils.load_backend for robustness against inheritance changes.

These refactoring improvements enhance code maintainability and testability while preserving full functionality across both psycopg2 and psycopg3 drivers.

@goodtune goodtune requested a review from Copilot September 2, 2025 09:54
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR fixes a RecursionError that occurs when using the psycopg3 PostgreSQL driver with Django's DEBUG=True setting. The bug was caused by Django's SQL debug logging triggering psycopg3's mogrify operation, which opens fresh cursors that re-enter the _cursor() method during search_path setting, creating infinite recursion.

  • Implements ContextVar-based recursion prevention during search_path operations
  • Switches from raw SQL formatting to parameterized queries using set_config()
  • Adds smart caching to avoid redundant search_path operations
  • Optimizes last_executed_query to skip unnecessary mogrify calls

Reviewed Changes

Copilot reviewed 9 out of 9 changed files in this pull request and generated 3 comments.

Show a summary per file
File Description
src/tenant_schemas/postgresql_backend/base.py Core fix implementing recursion prevention, parameterized queries, and caching optimizations
src/tenant_schemas/tests/test_psycopg3_recursion.py Comprehensive test suite covering recursion prevention and caching behavior
src/tenant_schemas/tests/testcases.py Refactors test settings management using @override_settings decorator
src/tenant_schemas/tests/test_tenants.py Updates test classes to use @override_settings instead of direct settings modification
src/tenant_schemas/tests/test_routes.py Removes redundant settings configuration
src/tenant_schemas/tests/test_apps.py Fixes f-string formatting in warning message
src/tenant_schemas/apps.py Fixes f-string formatting in warning message
dts_test_project/dts_test_app/migrations/*.py Removes unused imports

Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

- Cache Django backend loading at class level to avoid repeated loading in tests
- Replace potentially fragile len(params) check with more robust not params condition
- Improve test performance by eliminating redundant backend loading operations

Changes:
- Add `_original_backend` class attribute to cache loaded backend
- Update test methods to use cached backend instead of loading each time
- Replace `params is None or len(params) == 0` with simple `not params`
- Maintain full functionality while improving performance and robustness
@goodtune
Copy link
Collaborator Author

goodtune commented Sep 2, 2025

Addressed the final round of performance and robustness feedback:

1. & 2. Backend loading inefficiency - Added class-level backend cache to avoid repeated Django backend loading in test methods, improving test performance.

3. Potential exception with len(params) - Replaced the potentially fragile params check with the more robust not params condition that safely handles all falsy values and different container types without risk of exceptions.

These optimizations improve both performance and code robustness while maintaining full functionality across both PostgreSQL drivers.

@goodtune goodtune changed the title Fix RecursionError with psycopg3 when DEBUG=True Fix RecursionError with psycopg when DEBUG=True Sep 2, 2025
@goodtune goodtune merged commit 84b7b1a into bernardopires:master Sep 2, 2025
26 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant