Skip to content

Commit dde905c

Browse files
abnegateclaude
andcommitted
fix(sqlite): address remaining FTS5 review findings
- Use double-quoted identifiers in `content=` and `content_rowid=` inside `USING fts5(...)`. FTS5's config grammar parses backticks as part of the literal value rather than as an identifier delimiter on some SQLite builds, leaving the engine unable to locate the content table. - Restrict the FTS5 update trigger to `AFTER UPDATE OF <indexed cols>` so the trigger no longer fires (and re-tokenises) for parent UPDATEs that touch only `_updatedAt` / `_permissions`. - Add `getTenantSegment()` and route every `$this->tenant` interpolation into a SQL identifier through it. The base property is typed `int|string|null`, so a string-typed tenant would otherwise reach DDL identifier names (regular index lookup, DROP INDEX, getSQLIndex) without going through `filter()`. - Tenant-scope FTS5 table names under sharedTables to mirror the regular- index naming pattern. Without this, tenant A's `deleteIndex(<fulltext>)` drops the FTS5 vtable that tenant B is also using; tenant-scoped names give each tenant their own vtable, so each drop is isolated. - Throw a `DatabaseException` from `dropFulltextIndexById` when multiple FTS5 tables exist on a collection and the index id can't be resolved via metadata. Returning `false` previously fell through to `DROP INDEX`, whose "no such index" error was swallowed by `deleteIndex` as a benign already-gone case — silent success while the FTS5 tables survived. - Restructure `ftsTableCache` from `<collection>::<attribute>` flat keys to `<collection> => <attribute, table>` populated in one pass per collection. Eliminates the N+1 `PRAGMA table_info` storm on multi- attribute search batches and lets DDL invalidate just the affected collection's slice instead of wiping the entire cache. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
1 parent b97e5c4 commit dde905c

1 file changed

Lines changed: 87 additions & 26 deletions

File tree

src/Database/Adapter/SQLite.php

Lines changed: 87 additions & 26 deletions
Original file line numberDiff line numberDiff line change
@@ -58,12 +58,18 @@ class SQLite extends MariaDB
5858
private const FTS_TRIGGER_UPDATE = 'au';
5959

6060
/**
61-
* Memo of FTS5 table resolution keyed by `<collection>::<attribute>`,
62-
* value is the matching FTS5 table name or null when no match exists.
63-
* Avoids re-scanning sqlite_master + PRAGMA table_info on every search
64-
* condition. Cleared whenever an FTS5 table is created or dropped.
61+
* Memo of FTS5 table resolution per collection. Outer key is the
62+
* collection name; inner map is attribute → matching FTS5 table
63+
* (or null when no FTS5 table covers that attribute). Populated in
64+
* one sqlite_master + PRAGMA pass per collection so subsequent
65+
* lookups for any attribute are O(1) — the previous flat
66+
* `<collection>::<attribute>` scheme re-issued PRAGMA per attribute
67+
* miss, building an N+1 storm on multi-attribute search batches.
68+
* Invalidated per-collection on FTS create/drop and on
69+
* deleteCollection — only the affected collection's slice is
70+
* cleared, not the whole map.
6571
*
66-
* @var array<string, ?string>
72+
* @var array<string, array<string, ?string>>
6773
*/
6874
private array $ftsTableCache = [];
6975

@@ -532,7 +538,7 @@ public function deleteCollection(string $id): bool
532538
// Drop any cached FTS table lookups for this collection — a
533539
// subsequent createCollection of the same name with a different
534540
// attribute set must not resolve to the just-dropped tables.
535-
$this->ftsTableCache = [];
541+
unset($this->ftsTableCache[$id]);
536542

