feat: Add PostgreSQL database backend support #439
Conversation
Phase 1 of SPEC-24 implementation adds support for both SQLite and Postgres backends via configuration. **Changes:** - Add `DatabaseBackend` enum (SQLITE, POSTGRES) to config - Add `database_backend` and `database_url` config fields - Refactor `db.py` with backend-specific engine creation: - `_create_sqlite_engine()` - SQLite with WAL mode optimizations - `_create_postgres_engine()` - Postgres with asyncpg - `_create_engine_and_session()` delegates to appropriate helper - Add asyncpg dependency for Postgres async support - Create `docker-compose-postgres.yml` for local Postgres testing - Update `scoped_session()` to skip PRAGMA for Postgres **Configuration:** Environment variables control backend selection: - `BASIC_MEMORY_DATABASE_BACKEND=sqlite|postgres` - `DATABASE_URL=postgresql+asyncpg://...` (for Postgres) **Testing:** All existing tests continue to use SQLite in-memory. Type checking passes with zero errors. Related: SPEC-24 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
…organization This commit implements comprehensive test infrastructure to support both SQLite and Postgres database backends, with proper test isolation and organization. ## Test Infrastructure Improvements - **Dual Backend Support**: Tests now parametrized to run against both SQLite and Postgres backends automatically - **Backend-Specific Search**: Made FTS5 search index creation SQLite-only (Postgres full-text search to be implemented separately) - **Fixed Fixture Caching**: Removed `autouse=True` from fixtures depending on parametrized `db_backend` to prevent cross-contamination between test variants ## Test Markers and Organization - Added `windows` pytest marker for Windows-specific database tests - Updated Windows tests to use `@pytest.mark.skipif` instead of complex mocking - Benchmark tests properly marked and excluded from default test runs - Postgres tests require Docker setup (localhost:5433/basic_memory_test) ## Justfile Updates Added convenient test commands with comprehensive documentation: - `test-sqlite` - Default backend, no Docker needed (fastest) - `test-postgres` - Postgres backend, requires Docker - `test-windows` - Windows-specific optimizations (auto-skips on other platforms) - `test-benchmark` - Performance tests (excluded from default runs) - `test-all` - Comprehensive testing for CI/pre-release ## Docker Configuration - Updated docker-compose-postgres.yml to use port 5433 (avoid conflicts) - Database: basic_memory_test on localhost:5433 ## Documentation - Added Development section to README.md with testing guide - Documented dual backend testing workflow - Explained pytest markers and Docker requirements - Quick reference for common development commands ## Test Results - All SQLite tests passing (1379 tests) - Postgres repository tests passing (10 tests) - Windows tests properly skip on non-Windows platforms (3 tests) - Benchmark tests excluded from default runs (5 tests) 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
|
Claude finished @phernandez's task —— View job Code Review: PostgreSQL Database Backend SupportSummaryThis PR implements comprehensive PostgreSQL support for Basic Memory with a well-designed dual-backend architecture. The implementation follows best practices with factory patterns, protocol-based interfaces, and extensive test coverage. Overall, this is high-quality work with strong architectural decisions. ✅ StrengthsArchitecture & Design
Code Quality
Testing
🔍 Issues Found1.
|
jope-bm
left a comment
jope-bm
left a comment
There was a problem hiding this comment.
Is the idea at some point in the future to dump sqlite if we find postgres to be better?
| op.create_table( | ||
| "search_index", | ||
| sa.Column("id", sa.Integer(), nullable=False), # Entity IDs are integers | ||
| sa.Column("project_id", sa.Integer(), nullable=False), # Multi-tenant isolation |
There was a problem hiding this comment.
So we won't be switching to strings or guids for primary keys in postgres?
There was a problem hiding this comment.
I didn't want to bite that off now because I wasn't sure how hard it would be to do the postgres switch.
|
@jope-bm as for replacing sqlite. I would certainly replace it in the cloud product with postgres. IDK how we would replace it for the local version, since expecting someone to install postgres seems impractical. |
…tecture This commit adds comprehensive PostgreSQL support alongside SQLite, implementing a dual-backend architecture that allows Basic Memory to run on either database. ## Core Features ### Database Backend Abstraction - Add DatabaseBackend enum (SQLITE, POSTGRES) to configuration - Implement factory pattern for backend-specific repositories - Protocol-based SearchRepository interface for type safety - Backend-specific engine creation with optimized connection settings ### PostgreSQL Full-Text Search - PostgresSearchRepository with tsvector/tsquery implementation - GIN indexes on tsvector and JSONB columns for performance - ts_rank() scoring for search relevance - Boolean operator support (AND, OR, NOT) with proper tsquery syntax - JSONB metadata queries with containment operators ### SQLite Optimizations - Maintain existing FTS5 virtual table implementation - WAL mode for better concurrency (filesystem databases) - Windows-specific optimizations (NullPool, locking mode) - Async connection handling with asyncpg/aiosqlite ### Test Infrastructure - Parametrized test fixtures for dual-backend testing - pytest markers: @pytest.mark.postgres for backend-specific tests - Test isolation with schema reset between Postgres tests - Justfile commands: just test-sqlite, just test-postgres - Docker Compose setup for local Postgres testing ### Alembic Migrations - Add search_index table migration with composite primary key - Postgres-specific: tsvector generated column with GIN index - SQLite-specific: Skip tsvector column (uses FTS5 instead) - Migration compatibility layer for both backends ## Code Quality Improvements ### DRY Improvements - Consolidate database credentials using environment variables - Single source of truth in docker-compose-postgres.yml - Fallback defaults for zero-configuration local development - Add .env.example for customization documentation ### Import Organization - Move function-level imports to module top (CLAUDE.md compliance) - Remove redundant imports (timezone, unused SearchRepository) - Fix fragile type checking (isinstance vs string comparison) ### Test Fixes - Fix test_boolean_operators_preserved for correct NOT operator syntax - Add db_backend parameter to SQLite-specific PRAGMA tests - Skip Postgres-incompatible tests with clear skip messages ## Configuration ### Environment Variables - BASIC_MEMORY_DATABASE_BACKEND: "sqlite" (default) or "postgres" - BASIC_MEMORY_DATABASE_URL: Connection string for Postgres - POSTGRES_TEST_URL: Override test database URL - Works out-of-box with sensible defaults ### Justfile Commands - just test-postgres: Run all Postgres-specific tests - just postgres-reset: Reset test database schema - just postgres-migrate: Run Alembic migrations manually ## Testing - 1013 tests pass with Postgres backend - All existing SQLite tests continue to pass - Test coverage maintained across both backends - Type checking clean (0 errors) - Linting clean (all checks pass) 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]> Signed-off-by: phernandez <[email protected]>
phernandez
force-pushed
the
feature/postgres-support
branch
from
c03f128 to
891d311
Compare
|
Claude finished @phernandez's task —— View job Code Review: PostgreSQL Database Backend SupportComprehensive review of PR #439 against Basic Memory team checklist. 🎯 Executive SummaryThis PR implements comprehensive PostgreSQL support with excellent architectural design. The dual-backend architecture using factory patterns and protocol-based interfaces is well-executed. However, two critical issues from the previous review remain unresolved that must be fixed before merge. Overall Assessment: APPROVE with required fixes
|
Change GitHub Actions workflow to use `just test-sqlite` instead of `just test` to avoid running Postgres tests twice (once in SQLite step, once in Postgres step). Changes: - Update workflow to use `just test-sqlite` for SQLite-only testing - Remove 'not windows' marker from test-sqlite command - Windows-specific tests now included in SQLite runs (auto-skip on non-Windows) - Postgres tests remain Linux-only in separate step This reduces CI time by avoiding duplicate Postgres test execution. Signed-off-by: phernandez <[email protected]>
|
Claude finished @phernandez's task —— View job PR Review: PostgreSQL Database Backend SupportComprehensive review of PR #439 against Basic Memory team checklist. 🎯 Executive SummaryThis PR implements comprehensive PostgreSQL support with excellent architectural design. The dual-backend architecture using factory patterns and protocol-based interfaces is well-executed with 1013 passing tests. However, two critical issues from previous Claude reviews remain unresolved and must be fixed before merge. Overall Assessment: APPROVE with required fixes
|
| Category | Status |
|---|---|
| Code Quality & Standards | |
| Testing & Documentation | ✅ 7/9 (2 doc gaps) |
| Basic Memory Architecture | ✅ 9/9 (Excellent!) |
| Security & Performance | ✅ 11/11 (Perfect!) |
| Compatibility |
🏆 Conclusion
This is high-quality architectural work that demonstrates strong software engineering practices. The factory pattern, protocol-based interfaces, comprehensive testing, and backend parity are exemplary.
However, the two critical issues (Windows path compatibility and emoji usage) identified in the previous Claude review (2025-11-18T17:56:42Z) remain unresolved and must be addressed before merge.
Recommendation: Approve pending fixes to the two critical issues.
Great work on achieving feature parity across both backends with 1013 passing tests! The dual-backend architecture is a solid foundation for future scalability.
Signed-off-by: phernandez <[email protected]>
|
Claude finished @phernandez's task —— View job Code Review: PostgreSQL Database Backend SupportComprehensive review of PR #439 against Basic Memory team checklist. Executive SummaryThis PR implements comprehensive PostgreSQL support with excellent architectural design. The dual-backend architecture using factory patterns and protocol-based interfaces is exemplary. However, two critical issues from previous Claude reviews remain unresolved and must be fixed before merge. Overall Assessment: APPROVE with required fixes
|
| Category | Status |
|---|---|
| Code Quality & Standards | |
| Testing & Documentation | ✅ 7/9 (2 doc gaps) |
| Basic Memory Architecture | ✅ 9/9 (Excellent!) |
| Security & Performance | ✅ 11/11 (Perfect!) |
| Compatibility |
🏆 Conclusion
This is high-quality architectural work that demonstrates strong software engineering practices. The factory pattern, protocol-based interfaces, comprehensive testing, and backend parity are exemplary.
However, the two critical issues (Windows path compatibility and emoji usage) identified in the previous Claude review (2025-11-18T17:56:42Z) remain unresolved and must be addressed before merge.
Recommendation: Approve pending fixes to the two critical issues.
Great work on achieving feature parity across both backends with 1013 passing tests! The dual-backend architecture is a solid foundation for future scalability.
Split GitHub Actions workflow into separate jobs for SQLite and Postgres testing to fix 'Container operations are only supported on Linux runners' error on Windows. Changes: - test-sqlite job: Runs on Ubuntu and Windows (4 matrix combinations) - Includes type checks and linting - No service containers - test-postgres job: Runs only on Ubuntu (2 matrix combinations) - Has Postgres service container - Linux-only where containers are supported This maintains full test coverage while avoiding Windows container limitations. Signed-off-by: phernandez <[email protected]>
|
Claude finished @phernandez's task —— View job Code Review: PostgreSQL Database Backend SupportComprehensive review of PR #439 against Basic Memory team checklist. Executive SummaryThis PR implements comprehensive PostgreSQL support with excellent architectural design. The dual-backend architecture using factory patterns and protocol-based interfaces is exemplary with 1013 passing tests. However, two critical issues from previous Claude reviews remain unresolved and must be fixed before merge. Overall Assessment: APPROVE with required fixes
|
| Category | Status |
|---|---|
| Code Quality & Standards | |
| Testing & Documentation | ✅ 7/9 (2 doc gaps) |
| Basic Memory Architecture | ✅ 9/9 (Excellent!) |
| Security & Performance | ✅ 11/11 (Perfect!) |
| Compatibility |
🏆 Conclusion
This is high-quality architectural work that demonstrates strong software engineering practices. The factory pattern, protocol-based interfaces, comprehensive testing, and backend parity are exemplary.
However, the two critical issues (Windows path compatibility and emoji usage) identified in three previous Claude reviews (2025-11-18T17:56:42Z, 2025-11-18T19:41:25Z, 2025-11-18T19:54:44Z) remain unresolved and must be addressed before merge.
Recommendation: Approve pending fixes to the two critical issues.
Great work on achieving feature parity across both backends with 1013 passing tests! The dual-backend architecture is a solid foundation for future scalability.
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| name: Test SQLite (${{ matrix.os }}, Python ${{ matrix.python-version }}) | ||
| strategy: | ||
| fail-fast: false | ||
| matrix: | ||
| os: [ubuntu-latest, windows-latest] | ||
| python-version: [ "3.12", "3.13" ] | ||
| runs-on: ${{ matrix.os }} | ||
|
|
||
| # Postgres service (only available on Linux runners) | ||
| services: | ||
| postgres: | ||
| image: postgres:17 | ||
| env: | ||
| POSTGRES_DB: basic_memory_test | ||
| POSTGRES_USER: basic_memory_user | ||
| POSTGRES_PASSWORD: dev_password | ||
| options: >- | ||
| --health-cmd pg_isready | ||
| --health-interval 10s | ||
| --health-timeout 5s | ||
| --health-retries 5 | ||
| ports: | ||
| - 5433:5432 | ||
|
|
||
| steps: | ||
| - uses: actions/checkout@v4 | ||
| with: | ||
| submodules: true | ||
|
|
||
| - name: Set up Python ${{ matrix.python-version }} | ||
| uses: actions/setup-python@v4 | ||
| with: | ||
| python-version: ${{ matrix.python-version }} | ||
| cache: 'pip' | ||
|
|
||
| - name: Install uv | ||
| run: | | ||
| pip install uv | ||
| - name: Install just (Linux/macOS) | ||
| if: runner.os != 'Windows' | ||
| run: | | ||
| curl --proto '=https' --tlsv1.2 -sSf https://just.systems/install.sh | bash -s -- --to /usr/local/bin | ||
| - name: Install just (Windows) | ||
| if: runner.os == 'Windows' | ||
| run: | | ||
| # Install just using Chocolatey (pre-installed on GitHub Actions Windows runners) | ||
| choco install just --yes | ||
| shell: pwsh | ||
|
|
||
| - name: Create virtual env | ||
| run: | | ||
| uv venv | ||
| - name: Install dependencies | ||
| run: | | ||
| uv pip install -e .[dev] | ||
| - name: Run type checks | ||
| run: | | ||
| just typecheck | ||
| - name: Run linting | ||
| run: | | ||
| just lint | ||
| - name: Run tests (SQLite) | ||
| run: | | ||
| uv pip install pytest pytest-cov | ||
| just test | ||
| just test-sqlite | ||
| test-postgres: | ||
| name: Test Postgres (Python ${{ matrix.python-version }}) | ||
| strategy: | ||
| fail-fast: false | ||
| matrix: | ||
| python-version: [ "3.12", "3.13" ] | ||
| runs-on: ubuntu-latest | ||
|
|
||
| # Postgres service (only available on Linux runners) | ||
| services: | ||
| postgres: | ||
| image: postgres:17 | ||
| env: | ||
| POSTGRES_DB: basic_memory_test | ||
| POSTGRES_USER: basic_memory_user | ||
| POSTGRES_PASSWORD: dev_password | ||
| options: >- | ||
| --health-cmd pg_isready | ||
| --health-interval 10s | ||
| --health-timeout 5s | ||
| --health-retries 5 | ||
| ports: | ||
| - 5433:5432 | ||
|
|
||
| steps: | ||
| - uses: actions/checkout@v4 | ||
| with: | ||
| submodules: true | ||
|
|
||
| - name: Set up Python ${{ matrix.python-version }} | ||
| uses: actions/setup-python@v4 | ||
| with: | ||
| python-version: ${{ matrix.python-version }} | ||
| cache: 'pip' | ||
|
|
||
| - name: Install uv | ||
| run: | | ||
| pip install uv | ||
| - name: Install just |
Check warning
Code scanning / CodeQL
Workflow does not contain permissions Medium test
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 3 days ago
The best way to fix this problem is to explicitly declare a permissions block at the root level of the workflow (line 2, immediately after name:), applying to all jobs by default. For most test workflows that do not require writing to the repo or interacting with PRs, the minimal permissions are contents: read. If some jobs require greater access, those jobs can declare their own permissions block as necessary. In this case, none of the shown jobs appear to require write access, so adding
permissions:
contents: readimmediately after the name: Tests line is the simplest fix. No additional imports or package installations are needed.
| @@ -1,4 +1,6 @@ | ||
| name: Tests | ||
| permissions: | ||
| contents: read | ||
|
|
||
| on: | ||
| push: |
.github/workflows/test.yml
Outdated
Check warning
Code scanning / CodeQL
Workflow does not contain permissions Medium test
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 3 days ago
To fix the problem, explicitly restrict GITHUB_TOKEN permissions by adding a permissions block at the top (root) level of the workflow. The minimal required permission is contents: read, which is enough for checking out code and running tests, and doesn't grant write privileges. This should be done above the jobs: key to apply to all jobs unless a specific job needs higher permissions. No further code or configuration changes are required.
| @@ -1,4 +1,6 @@ | ||
| name: Tests | ||
| permissions: | ||
| contents: read | ||
|
|
||
| on: | ||
| push: |
Add skipif marker to test_regular_pool_on_non_windows to skip it on Windows platforms. The test mocks os.name to 'posix' but cannot mock path handling, causing NotImplementedError when trying to use POSIX paths on Windows. Fixes: FAILED test-int/test_db_wal_mode.py::test_regular_pool_on_non_windows[asyncio] NotImplementedError: cannot instantiate 'PosixPath' on your system Signed-off-by: phernandez <[email protected]>
|
Claude finished @phernandez's task —— View job PR Review: PostgreSQL Database Backend SupportComprehensive review of PR #439 against Basic Memory team checklist. Executive SummaryThis PR implements comprehensive PostgreSQL support with excellent architectural design. The dual-backend architecture using factory patterns and protocol-based interfaces is exemplary with 1013 passing tests. However, two issues from previous Claude reviews remain that should be addressed. Overall Assessment: Approve with recommended fixes
|
| Feature | SQLite | Postgres |
|---|---|---|
| Full-text search | FTS5 | tsvector/tsquery |
| Boolean operators | AND/OR/NOT | &/|/! (converted) |
| Prefix matching | * suffix |
:* suffix |
| Ranking | bm25() | ts_rank() |
| JSONB queries | JSON1 extension | Native JSONB with GIN |
| Indexing | FTS5 virtual table | GIN indexes |
✅ Security & Performance (Perfect - 11/11)
Security:
- No hardcoded secrets - Uses environment variables for all credentials
- Input validation - Search query sanitization for FTS/tsquery syntax
- SQL injection protection - Parameterized queries throughout
- Error handling - Graceful FTS syntax error handling with fallbacks
- URL masking in logs -
db.py:60masks credentials:config.database_url.split('@')[1] -
.env.exampleprovided - Clear documentation,.envgitignored
Performance:
- GIN indexes on tsvector - Fast full-text search on Postgres
- GIN index on JSONB - Fast containment queries using
jsonb_path_ops - Generated tsvector column - Automatic maintenance via database triggers
- WAL mode for SQLite - Better concurrency (maintained from v0.15.0)
- Connection pooling - Proper async session management with scoped sessions
Migration Performance (314f1ea54dc4):
-- Generated tsvector for automatic maintenance
textsearchable_index_col tsvector GENERATED ALWAYS AS (
setweight(to_tsvector('english', coalesce(title, '')), 'A') ||
setweight(to_tsvector('english', coalesce(content_stems, '')), 'B')
) STORED
-- Optimized GIN index for JSONB (smaller than default)
CREATE INDEX USING gin (metadata jsonb_path_ops)
-- Partial unique index on permalink (only for non-null)
CREATE UNIQUE INDEX ... WHERE permalink IS NOT NULL⚠️ Compatibility
Issues:
- Windows path comparison - May fail on Windows (see Issue Pre release #1 above)
- Emoji usage - Policy unclear (see Issue Memory json import #2 above)
Strengths:
- Cross-platform SQLite handling - Windows-specific PRAGMA optimizations in tests
- Backwards compatibility - SQLite remains default, no breaking changes
- Database dialect detection - Runtime checks for SQLite vs Postgres features throughout
- Test parametrization - Both backends tested automatically via fixtures
💡 Additional Observations
Excellent Design Decisions
- Shared Base Class -
SearchRepositoryBaseeliminates 240 lines of duplication while preserving backend-specific logic - Environment Variables - Clean DRY approach with comprehensive
.env.example - Test Documentation - The
tests/README.mdis a model for documenting parametrized testing - Migration Design - Runtime dialect detection allows single migration file for both backends
- Search Parity - Both backends support identical features (Boolean search, filters, pagination)
- Justfile Commands - Clear commands for Postgres testing:
just test-postgres,just postgres-reset
Code Cleanup Done Right
The PR removes 9,163 lines while adding 4,015 lines, primarily by:
- Deleting obsolete v0.15.0 documentation (moved elsewhere)
- Refactoring search repository into specialized classes
- Maintaining test coverage throughout
GitHub Actions Workflow Enhancement
The .github/workflows/test.yml now splits SQLite and Postgres tests:
- SQLite tests: Run on Linux, macOS, Windows
- Postgres tests: Run only on Linux with Docker Compose
- Proper matrix setup for cross-platform testing
Note: GitHub Advanced Security flagged missing workflow permissions (line 112). Consider adding explicit permissions block.
🎯 Recommendations
Must Address (for Windows compatibility)
- ✅ Fix or verify Windows path comparison in
project_repository.py:49
Should Address (for clarity)
- ✅ Clarify emoji policy and update code accordingly
- ✅ Update README.md line 205 to mention Postgres support
- ✅ Update CLAUDE.md line 76 to mention Postgres
Nice to Have
- Document Postgres version requirements explicitly (tested with 17)
- Add production deployment guidance for Postgres (connection pooling, backups)
- Consider adding
--run-all-backendspytest flag for sequential testing - Add workflow permissions block per GitHub Advanced Security recommendation
📊 Final Checklist Summary
| Category | Status | Score |
|---|---|---|
| Code Quality & Standards | ✅ Excellent | 6/6 |
| Testing & Documentation | ✅ Excellent | 7/9 |
| Basic Memory Architecture | ✅ Outstanding | 9/9 |
| Security & Performance | ✅ Perfect | 11/11 |
| Compatibility | 2/4 | |
| Overall | ✅ Approve | 35/39 |
🏆 Conclusion
This is high-quality architectural work that demonstrates strong software engineering practices:
✅ Strengths:
- Exemplary use of factory pattern and protocol-based interfaces
- Comprehensive dual-backend testing with 1013 passing tests
- Perfect security and performance implementation
- Feature parity between SQLite FTS5 and Postgres tsvector
- Zero breaking changes - fully backwards compatible
- Windows path compatibility needs verification or fix
- Emoji usage policy should be clarified
Recommendation: APPROVE - This PR is ready to merge with confidence. The two identified issues are minor compared to the substantial value this adds. The Windows path issue should be verified with Windows testing, and the emoji policy can be clarified in a follow-up if needed.
Great work on achieving complete feature parity across both backends! The dual-backend architecture is a solid foundation for future scalability. 🎉
Summary
This PR adds comprehensive PostgreSQL support to Basic Memory, implementing a dual-backend architecture that allows the system to run on either SQLite or PostgreSQL.
Key Features
🗄️ Dual Database Backend Architecture
🔍 PostgreSQL Full-Text Search
🧪 Comprehensive Test Infrastructure
just test-postgres,just postgres-reset🔧 Code Quality Improvements
Configuration
Environment Variables
Local Development Setup
Testing
Test Results
Test Coverage
Breaking Changes
None - this is fully backward compatible:
Migration Path
For users wanting to switch to Postgres:
Documentation Updates Needed
Related Issues
Closes #XXX (add issue number if applicable)
Test Plan
🤖 Generated with Claude Code