Skip to content

Commit 5aaa0a9

Browse files
committed
Fix re-critique: complete SQL bridge rename, runnable rebuild recipe, readiness-probe doc
- Finish the createEmbedRegistrySqlClient -> createRawSqlClient rename at the four remaining internal call sites (search.ts:11,521,639, capture.ts:29,503) and delete the now-unused deprecated alias — the symbol was never exported, so it had no external consumer to protect. - The rebuild recipe (fts-language.ts error text + IMPLEMENTATION.md) wrapped CREATE INDEX CONCURRENTLY inside BEGIN/COMMIT, which Postgres has rejected unconditionally since 8.2 (not version-dependent, as previously claimed). Verified against a live postgres:16 container: the wrapped form fails with "CREATE INDEX CONCURRENTLY cannot run inside a transaction block"; the split form (BEGIN/ALTER/ALTER/COMMIT, then CONCURRENTLY as its own statement) succeeds. Both error sites now share one verified recipe via rebuildColumnRecipe(). - The schema-qualified-config error previously said "see the recipe below" with nothing below it in the thrown string; it now includes the same recipe inline. - Documented the FTS verification gap explicitly (mount-time comment in knowledge.ts, IMPLEMENTATION.md x2): the serving-path check is lazy and memoized, not boot-time — a host that neither runs runKnowledgeMigrations nor wires its own readiness probe learns about a language mismatch on the first real query, not at mount. Not re-architected, per instructions — made explicit instead. Claude-Session: https://claude.ai/code/session_017GTgGzn5xAwvkU2GAPAHpF
1 parent d45720f commit 5aaa0a9

6 files changed

Lines changed: 71 additions & 35 deletions

File tree

‎IMPLEMENTATION.md‎

Lines changed: 29 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -41,6 +41,12 @@ routes; each reads identity from the context (`caller(c)`) and guards via
4141
singletons except the logger). Nothing has import-time side effects, so unit
4242
tests exercise the routes and services directly without a listening server.
4343

44+
Mounting does **not** verify the FTS language against the database at boot —
45+
see the `knowledge_chunk` section below. A host is expected to either run
46+
`runKnowledgeMigrations` itself (which verifies) or wire its own readiness
47+
probe to call `verifyFtsLanguage`; without one of those, a language mismatch
48+
surfaces as a runtime failure on the plane's first query, not at mount time.
49+
4450
## Config
4551

4652
There are two config types, both in the SDK:
@@ -136,33 +142,45 @@ read-only against the catalog: `runKnowledgeMigrations` checks after
136142
applying (the deploy step), and the knowledge plane runs the same check
137143
once, memoized, before its first query (the serving path) — so a mismatch
138144
or unmigrated schema fails loudly on first use regardless of who ran the
139-
migrations. Hosts with a readiness probe can call the exported
140-
`verifyFtsLanguage` there instead. Chunks are **never** reused across
141-
versions — every new version gets a fresh full insert of its own chunks.
145+
migrations. **The serving-path check only runs when something actually
146+
calls it** — `search()`/`capture()` invoke it lazily and memoize the result,
147+
but nothing forces that first call to happen at boot. A host that mounts the
148+
engine without running `runKnowledgeMigrations` itself and without wiring a
149+
readiness probe will not learn about a language mismatch until the first
150+
real query or capture fails — not at startup. Hosts that want a boot-time
151+
guarantee **must** call the exported `verifyFtsLanguage` from their own
152+
readiness probe; it is not optional belt-and-suspenders, it is the only way
153+
to get a boot-time check if this SDK instance isn't the one that migrated.
154+
Chunks are **never** reused across versions — every new version gets a
155+
fresh full insert of its own chunks.
142156

143157
**Changing `FTS_LANGUAGE` on an already-migrated database** (the mismatch
144158
`verifyFtsLanguage` throws on) requires rebuilding the generated column —
145159
`runKnowledgeMigrations` only applies new files and will not retroactively
146-
alter an existing one. One-time recipe:
160+
alter an existing one. One-time recipe (verified against a live
161+
`postgres:16` instance):
147162

148163
```sql
149164
BEGIN;
165+
DROP INDEX IF EXISTS knowledge_chunk_text_fts_idx;
150166
ALTER TABLE knowledge_chunk DROP COLUMN text_fts;
151167
ALTER TABLE knowledge_chunk ADD COLUMN text_fts tsvector
152168
GENERATED ALWAYS AS (to_tsvector('<new_language>', "text")) STORED;
153-
CREATE INDEX CONCURRENTLY knowledge_chunk_text_fts_idx ON knowledge_chunk USING gin (text_fts);
154169
COMMIT;
170+
171+
-- Separate statement/connection — CREATE INDEX CONCURRENTLY is rejected
172+
-- inside any transaction block, unconditionally, since Postgres 8.2. It
173+
-- cannot be combined with the BEGIN/COMMIT block above.
174+
CREATE INDEX CONCURRENTLY knowledge_chunk_text_fts_idx ON knowledge_chunk USING gin (text_fts);
155175
```
156176