537543
return true;
538544
}
@@ -730,7 +736,7 @@ public function createIndex(string $collection, string $id, string $type, array
730736
FROM sqlite_master
731737
WHERE type='index' AND name=:_index;
732738
");
733-
$stmt->bindValue(':_index', "{$this->getNamespace()}_{$this->tenant}_{$name}_{$id}");
739+
$stmt->bindValue(':_index', "{$this->getNamespace()}_{$this->getTenantSegment()}_{$name}_{$id}");
734740
$stmt->execute();
735741
$index = $stmt->fetch();
736742
if (!empty($index)) {
@@ -792,7 +798,14 @@ protected function createFulltextIndex(string $collection, string $id, array $at
792798
// index — the next document write would silently desync.
793799
$this->startTransaction();
794800
try {
795-
$createSql = "CREATE VIRTUAL TABLE `{$ftsTable}` USING fts5({$ftsColumnList}, content=`{$parentTable}`, content_rowid=`_id`)";
801+
// FTS5's `fts5(...)` config grammar parses `content=` and
802+
// `content_rowid=` values as either bare identifiers, double-
803+
// quoted identifiers, or single-quoted strings. Backticks are
804+
// not part of the grammar — SQLite's lenient parser accepts
805+
// them in some builds but treats them as part of the literal
806+
// value in others, where the engine then can't locate the
807+
// content table. Use double quotes per the documented form.
808+
$createSql = "CREATE VIRTUAL TABLE `{$ftsTable}` USING fts5({$ftsColumnList}, content=\"{$parentTable}\", content_rowid=\"_id\")";
796809
$createSql = $this->trigger(Database::EVENT_INDEX_CREATE, $createSql);
797810
$this->getPDO()->prepare($createSql)->execute();
798811

@@ -813,8 +826,13 @@ protected function createFulltextIndex(string $collection, string $id, array $at
813826
$this->getPDO()->prepare($deleteTrigger)->execute();
814827

815828
$updateSuffix = self::FTS_TRIGGER_UPDATE;
829+
// Restrict the update trigger to changes that touch indexed
830+
// columns. Without `OF <cols>` the trigger fires on every
831+
// UPDATE — including timestamp- or permission-only writes —
832+
// forcing FTS5 to delete-marker + re-tokenise the row even
833+
// when the indexed text is unchanged.
816834
$updateTrigger = "
817-
CREATE TRIGGER `{$ftsTable}_{$updateSuffix}` AFTER UPDATE ON `{$parentTable}` BEGIN
835+
CREATE TRIGGER `{$ftsTable}_{$updateSuffix}` AFTER UPDATE OF {$columnList} ON `{$parentTable}` BEGIN
818836
INSERT INTO `{$ftsTable}` (`{$ftsTable}`, rowid, {$columnList}) VALUES ('delete', OLD.`_id`, {$oldColumnList});
819837
INSERT INTO `{$ftsTable}` (rowid, {$columnList}) VALUES (NEW.`_id`, {$newColumnList});
820838
END
@@ -834,7 +852,7 @@ protected function createFulltextIndex(string $collection, string $id, array $at
834852
throw $e;
835853
}
836854

837-
$this->ftsTableCache = [];
855+
unset($this->ftsTableCache[$collection]);
838856

839857
return true;
840858
}
@@ -862,17 +880,33 @@ protected function getFulltextTableName(string $collection, array|string $attrib
862880
}
863881

864882
/**
865-
* Common prefix for every FTS5 table belonging to `$collection`. Used by
866-
* lookup helpers that scan sqlite_master with a LIKE pattern. Mirrors
867-
* getSQLTable's naming — namespace + collection only, no tenant. Under
868-
* shared tables the tenant is a column, not a name suffix, and the FTS5
869-
* table tracks the underlying collection 1:1.
883+
* Common prefix for every FTS5 table belonging to `$collection`. Used
884+
* by lookup helpers that scan sqlite_master with a LIKE pattern. Under
885+
* shared tables, mirror the regular-index naming and include the
886+
* tenant segment so each tenant gets their own FTS5 vtable — without
887+
* that, tenant A's deleteIndex(<fulltext>) would drop the FTS5 table
888+
* tenant B is also using.
870889
*/
871890
protected function getFulltextTablePrefix(string $collection): string
872891
{
892+
if ($this->sharedTables) {
893+
return "{$this->getNamespace()}_{$this->getTenantSegment()}_{$this->filter($collection)}_";
894+
}
895+
873896
return "{$this->getNamespace()}_{$this->filter($collection)}_";
874897
}
875898

899+
/**
900+
* Filtered string form of the active tenant for safe interpolation
901+
* into SQL identifiers. The base property is typed `int|string|null`,
902+
* so a string-typed tenant (rare but allowed by the contract) would
903+
* otherwise reach DDL identifiers without going through filter().
904+
*/
905+
private function getTenantSegment(): string
906+
{
907+
return $this->filter((string) ($this->tenant ?? ''));
908+
}
909+
876910
/**
877911
* Delete Index
878912
*
@@ -891,7 +925,7 @@ public function deleteIndex(string $collection, string $id): bool
891925
// DROP INDEX path. Otherwise the index is either an FTS5 virtual
892926
// table (whose name is keyed off attributes, not the id) or
893927
// already absent — try the FTS5 path before erroring.
894-
$regularIndex = "{$this->getNamespace()}_{$this->tenant}_{$name}_{$id}";
928+
$regularIndex = "{$this->getNamespace()}_{$this->getTenantSegment()}_{$name}_{$id}";
895929
$stmt = $this->getPDO()->prepare("
896930
SELECT name FROM sqlite_master WHERE type='index' AND name=:_index
897931
");
@@ -952,7 +986,17 @@ protected function dropFulltextIndexById(string $collection, string $id): bool
952986
if (\count($tables) === 1) {
953987
$ftsTable = $tables[0];
954988
} else {
955-
return false;
989+
// Returning false would let deleteIndex fall through to
990+
// `DROP INDEX <regular>` which then errors with "no such
991+
// index" — the catch in deleteIndex swallows that as a
992+
// benign already-gone case, so the FTS5 tables would
993+
// silently survive a delete that reported success. Surface
994+
// the ambiguity instead.
995+
throw new DatabaseException(
996+
"Cannot resolve fulltext index '{$id}' on '{$collection}': "
997+
. \count($tables) . ' FTS5 tables exist and metadata does not'
998+
. ' match any of them.'
999+
);
9561000
}
9571001
}
9581002
$triggerSuffixes = [
@@ -978,7 +1022,7 @@ protected function dropFulltextIndexById(string $collection, string $id): bool
9781022
throw $e;
9791023
}
9801024

981-
$this->ftsTableCache = [];
1025+
unset($this->ftsTableCache[$collection]);
9821026

9831027
return true;
9841028
}
@@ -1720,7 +1764,7 @@ protected function getSQLIndex(string $collection, string $id, string $type, arr
17201764
$attributes[$key] = "`{$attribute}` {$postfix}";
17211765
}
17221766

1723-
$key = "`{$this->getNamespace()}_{$this->tenant}_{$collection}_{$id}`";
1767+
$key = "`{$this->getNamespace()}_{$this->getTenantSegment()}_{$collection}_{$id}`";
17241768
$attributes = implode(', ', $attributes);
17251769

17261770
if ($this->sharedTables) {
@@ -3219,28 +3263,45 @@ private function buildSearchLikeFallback(
32193263
* Find the FTS5 virtual table on `$collection` that covers `$attribute`,
32203264
* or null if none exists. Resolves the multi-column case where the
32213265
* stored table name is keyed off the full sorted attribute set and
3222-
* can't be reconstructed from a single attribute.
3266+
* can't be reconstructed from a single attribute. Populates the per-
3267+
* collection attribute → table map on first call so subsequent lookups
3268+
* hit memory.
32233269
*/
32243270
protected function findFulltextTableForAttribute(string $collection, string $attribute): ?string
32253271
{
3226-
$cacheKey = "{$collection}::{$attribute}";
3227-
if (\array_key_exists($cacheKey, $this->ftsTableCache)) {
3228-
return $this->ftsTableCache[$cacheKey];
3272+
if (!\array_key_exists($collection, $this->ftsTableCache)) {
3273+
$this->ftsTableCache[$collection] = $this->buildFulltextAttributeMap($collection);
32293274
}
32303275

3276+
return $this->ftsTableCache[$collection][$attribute] ?? null;
3277+
}
3278+
3279+
/**
3280+
* Walk every FTS5 table on `$collection` once and return a flat
3281+
* attribute → table map. Each FTS5 table contributes its columns,
3282+
* with the last write winning if two tables happen to share a
3283+
* column (createFulltextIndex's `IF NOT EXISTS` guard plus the
3284+
* sha1-hashed naming make that practically unreachable).
3285+
*
3286+
* @return array<string, string>
3287+
*/
3288+
private function buildFulltextAttributeMap(string $collection): array
3289+
{
3290+
$map = [];
32313291
foreach ($this->findFulltextTables($collection) as $table) {
32323292
$info = $this->getPDO()->prepare("PRAGMA table_info(`{$table}`)");
32333293
$info->execute();
32343294
$cols = $info->fetchAll(PDO::FETCH_ASSOC);
32353295
$info->closeCursor();
32363296
foreach ($cols as $col) {
3237-
if (($col['name'] ?? null) === $attribute) {
3238-
return $this->ftsTableCache[$cacheKey] = $table;
3297+
$name = $col['name'] ?? null;
3298+
if (\is_string($name) && $name !== '') {
3299+
$map[$name] = $table;
32393300
}
32403301
}
32413302
}
32423303

3243-
return $this->ftsTableCache[$cacheKey] = null;
3304+
return $map;
32443305
}
32453306

32463307
/**

0 commit comments

Comments
 (0)