Add database_name and host GUCs and peer authentication - #9
Add database_name and host GUCs and peer authentication#9Pino de Candia (pinodeca) wants to merge 2 commits into
Conversation
- Add pg_durable.host GUC for Unix socket directory configuration - Add pg_durable.database_name GUC to specify worker database - Extension creates duroxide schema with validation checks - Background worker waits for extension, then initializes duroxide tables - Use Unix domain sockets only, not TCP connections - Update CI workflow to use new GUCs for pg_regress tests Design documented in docs/dbname_and_host_gucs.md
There was a problem hiding this comment.
Pull request overview
This PR implements configurable database connection for pg_durable through two new GUCs (pg_durable.host and pg_durable.database_name) and switches to Unix domain socket connections with peer authentication. The changes enable the background worker to connect to a specific database without relying on environment variables or custom roles, and introduce a dynamic worker lifecycle that can handle DROP/CREATE EXTENSION cycles gracefully.
Changes:
- Added two postmaster-context GUCs for configurable Unix socket directory and target database
- Implemented peer authentication using OS-level credentials (no custom roles)
- Refactored background worker to dynamically monitor schema existence and manage runtime lifecycle
- Extension creation now validates configuration and creates empty duroxide schema (worker populates it)
- Comprehensive E2E tests for extension security, drop/recreate cycles, and worker restart behavior
Reviewed changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| src/lib.rs | Added GUC definitions and extension initialization validation SQL |
| src/types.rs | Implemented connection string builder using GUCs with peer authentication |
| src/worker.rs | Refactored to dynamic runtime lifecycle with schema monitoring loop |
| tests/e2e/sql/25_extension_creation_security.sql | Enhanced security tests for schema ownership and DROP CASCADE behavior |
| tests/e2e/sql/26_drop_create_loop.sql | New test for multiple drop-recreate cycles |
| scripts/pg-start.sh | Added optional database_name parameter for GUC configuration |
| scripts/test-e2e-local.sh | Updated to remove database_name GUC (uses default behavior) |
| test/regress/Makefile | Improved PG_CONFIG detection logic and made connection parameters environment-based |
| test/regress/README.md | Updated documentation for new GUC-based configuration |
| Makefile | Updated installcheck target with better environment variable guidance |
| .github/workflows/ci.yml | Updated pg_regress workflow comments (commented out) |
| docs/dbname_and_host_gucs.md | Comprehensive design documentation for GUC configuration and security model |
Comments suppressed due to low confidence (1)
test/regress/README.md:41
- Documentation states the worker retries connection "every 5 seconds" but the actual implementation in src/worker.rs line 180 uses a 1 second retry interval. Update the documentation to reflect the actual retry interval of 1 second.
**Retry logic:** If the database doesn't exist yet (common during startup), the worker retries the connection every 5 seconds until:
- The database is created by pg_regress, OR
- PostgreSQL shuts down
|
|
||
| # - name: Stop PostgreSQL after pg_regress | ||
| # if: steps.pg_regress.outcome != 'skipped' | ||
| # run: ./scripts/pg-reset.sh |
There was a problem hiding this comment.
Duplicate comment block detected. Lines 114-116 and 118-120 contain identical code with the same stop action comment. Remove the duplicate lines 118-120.
| # - name: Stop PostgreSQL after pg_regress | |
| # if: steps.pg_regress.outcome != 'skipped' | |
| # run: ./scripts/pg-reset.sh |
| if duroxide_runtime.is_some() && check_schema_or_tables_missing(&pg_pool).await { | ||
| log!("pg_durable: duroxide schema or tables dropped, stopping runtime..."); | ||
| duroxide_runtime | ||
| .take() | ||
| .unwrap() | ||
| .shutdown(Some(10_000)) | ||
| .await; | ||
| log!("pg_durable: duroxide runtime stopped"); | ||
| } | ||
|
|
||
| // Retry after 5 seconds | ||
| tokio::time::sleep(Duration::from_secs(5)).await; | ||
| // 3. If runtime is not initialized, check if we should start it | ||
| if duroxide_runtime.is_none() && check_schema_exists(&pg_pool).await { | ||
| log!("pg_durable: duroxide schema detected, initializing runtime..."); | ||
| duroxide_runtime = initialize_duroxide_runtime(pg_pool.clone()).await; | ||
| if duroxide_runtime.is_some() { | ||
| log!("pg_durable: duroxide runtime started, processing durable functions..."); | ||
| } | ||
| } |
There was a problem hiding this comment.
The comment states this check handles the edge case where "DROP EXTENSION CASCADE + CREATE EXTENSION happens within the check interval," but the implementation may not fully handle this. If DROP CASCADE and CREATE EXTENSION happen between line 106 and the schema existence check at line 117, the worker could:
- At line 106: Detect tables missing (DROP CASCADE occurred)
- Shutdown runtime at lines 108-113
- At line 117: Detect schema exists (CREATE EXTENSION occurred)
- At line 119: Try to initialize runtime with a now-present but possibly empty schema
This is fine if PostgresProvider can handle an existing but empty schema gracefully. However, consider adding a comment explaining that PostgresProvider will properly initialize the schema with tables even if the schema already exists but is empty.
| # Remove any existing pg_durable.database_name setting | ||
| sed -i '/^pg_durable\.database_name/d' "$PG_CONF" |
There was a problem hiding this comment.
The sed command uses in-place editing without a backup extension, which has different syntax on macOS vs Linux. On macOS, sed -i requires an extension argument (even if empty: sed -i ''). For consistency with scripts/test-e2e-local.sh which uses sed -i.bak, consider using the same portable approach here: sed -i.bak '/^pg_durable.database_name/d'.
| # Remove any existing pg_durable.database_name setting | |
| sed -i '/^pg_durable\.database_name/d' "$PG_CONF" | |
| # Remove any existing pg_durable.database_name setting (portable sed -i usage) | |
| sed -i.bak '/^pg_durable\.database_name/d' "$PG_CONF" && rm -f "$PG_CONF.bak" |
|
Copilot the unit tests are failing in CI. Propose a patch to this PR that fixes the unit tests. |
|
Pino de Candia (@pinodeca) I've opened a new pull request, #12, to work on those changes. Once the pull request is ready, I'll request review from you. |
* Fix unit tests by disabling background worker during tests * Fix E2E test race condition in extension drop/recreate tests Add 1-second delay after worker table initialization to avoid migration conflicts when client connects. The race condition occurred when: 1. Background worker initializes duroxide-pg tables 2. Client session calls df.start() and creates its own PostgresProvider 3. Both try to run migrations concurrently, causing duplicate key errors Tests fixed: 25_extension_creation_security, 26_drop_create_loop
|
Most of this change (except for Unix Domain Sockets) was merged with #30 |
Design documented in docs/dbname_and_host_gucs.md