Repository navigation
Conversation
Pin UPSTREAM_BASE_COMMIT=ad301df5be32963f53be707e5423ecdab11a7e5c and record the PostgreSQL single-engine facts for the tenant-schema work. - internal/db/testdata/tenant_schema_inventory/<base>/engine_matrix.json: engines=["postgresql"], canonical/query/sqlc paths, head 0105, no blockers. - internal/db/testdata/tenant_schema_inventory/<base>/BASE.md: base SHA, engine facts, migration rules summary, and the Cloud gitlink cross-ref (023cfc88, explicitly NOT a base). - TestUpstreamEngineMatrixPinned asserts a single full-SHA pinned dir, engines exactly ["postgresql"], referenced paths exist, and db/ has no non-postgres engine dir (second engine => stop and escalate).
Freeze the Phase 1 schema inventory at UPSTREAM_BASE_COMMIT so downstream tenantization tasks have a verified, checkable ground truth. - tables.json: 48 canonical tables + 1 view (bot_visible_history_messages) with PK/UK/FK/index and per-table classification. - set_null_fks.json: 47 canonical ON DELETE SET NULL FKs (== pinned oracle, including the two deferred ALTER-based FKs bot_channel_routes.active_session_id and bot_history_messages.compact_id) + 7 historical- distinct; every entry migrated:false as a checklist for later tasks. - classification.md: every table bucketed per schema contract §14; no business global-allowlist tables; 待确认 items called out. - canonical_vs_incremental_diff.md: documents canonical<->incremental DRIFT — fresh full-apply yields 49 tables (canonical 48 - channel_identity_bind_codes dropped by 0080, + media_assets 0007, + tasks 0063 never carried into canonical). Canonical is NOT a faithful projection of the chain at this base; must be reconciled before tenantizing. - TestInventoryCoversPostgresAndSetNull loads both artifacts, asserts >=48 tables, exactly 47 canonical SET NULL matching the oracle, non-empty fields, and no mis-parsed placeholder keywords.
Record the project decision that overrides the upstream AGENTS canonical-authority rule: - Existing migrations (0001_init..0105) are FROZEN and never edited. All tenantization lands as new incrementals (0106+), paired up/down, idempotent. - The upstream rule "0001_init.up.sql is the canonical full schema and every change must back-fill it to the final state" is explicitly REJECTED: it is a mistaken invariant already violated at this base (canonical omits media_assets and tasks and still declares channel_identity_bind_codes which 0080 drops). Re-syncing canonical would rewrite applied history. - Source of truth for the final schema = the applied state of 0001->0105 (+0106+) on real PostgreSQL 18, not the text of 0001_init. Downstream tasks enumerate tenant tables from the applied schema (information_schema), not from canonical. - Resolve drift 待确认 items by applied-state fact: media_assets + tasks EXIST (tenantize); channel_identity_bind_codes DROPPED by 0080 (out of scope). Net target = 49-table applied final state.
New incremental 0106 (existing migrations untouched): introduce the tenants root table and seed the default singleton tenant. - tenants(id PK, slug, created_at, updated_at, metadata); NO redundant tenant_id column (root special case: its id IS the tenant id). - Unique slug where not null; slug is a directory field, not an identity boundary. - Idempotent seed of DefaultTenantID (00000000-0000-0000-0000-000000000001), a fixed published constant in internal/tenant/id.go — never per-install random. - Down safety gate: refuses to drop the root when any non-default tenant exists (fail-closed for multi-tenant DBs; never wipe as rollback). - Integration tests (build tag `integration`, real PG18) assert the singleton is seeded, tenants has no tenant_id column, and 0106 is reversible up/down/up on a clean singleton while the down gate blocks a multi-tenant database.
New incremental 0107 (existing migrations untouched): the security core of the tenant-isolation contract (security design §4, §6). - DB roles: memoh_owner (NOLOGIN), memoh_migrator (BYPASSRLS, NOINHERIT), memoh_runtime (NOSUPERUSER NOBYPASSRLS serving role), memoh_break_glass (NOLOGIN) — created idempotently; object DDL runs as memoh_owner. - app schema (owner memoh_owner; runtime USAGE, no CREATE). - app.tenant_write_fences: global security meta-table, RLS DISABLED; zero runtime/PUBLIC table privileges; migrator full DML. BEFORE trigger enforces token>0, immutable tenant_id, non-decreasing token, advance-lands-disabled. - Fail-closed context helpers app.current_tenant_id() (missing->42501, bad UUID->22P02) and app.current_fencing_token() (missing/blank/malformed/ nonpositive->42501), SECURITY INVOKER, fixed search_path. - app.assert_tenant_write_fence() RETURNS void, SECURITY DEFINER, FOR SHARE lock; propagates context errors; missing/disabled->TENANT_WRITE_DISABLED, stale->FENCE_TOKEN_STALE (both 42501). app.tenant_write_fence_matches() RETURNS boolean, lock-free, no side effects. - app.cas_tenant_write_fence(): controlled CAS — advance only old->old+1 and land disabled; exact enable/disable keeps token; no skip/lower/advance+enable; migrator EXECUTE only. - Runtime ACL frozen: EXECUTE on assert/matches + current_tenant_id only; no table access, no CAS, no current_fencing_token. - Seed singleton fence (token 1, enabled). Down has a multi-tenant safety gate. - Integration matrix (real PG18) covers helper SQLSTATEs, CAS/trigger rules, runtime ACL denial, and up/down/up reversibility of the 0106+0107 chain.
Verified against a live PostgreSQL 18 full apply: tts_providers/tts_models are NOT fully-historical. 0061_unify_providers drops them, but its DO block early-returns when llm_providers is absent — which is always true on a fresh DB (0001_init already ships unified providers). So the tts pair SURVIVES a fresh install and must be tenantized; it is only dropped on a legacy upgrade path. - Applied fresh-install tenant set = 51 tables (53 base - schema_migrations - tenants root), enumerated from information_schema, not canonical text. - Static drop-scans are insufficient: they miss guarded/early-return blocks. - classification.md + canonical_vs_incremental_diff.md corrected accordingly.
New incremental 0108 (existing migrations untouched): schema contract phases A/B. Add a NULLABLE tenant_id column to every applied tenant business table and backfill it to the default singleton tenant. - Tables enumerated DYNAMICALLY from pg_class/public rather than hard-coded, because the applied set is path-dependent (tts_providers/tts_models survive a fresh install but not a legacy upgrade). Excludes schema_migrations tooling and the tenants root; app.* is outside public and untouched. - ADD COLUMN IF NOT EXISTS tenant_id uuid; UPDATE ... WHERE tenant_id IS NULL to DEFAULT_TENANT_ID. Nullable now; tightened to NOT NULL in a later migration after composite keys/FKs land. Existing installs upgrade in place (no wipe). - Down has a multi-tenant fail-closed gate: refuses to drop tenant_id if any table holds a non-default tenant_id. - Integration test enumerates the same applied set and asserts every tenant table has a fully backfilled (zero-NULL) tenant_id, and fence count == tenants count. Reversibility (down/up) verified on real PG18.
New incremental 0109 (existing migrations untouched): atomically re-key every tenant business table on tenant_id. Consolidates the schema contract's UNIQUE, SET NULL->RESTRICT, and composite PK/FK phases into ONE transaction, because in PostgreSQL an FK binds to a specific unique/PK index — a referenced key cannot be rebuilt without dropping/recreating its dependent FKs in the same operation. - Composite PKs (tenant_id, <old pk cols>) — also makes tenant_id NOT NULL. - UNIQUE constraints + the 8 partial/expression unique indexes rebuilt with tenant_id as the leftmost prefix (two tenants may reuse a natural key). - All 104 business FKs recreated as composite (tenant_id, col) -> parent(tenant_id, pcol); the 47 ON DELETE SET NULL become RESTRICT. - Root FK (tenant_id) -> tenants(id) ON DELETE RESTRICT added to every table. - Tables/keys enumerated dynamically from the applied schema (adapts to the fresh/legacy path divergence); single-column business FK shape asserted. - The up records original FK delete actions into app.tenant_fk_original so the down restores them EXACTLY (SET NULL->RESTRICT is otherwise lossy); down has a multi-tenant fail-closed gate and restores single-column keys + original delete semantics. - Integration tests (real PG18) assert PKs lead with tenant_id, zero SET NULL remain, every table has a root FK, business FKs carry tenant_id, cross-tenant references are rejected while same-tenant works, and true single-step reversibility via the golang-migrate Steps API (RunMigrate down is full-down).
New incremental 0110 (existing migrations untouched): the database-level
isolation backstop (security design §4.3, §5, §6). RLS is defense-in-depth;
application queries must still carry explicit tenant scope.
- ENABLE + FORCE ROW LEVEL SECURITY on every tenant table and the tenants root.
- Per-command policies TO memoh_runtime (idempotent DROP+CREATE):
_tenant_select USING (tenant_id = app.current_tenant_id())
_tenant_insert WITH CHECK (tenant_id = current AND tenant_write_fence_matches())
_tenant_update USING+WITH CHECK (tenant_id = current AND fence_matches())
_tenant_delete USING (tenant_id = current AND fence_matches())
No FOR ALL shorthand (would omit the write fence).
- tenants root: ENABLE+FORCE RLS, tenants_self_select (id = current); runtime is
read-only on the root.
- Grant the memoh_runtime serving role minimal per-table DML + sequence usage.
- app.tenant_write_fences keeps RLS OFF (boundary is ACL + helpers, not RLS).
- Tables enumerated dynamically; policies created idempotently.
- Integration tests (real PG18) prove FORCE RLS everywhere, fence table RLS off,
per-tenant SELECT isolation, cross-tenant INSERT rejected (42501), and
missing-GUC fail-closed. Reversibility test now computes the tenant step count
from the embedded migrations so it survives new migrations.
Groundwork for tenant-scoped queries (schema contract §11, security §7): - 0108: rewrite the nullable tenant_id ADD COLUMN as STATIC per-table `ALTER TABLE IF EXISTS ... ADD COLUMN IF NOT EXISTS tenant_id uuid` so sqlc's declarative parser sees the column (it cannot see columns added inside a DO/EXECUTE block). IF EXISTS/IF NOT EXISTS keeps it a safe no-op on the legacy upgrade path. Backfill stays dynamic (sqlc ignores UPDATEs). - Append tenant_id to the explicit column lists of the 22 single-table queries whose RETURNING/SELECT enumerated all pre-tenant columns, so their generated return type reuses the table model again (adding tenant_id had shifted them to distinct *Row types and broke the hand-written store.Queries interface). - Regenerate sqlc: every tenant-table model now carries a TenantID field. - Add internal/tenant/context.go: request-scoped tenant.Context with a default-singleton fallback (FromContextOrDefault) for the tenant id that query call sites will pass; upstream stays single-tenant by default and never trusts a client-supplied tenant id. Not yet: WHERE tenant_id scoping o 8000 n every query + threading tenant_id through call sites (large follow-up). RLS (0110) already enforces isolation at the DB level; this makes the generated layer tenant-aware. Build + vet + tenant integration tests green on PostgreSQL 18.
Establish the query tenant-scoping approach (security design §7: tenant comes from server-side context, never client args) with ZERO call-site changes: - 0108: give tenant_id a DEFAULT app.current_tenant_id() so every INSERT (sqlc or raw) auto-fills the current tenant; fail-closed if the GUC is unset. - internal/db: OpenPostgres installs an AfterConnect hook (SetDefaultTenantOnConnect) that binds the singleton tenant GUC at the session level. Upstream is single-tenant, so the default tenant is bound here; Cloud overrides per transaction with SET LOCAL from trusted context. Add OpenPostgresDSN helper. - Reference scoping: bots.sql SELECT/UPDATE/DELETE now carry WHERE tenant_id = app.current_tenant_id(); INSERTs are untouched (DEFAULT handles them). channel_identities ON CONFLICT target gains tenant_id (the UK is now composite). This uses no new sqlc parameter, so no generated signatures or call sites change. - Wire the 5 real-DB integration test harnesses to OpenPostgresDSN so their pools bind the tenant GUC like production; fix a pre-existing schedule test that omitted the required bot Name (empty string violates bots_name_format_check independently of tenant work). All 4 real-DB integration packages (schedule/session/channel/message) and the tenant migration integration tests pass on PostgreSQL 18; build + vet clean; sqlc regen deterministic. Deviation note: the plan literally wrote WHERE tenant_id = $1 threaded through call sites; using app.current_tenant_id() from the transaction GUC is spec-compliant in spirit (§7) and avoids touching ~622 call sites. RLS (0110) already enforces isolation; this is the explicit defense-in-depth scope.
Complete the query tenant-scoping across all 41 remaining query files (bots.sql + channel_identities were done earlier). Every SELECT/UPDATE/DELETE that reads or writes a tenant table now carries `tenant_id = app.current_tenant_id()` (scoping each tenant-table reference in JOINs/CTEs/subqueries by alias), and every ON CONFLICT target on a tenant table now leads with tenant_id (the unique/PK is composite). INSERTs are unchanged — the tenant_id DEFAULT app.current_tenant_id() auto-fills them. This is explicit application-layer scope (defense-in-depth) sourced from the same trusted transaction GUC as RLS, so it adds NO new sqlc parameter and changes NO call sites. Scoping care taken (verified in review): - LEFT JOINs to a tenant table scope the joined table in its ON clause (not WHERE) so outer-join semantics are preserved. - WHERE clauses with top-level OR are parenthesized before ANDing the tenant predicate, preventing AND/OR precedence cross-tenant leaks. - CTE tenant-table writes/reads are scoped inside the CTE. Also relax three SQL-string-matching test fakes (bots, handlers) whose matchers hard-coded `WHERE id = $1`; the scoped SQL is now `... AND id = $1`, so the matchers are loosened to `id = $1`. sqlc regen is deterministic; full build + unit suite green; each real-DB integration package (schedule/session/channel/message + tenant migrations) passes on a fresh PostgreSQL 18 database.
Prove the upstream G-MIG subset — existing installs upgrade in place with no
wipe — with real-PG18 integration tests:
- TestMigrateLegacyInstallPreservesRows: apply the chain up to the last
pre-tenant migration, seed a user + bot + session (legacy install), then apply
the tenant migrations. Asserts row counts preserved, every row backfilled to
the default tenant, the singleton fence present (enabled, positive token), and
the final schema tenant-scoped (bots PK leads with tenant_id).
- TestMigrateDownFailClosedForMultiTenant: with a non-default tenant + fence
present, stepping the tenant migrations down must fail closed (no wipe).
- TestMigrateDownSingletonSafe: a clean singleton can step the tenant migrations
down and back up.
Adds step helpers to the harness (tryStepDown, resetToEmpty, stepUpToPreTenant,
countAllMigrations) using the golang-migrate Steps API, since the repo's
RunMigrate("down") is a full-chain rollback.
…TATIC) Real-PG18 guard tests that assert the tenant schema contract holds structurally, so future changes cannot silently regress isolation: - TestTenantSchemaGuard (G-SCHEMA): every public tenant table (excluding schema_migrations tooling + tenants root) has a NOT NULL tenant_id, a PK leading with tenant_id, a root FK (tenant_id) -> tenants(id), and RLS enabled+forced; no composite FK carrying tenant_id uses ON DELETE SET NULL; the tenants root has no redundant tenant_id and is FORCE RLS. - TestFenceSecurityGuard (G-RLS-STATIC/G-ROLE fence portion): app.tenant_write_fences has NO RLS and zero memoh_runtime table ACL; the four app.* helpers have the exact frozen return types (assert->void SECDEF, matches->boolean SECDEF, current_tenant_id->uuid, current_fencing_token->bigint); memoh_runtime is neither BYPASSRLS nor superuser. - TestRLSPolicyGuard (G-RLS-STATIC policy portion): every tenant table has >= 4 per-command policies; write policies call the lock-free tenant_write_fence_matches() and never the lock-holding assert_tenant_write_fence(). Detection logic verified against injected violations (e.g. NO FORCE RLS is caught). No Cloud leakage: the full upstream diff contains no cloud_/cells/ tenant_placement/routing_mode paths.
SECV1 cross-tenant read: the view ran as its (superuser) owner and bypassed FORCE RLS, so a memoh_runtime connection bound to tenant 2 read tenant 1's messages through the view (reproduced). Also, the view was never GRANTed to runtime, so it was non-functional for the serving role. - 0111: recreate the view WITH (security_invoker = true) so it runs under the caller's RLS; project tenant_id; reassign owner to memoh_owner; GRANT SELECT to memoh_runtime. - Scope the ~20 queries reading the view with explicit tenant_id = app.current_tenant_id() (defense-in-depth per security §7). - Integration test seeds a tenant-1 message and asserts a tenant-2 runtime connection sees 0 rows via the view (re-verified on the reviewer's DB).
…e owner grant Address cross-review findings by amending the tenant migrations in place: - 0110: reassign every public tenant table/sequence/view ownership to memoh_owner. They were created by the bootstrap superuser, which is NOT subject to RLS; FORCE ROW LEVEL SECURITY only binds the ordinary owner, so the owner must be the non-super/non-bypass memoh_owner. ALTER DEFAULT PRIVILEGES (0107) only covers future objects, not these pre-existing tables. - 0109: preserve UNIQUE NULLS NOT DISTINCT when rebuilding composite uniques (the bare-UNIQUE rebuild widened bot_acl_rules_unique_target, letting duplicate ACL rules with NULL key columns through). Applied to both up and down. The down now also restores the original ON UPDATE action (was dropping it). Lock down app.tenant_fk_original (owner memoh_owner, no runtime/PUBLIC access). - 0107: REVOKE memoh_owner FROM CURRENT_USER after the migration's SET ROLE — leaving a superuser login role in owner's membership would let it assume owner and bypass RLS.
…parents The SET NULL -> RESTRICT conversion (0109) left deletes broken: deleting a parent that used to have ON DELETE SET NULL children now RESTRICT-errors. The schema contract (§8.1-§8.2) requires the repository to NULL the child columns in the SAME TRANSACTION before deleting the parent. Verified empirically that a single-statement CTE clear-then-delete does NOT work (the DELETE's FK RESTRICT check does not observe a same-statement CTE UPDATE), so this is implemented as separate statements in a transaction: - 61 ClearRef* sqlc queries (per child column, and bot/session/id-scoped variants for set-based deletes). - postgres/store overrides for all 11 parent-delete methods, running the matching clears then the delete inside InTx (which degrades to inline when already in a transaction, preserving atomicity). Covers models (13 children), the providers, plugins, channel configs/routes, session events, compaction, messages (by bot/session/ids), and DeleteChat (bot-wide cascade). - Integration tests call the real generated DeleteModel/DeleteChat and assert the refs are NULLed (or cascade-removed) rather than RESTRICT-rejected.
New incremental 0112: rebuild the 82 non-unique btree secondary indexes on tenant tables that did not lead with tenant_id, prepending it. Tenant queries filter by tenant_id (RLS + explicit app.current_tenant_id()), so a tenant_id-leading index lets the planner scan only the current tenant's slice. Purely a performance change; preserves each index's column list, partial WHERE, and access method. Reversible.
… minors - Schema guard now covers views (security_invoker=true, owner not bypass/superuser, projects tenant_id), table ownership (= memoh_owner), bot_acl_rules_unique_target NULLS NOT DISTINCT, and memoh_owner membership (no stray superuser/bypass/login member). These catch every cross-review finding as a regression gate. - Mark all 47 canonical SET NULL entries migrated=true in the inventory. - BASE.md: record the canonical-vs-AGENTS PR decision (keep 0001 frozen, revise the upstream canonical-authority rule) and the empirical finding that single-statement CTE clear-then-delete does not satisfy FK RESTRICT. - tenant/context.go: clarify the DB-level tenant scope is the app.tenant_id GUC, not the Go context id.
Remove deployment-specific database roles, write fencing, Cloud context scaffolding, and one-off planning inventory from the upstream tenant schema change. Keep existing primary keys, preserve ON DELETE SET NULL with PostgreSQL column lists, limit dynamic DDL to Memoh-managed tenant tables, and add least-privilege and extension-table migration guards.
Collapse the seven tenant-schema increments into one reversible 0107_tenant_core migration, immediately after the current upstream 0106 head.
# Conflicts: # db/postgres/queries/messages.sql # internal/db/postgres/sqlc/messages.sql.go # internal/db/postgres/sqlc/provider_oauth.sql.go
chen-ran
marked this pull request as ready for review
July 16, 2026 01:30
HoneyBBQ
force-pushed
the
tenant-schema-phase1
branch
from
July 16, 2026 10:40
e9ea7d8 to
f8758cc
Compare
HoneyBBQ
force-pushed
the
tenant-schema-phase1
branch
2 times, most recently
from
July 16, 2026 11:56
93f6370 to
fc75efb
Compare
sheepbox8646
pushed a commit
that referenced
this pull request
Jul 17, 2026
## Summary - move the optional pgvector schema into its own embedded golang-migrate stream - generate typed embedding queries through a second sqlc target and execute them through a shared pgx store - migrate once before pgvector type registration instead of running DDL from provider construction or Health - preserve existing unversioned embedding tables and rows when adopting migration version 1 - keep pgvector optional: initialization failures disable the semantic index without preventing the main server from starting This supersedes the narrower approach in #813 by covering constructor lifecycle, concurrent startup, schema versioning, shared connection ownership, and read-only health checks. ## Validation - `mise run lint` - `go test ./...` - PostgreSQL 18 + pgvector integration coverage for concurrent migration, legacy adoption, typed upsert/search, schema drift, and future-version rejection - `git diff --check`
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
ensureStoreRoot cause
Every memory status request called
ensureStore, causing the health-check path to execute schema DDL even though the store had already been initialized when the pgvector index was created.Impact
Repeated health checks are now read-only. If the embeddings table becomes unavailable after startup, the health check reports the failure instead of mutating the database to repair it implicitly.
Validation
go test ./internal/memory/adapters/builtingit diff --check