157177
Both `ALTER TABLE` statements take an `ACCESS EXCLUSIVE` lock and force a
158178
full table rewrite (dropping then re-adding a `STORED` generated column
159179
always rewrites) — plan for a stall on `knowledge_chunk` for the duration on
160-
a populated database; run in a maintenance window. `CREATE INDEX
161-
CONCURRENTLY` cannot run inside the same transaction as the `ALTER`s on some
162-
Postgres versions — if it errors there, `COMMIT` the `ALTER`s first and run
163-
the index creation as a separate statement. Only unqualified `pg_catalog`
164-
config names are supported; a schema-qualified config on this column is
165-
rejected explicitly by `verifyFtsLanguage` rather than silently mis-parsed.
180+
a populated database; run in a maintenance window. Only unqualified
181+
`pg_catalog` config names are supported; a schema-qualified config on this
182+
column is rejected explicitly by `verifyFtsLanguage` (with this same recipe
183+
in the error) rather than silently mis-parsed.
166184

167185
### `knowledge_entity` / `knowledge_edge`
168186
Lightweight graph rows. `knowledge_entity` has no unique constraint; dedupe on

‎src/core/embed-sql.ts‎

Lines changed: 0 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -16,6 +16,3 @@ export function createRawSqlClient(sql: RawSql): EmbedRegistrySqlClient {
1616
},
1717
};
1818
}
19-
20-
/** @deprecated Use `createRawSqlClient` — same bridge, name predates its FTS use. */
21-
export const createEmbedRegistrySqlClient = createRawSqlClient;

‎src/core/fts-language.ts‎

Lines changed: 30 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -51,6 +51,32 @@ export function createFtsVerification(
5151
}));
5252
}
5353

54+
/**
55+
* The one-time column-rebuild recipe, shared by every error that needs to
56+
* point an operator at it. `CREATE INDEX CONCURRENTLY` is deliberately its
57+
* own statement, outside the `BEGIN`/`COMMIT` block: Postgres has rejected
58+
* CONCURRENTLY inside a transaction block unconditionally since 8.2 (not a
59+
* version-dependent quirk) — wrapping it in the same transaction as the
60+
* ALTERs makes this recipe fail outright instead of fixing anything.
61+
* Verified against a real postgres:16 container before being written here.
62+
*/
63+
function rebuildColumnRecipe(language: string): string {
64+
return (
65+
` BEGIN;\n` +
66+
` DROP INDEX IF EXISTS knowledge_chunk_text_fts_idx;\n` +
67+
` ALTER TABLE knowledge_chunk DROP COLUMN text_fts;\n` +
68+
` ALTER TABLE knowledge_chunk ADD COLUMN text_fts tsvector\n` +
69+
` GENERATED ALWAYS AS (to_tsvector('${language}', "text")) STORED;\n` +
70+
` COMMIT;\n\n` +
71+
` -- Separate statement/connection — CANNOT run inside the transaction\n` +
72+
` -- above, or any transaction block, ever:\n` +
73+
` CREATE INDEX CONCURRENTLY knowledge_chunk_text_fts_idx ON knowledge_chunk USING gin (text_fts);\n\n` +
74+
`Both ALTER TABLE statements take an ACCESS EXCLUSIVE lock and rewrite the table ` +
75+
`(DROP COLUMN then re-adding a STORED generated column forces a full rewrite) — ` +
76+
`expect a stall on this table for the duration on a populated database; run during a maintenance window.`
77+
);
78+
}
79+
5480
export interface FtsVerifySqlClient {
5581
query: (sql: string, params: readonly unknown[]) => Promise<Array<Record<string, unknown>>>;
5682
}
@@ -106,26 +132,16 @@ export async function verifyFtsLanguage(
106132
throw new Error(
107133
`knowledge_chunk.text_fts was built with the schema-qualified text search config "${schema}.${applied}", ` +
108134
`but FTS_LANGUAGE only supports unqualified pg_catalog configs. ` +
109-
`Either drop the schema qualification (move/alias the config into pg_catalog) or rebuild the column — see the recipe below.`,
135+
`Either drop the schema qualification (move/alias the config into pg_catalog), or rebuild the column ` +
136+
`under an unqualified config name:\n\n${rebuildColumnRecipe(ftsLanguage)}`,
110137
);
111138
}
112139
if (applied !== ftsLanguage) {
113140
throw new Error(
114141
`FTS language mismatch: knowledge_chunk.text_fts was built with "${applied}" but the configuration says "${ftsLanguage}". ` +
115142
`Search would silently stem queries differently than the index.\n\n` +
116-
`To rebuild the column under the new language:\n` +
117-
` BEGIN;\n` +
118-
` ALTER TABLE knowledge_chunk DROP COLUMN text_fts;\n` +
119-
` ALTER TABLE knowledge_chunk ADD COLUMN text_fts tsvector\n` +
120-
` GENERATED ALWAYS AS (to_tsvector('${ftsLanguage}', "text")) STORED;\n` +
121-
` CREATE INDEX CONCURRENTLY knowledge_chunk_text_fts_idx ON knowledge_chunk USING gin (text_fts);\n` +
122-
` COMMIT;\n\n` +
123-
`Both ALTER TABLE statements take an ACCESS EXCLUSIVE lock and rewrite the table ` +
124-
`(DROP COLUMN then re-adding a STORED generated column forces a full rewrite) — ` +
125-
`expect a stall on this table for the duration on a populated database; run during a ` +
126-
`maintenance window. CREATE INDEX CONCURRENTLY cannot run inside the same transaction as ` +
127-
`the ALTERs on some Postgres versions — if it errors there, commit the ALTERs first, then ` +
128-
`run the index creation separately. Or, fix FTS_LANGUAGE back to "${applied}" instead.`,
143+
`To rebuild the column under the new language:\n\n${rebuildColumnRecipe(ftsLanguage)}\n\n` +
144+
`Or, fix FTS_LANGUAGE back to "${applied}" instead.`,
129145
);
130146
}
131147
}

