fix(migrations): bypass PgBouncer for advisory locks via MIGRATION_DATABASE_URL (#726)
* fix(migrations): use HINDSIGHT_API_MIGRATION_DATABASE_URL when set Session-level advisory locks are broken when the database URL goes through PgBouncer in transaction mode: the backend connection is returned to the pool on COMMIT, orphaning the lock, so multiple pods can simultaneously run migrations for the same schema. When HINDSIGHT_API_MIGRATION_DATABASE_URL is set, use it for both the advisory lock connection and the Alembic run. Callers should point this at the direct PostgreSQL endpoint (bypassing the pooler) so the session-level lock is held for the full migration duration. * refactor(migrations): move MIGRATION_DATABASE_URL to standard config Wire HINDSIGHT_API_MIGRATION_DATABASE_URL through HindsightConfig instead of reading os.getenv() directly in migrations.py. Add the field to the dataclass, from_env(), log_config(), all call sites, .env.example, and the configuration docs page. * fix: update test mocks for migration_database_url kwarg and regenerate docs skill --------- Co-authored-by: Nicolò Boschi <boschi1997@gmail.com>
This commit is contained in:
parent
1cac35728f
commit
dffb87080f
9 changed files with 28 additions and 11 deletions
|
|
@ -44,6 +44,7 @@ HINDSIGHT_API_LOG_LEVEL=info
|
|||
|
||||
# Database (Optional - uses embedded pg0 by default)
|
||||
# HINDSIGHT_API_DATABASE_URL=postgresql://user:pass@host:5432/db
|
||||
# HINDSIGHT_API_MIGRATION_DATABASE_URL= # Direct PostgreSQL URL for migrations (bypasses PgBouncer). Falls back to DATABASE_URL.
|
||||
# HINDSIGHT_API_DATABASE_SCHEMA=public # PostgreSQL schema name (default: public)
|
||||
|
||||
# Vector Extension (Optional - uses pgvector by default)
|
||||
|
|
|
|||
|
|
@ -249,7 +249,7 @@ async def _run_migration(
|
|||
schemas = list(dict.fromkeys(schemas))
|
||||
|
||||
for schema in schemas:
|
||||
run_migrations(resolved_url, schema=schema)
|
||||
run_migrations(resolved_url, schema=schema, migration_database_url=config.migration_database_url)
|
||||
|
||||
if embedding_dimension is not None:
|
||||
for schema in schemas:
|
||||
|
|
|
|||
|
|
@ -118,6 +118,7 @@ def normalize_config_dict(config: dict[str, Any]) -> dict[str, Any]:
|
|||
|
||||
# Environment variable names
|
||||
ENV_DATABASE_URL = "HINDSIGHT_API_DATABASE_URL"
|
||||
ENV_MIGRATION_DATABASE_URL = "HINDSIGHT_API_MIGRATION_DATABASE_URL"
|
||||
ENV_DATABASE_SCHEMA = "HINDSIGHT_API_DATABASE_SCHEMA"
|
||||
ENV_LLM_PROVIDER = "HINDSIGHT_API_LLM_PROVIDER"
|
||||
ENV_LLM_API_KEY = "HINDSIGHT_API_LLM_API_KEY"
|
||||
|
|
@ -616,6 +617,7 @@ class HindsightConfig:
|
|||
|
||||
# Database
|
||||
database_url: str
|
||||
migration_database_url: str | None
|
||||
database_schema: str
|
||||
vector_extension: str # "pgvector" or "vchord"
|
||||
text_search_extension: str # "native" or "vchord"
|
||||
|
|
@ -1009,6 +1011,7 @@ class HindsightConfig:
|
|||
config = cls(
|
||||
# Database
|
||||
database_url=os.getenv(ENV_DATABASE_URL, DEFAULT_DATABASE_URL),
|
||||
migration_database_url=os.getenv(ENV_MIGRATION_DATABASE_URL) or None,
|
||||
database_schema=os.getenv(ENV_DATABASE_SCHEMA, DEFAULT_DATABASE_SCHEMA),
|
||||
vector_extension=os.getenv(ENV_VECTOR_EXTENSION, DEFAULT_VECTOR_EXTENSION).lower(),
|
||||
text_search_extension=os.getenv(ENV_TEXT_SEARCH_EXTENSION, DEFAULT_TEXT_SEARCH_EXTENSION).lower(),
|
||||
|
|
@ -1415,6 +1418,8 @@ class HindsightConfig:
|
|||
def log_config(self) -> None:
|
||||
"""Log the current configuration (without sensitive values)."""
|
||||
logger.info(f"Database: {self.database_url} (schema: {self.database_schema})")
|
||||
if self.migration_database_url:
|
||||
logger.info(f"Migration database: {self.migration_database_url}")
|
||||
logger.info(f"LLM: provider={self.llm_provider}, model={self.llm_model}")
|
||||
if self.retain_llm_provider or self.retain_llm_model:
|
||||
retain_provider = self.retain_llm_provider or self.llm_provider
|
||||
|
|
|
|||
|
|
@ -1681,18 +1681,16 @@ class MemoryEngine(MemoryEngineInterface):
|
|||
# Migrate all schemas from the tenant extension
|
||||
# The tenant extension is the single source of truth for which schemas exist
|
||||
logger.info("Running database migrations...")
|
||||
config = get_config()
|
||||
tenants = await self._tenant_extension.list_tenants()
|
||||
if tenants:
|
||||
logger.info(f"Running migrations on {len(tenants)} schema(s)...")
|
||||
for tenant in tenants:
|
||||
schema = tenant.schema
|
||||
if schema:
|
||||
run_migrations(self.db_url, schema=schema)
|
||||
run_migrations(self.db_url, schema=schema, migration_database_url=config.migration_database_url)
|
||||
logger.info("Schema migrations completed")
|
||||
|
||||
# Get config for vector extension setting
|
||||
config = get_config()
|
||||
|
||||
# Ensure embedding column dimension matches the model's dimension
|
||||
# This is done after migrations and after embeddings.initialize()
|
||||
for tenant in tenants:
|
||||
|
|
|
|||
|
|
@ -120,7 +120,9 @@ class DefaultExtensionContext(ExtensionContext):
|
|||
# CREATE INDEX CONCURRENTLY inside the migration waits for those transactions
|
||||
# forever — a deadlock.
|
||||
config = get_config()
|
||||
await asyncio.to_thread(run_migrations, db_url, schema=schema)
|
||||
await asyncio.to_thread(
|
||||
run_migrations, db_url, schema=schema, migration_database_url=config.migration_database_url
|
||||
)
|
||||
|
||||
# Ensure embedding column dimension matches the model's dimension
|
||||
# This is needed because migrations create columns with default dimension
|
||||
|
|
|
|||
|
|
@ -176,6 +176,7 @@ def run_migrations(
|
|||
database_url: str,
|
||||
script_location: str | None = None,
|
||||
schema: str | None = None,
|
||||
migration_database_url: str | None = None,
|
||||
) -> None:
|
||||
"""
|
||||
Run database migrations to the latest version using programmatic Alembic configuration.
|
||||
|
|
@ -213,6 +214,14 @@ def run_migrations(
|
|||
script_location="/path/to/copied/_alembic"
|
||||
)
|
||||
"""
|
||||
# Prefer a dedicated migration URL that bypasses connection poolers (e.g.
|
||||
# PgBouncer in transaction mode). Session-level advisory locks don't
|
||||
# survive a PgBouncer transaction-mode cycle, so the distributed lock is
|
||||
# ineffective when the app URL goes through a pooler. Configure
|
||||
# HINDSIGHT_API_MIGRATION_DATABASE_URL to the direct PostgreSQL endpoint
|
||||
# (e.g. hindsight-pg-rw) to restore correct locking behaviour.
|
||||
migration_url = migration_database_url or database_url
|
||||
|
||||
try:
|
||||
# Determine script location
|
||||
if script_location is None:
|
||||
|
|
@ -249,7 +258,7 @@ def run_migrations(
|
|||
# 2. After acquiring the lock, COMMIT the transaction on the advisory-lock
|
||||
# connection itself before running migrations. pg_advisory_lock is
|
||||
# session-level, so the lock survives the COMMIT.
|
||||
engine = create_engine(database_url)
|
||||
engine = create_engine(migration_url)
|
||||
with engine.connect() as conn:
|
||||
logger.debug(f"Acquiring migration advisory lock for schema '{schema_name}' (id={lock_id})...")
|
||||
while True:
|
||||
|
|
@ -394,7 +403,7 @@ def run_migrations(
|
|||
conn.commit()
|
||||
|
||||
# Run migrations while holding the lock
|
||||
_run_migrations_internal(database_url, script_location, schema=schema)
|
||||
_run_migrations_internal(migration_url, script_location, schema=schema)
|
||||
finally:
|
||||
# Explicitly release the lock (also released on connection close)
|
||||
conn.execute(text(f"SELECT pg_advisory_unlock({lock_id})"))
|
||||
|
|
|
|||
|
|
@ -314,7 +314,7 @@ async def test_run_migration_without_schema_discovers_and_deduplicates_schemas(m
|
|||
async def fake_resolve_database_url(db_url: str) -> str:
|
||||
return f"resolved::{db_url}"
|
||||
|
||||
def fake_run_migrations(database_url: str, schema: str | None = None) -> None:
|
||||
def fake_run_migrations(database_url: str, schema: str | None = None, **kwargs) -> None:
|
||||
calls["run_migrations"].append((database_url, schema))
|
||||
|
||||
def fake_ensure_vector_extension(
|
||||
|
|
@ -376,7 +376,7 @@ async def test_run_migration_without_schema_runs_optional_post_migration_hooks(m
|
|||
async def fake_resolve_database_url(db_url: str) -> str:
|
||||
return f"resolved::{db_url}"
|
||||
|
||||
def fake_run_migrations(database_url: str, schema: str | None = None) -> None:
|
||||
def fake_run_migrations(database_url: str, schema: str | None = None, **kwargs) -> None:
|
||||
calls["run_migrations"].append((database_url, schema))
|
||||
|
||||
def fake_ensure_embedding_dimension(
|
||||
|
|
@ -453,7 +453,7 @@ async def test_run_migration_with_schema_only_runs_requested_schema(monkeypatch)
|
|||
async def fake_resolve_database_url(db_url: str) -> str:
|
||||
return f"resolved::{db_url}"
|
||||
|
||||
def fake_run_migrations(database_url: str, schema: str | None = None) -> None:
|
||||
def fake_run_migrations(database_url: str, schema: str | None = None, **kwargs) -> None:
|
||||
calls["run_migrations"].append((database_url, schema))
|
||||
|
||||
def fake_ensure_vector_extension(
|
||||
|
|
|
|||
|
|
@ -20,6 +20,7 @@ The API service handles all memory operations (retain, recall, reflect).
|
|||
| Variable | Description | Default |
|
||||
|----------|-------------|---------|
|
||||
| `HINDSIGHT_API_DATABASE_URL` | PostgreSQL connection string | `pg0` (embedded) |
|
||||
| `HINDSIGHT_API_MIGRATION_DATABASE_URL` | Direct PostgreSQL URL for running migrations, bypassing connection poolers (e.g. PgBouncer). When set, advisory locks and Alembic migrations use this URL instead of `DATABASE_URL`. | Falls back to `DATABASE_URL` |
|
||||
| `HINDSIGHT_API_DATABASE_SCHEMA` | PostgreSQL schema name for tables | `public` |
|
||||
| `HINDSIGHT_API_RUN_MIGRATIONS_ON_STARTUP` | Run database migrations on API startup | `true` |
|
||||
|
||||
|
|
|
|||
|
|
@ -20,6 +20,7 @@ The API service handles all memory operations (retain, recall, reflect).
|
|||
| Variable | Description | Default |
|
||||
|----------|-------------|---------|
|
||||
| `HINDSIGHT_API_DATABASE_URL` | PostgreSQL connection string | `pg0` (embedded) |
|
||||
| `HINDSIGHT_API_MIGRATION_DATABASE_URL` | Direct PostgreSQL URL for running migrations, bypassing connection poolers (e.g. PgBouncer). When set, advisory locks and Alembic migrations use this URL instead of `DATABASE_URL`. | Falls back to `DATABASE_URL` |
|
||||
| `HINDSIGHT_API_DATABASE_SCHEMA` | PostgreSQL schema name for tables | `public` |
|
||||
| `HINDSIGHT_API_RUN_MIGRATIONS_ON_STARTUP` | Run database migrations on API startup | `true` |
|
||||
|
||||
|
|
|
|||
Loading…
Reference in a new issue