‎src/knowledge.ts‎

Lines changed: 7 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -63,8 +63,13 @@ export function createKnowledgePlane(config: KnowledgeConfig): KnowledgePlane {
6363
// (Hibernate validate / Rails check_all_pending!): the mount is
6464
// synchronous, so "before accepting traffic" becomes a memoized check
6565
// awaited by the first query. Read-only; migration stays a deploy step.
66-
// Hosts with a real readiness probe can call verifyFtsLanguage there
67-
// instead — this memo then resolves against an already-verified schema.
66+
// NOTE this is a lazy check, not a boot-time one: nothing forces it to run
67+
// until the first real search()/capture() call, so a host that neither
68+
// runs runKnowledgeMigrations itself nor wires a readiness probe will not
69+
// learn about a language mismatch until that first call fails. A host
70+
// that wants a real boot-time guarantee MUST call the exported
71+
// verifyFtsLanguage from its own readiness probe — this memo then
72+
// resolves instantly against the already-verified schema.
6873
const ensureVerified = createFtsVerification(
6974
createRawSqlClient(sql),
7075
engineConfig.ftsLanguage ?? DEFAULT_FTS_LANGUAGE,

‎src/services/capture.ts‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -26,7 +26,7 @@ import type {
2626
EntityHint,
2727
} from "../core/schemas/adapted-document.ts";
2828
import type { KnowledgeEdgeHint } from "../core/schemas/entity-edge.ts";
29-
import { createEmbedRegistrySqlClient } from "../core/embed-sql.ts";
29+
import { createRawSqlClient } from "../core/embed-sql.ts";
3030
import { activateEmbedModel } from "../core/embed-model-registry.ts";
3131
import { EmbedClientConfigSchema, type EmbedClientConfig } from "../core/embed-client.ts";
3232
import { embedChunks, type EmbeddableChunk } from "../core/embed-worker.ts";
@@ -500,7 +500,7 @@ async function embedInsertedChunksWithConfig(
500500
if (chunks.length === 0) return { degraded: false };
501501

502502
try {
503-
const client = createEmbedRegistrySqlClient(sql);
503+
const client = createRawSqlClient(sql);
504504

505505
const activeTable = await activateEmbedModel(
506506
client,

‎src/services/search.ts‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -8,7 +8,7 @@ import {
88
knowledgeEdge,
99
knowledgeVersion,
1010
} from "../db/schema.ts";
11-
import { createEmbedRegistrySqlClient } from "../core/embed-sql.ts";
11+
import { createRawSqlClient } from "../core/embed-sql.ts";
1212
import {
1313
cosineDistanceExpr,
1414
EMBED_TABLE_NAME_PATTERN,
@@ -518,7 +518,7 @@ export async function fetchDenseCandidates(
518518

519519
if (query === "") return null;
520520

521-
const embedSqlClient = createEmbedRegistrySqlClient(rawSql);
521+
const embedSqlClient = createRawSqlClient(rawSql);
522522
const activeTable = await resolveActiveEmbedTable(embedSqlClient, tenantId);
523523
if (!activeTable) return null;
524524

@@ -636,7 +636,7 @@ async function fetchChunkVectors(
636636
): Promise<Map<string, number[]>> {
637637
if (chunkIds.length === 0) return new Map();
638638

639-
const embedSqlClient = createEmbedRegistrySqlClient(rawSql);
639+
const embedSqlClient = createRawSqlClient(rawSql);
640640
const activeTable = await resolveActiveEmbedTable(embedSqlClient, tenantId);
641641
if (!activeTable) return new Map();
642642

0 commit comments

Comments
 (0)