diff --git a/src/Database/Adapter/Postgres.php b/src/Database/Adapter/Postgres.php index 04863a5fa..de116edb7 100644 --- a/src/Database/Adapter/Postgres.php +++ b/src/Database/Adapter/Postgres.php @@ -233,7 +233,7 @@ public function createCollection(string $name, array $attributes = [], array $in CREATE INDEX \"{$createdIndex}\" ON {$this->getSQLTable($id)} (_tenant, \"_createdAt\"); CREATE INDEX \"{$updatedIndex}\" ON {$this->getSQLTable($id)} (_tenant, \"_updatedAt\"); CREATE INDEX \"{$tenantIdIndex}\" ON {$this->getSQLTable($id)} (_tenant, _id); - CREATE INDEX \"{$permissionsIndex}\" ON {$this->getSQLTable($id)} USING gin (_permissions); + CREATE INDEX \"{$permissionsIndex}\" ON {$this->getSQLTable($id)} USING gin (_permissions) WITH (fastupdate = off); "; } else { $uidIndex = $this->getShortKey("{$namespace}_{$id}_uid"); @@ -244,7 +244,7 @@ public function createCollection(string $name, array $attributes = [], array $in CREATE UNIQUE INDEX \"{$uidIndex}\" ON {$this->getSQLTable($id)} (\"_uid\" COLLATE utf8_ci_ai); CREATE INDEX \"{$createdIndex}\" ON {$this->getSQLTable($id)} (\"_createdAt\"); CREATE INDEX \"{$updatedIndex}\" ON {$this->getSQLTable($id)} (\"_updatedAt\"); - CREATE INDEX \"{$permissionsIndex}\" ON {$this->getSQLTable($id)} USING gin (_permissions); + CREATE INDEX \"{$permissionsIndex}\" ON {$this->getSQLTable($id)} USING gin (_permissions) WITH (fastupdate = off); "; } @@ -1510,8 +1510,10 @@ protected function handleDistanceSpatialQueries(Query $query, array &$binds, str $binds[":{$placeholder}_2"] = $degrees[0]; $binds[":{$placeholder}_3"] = $degrees[1]; + // Each use binds under its own name: the permissions condition's ?? fails once a placeholder repeats + $binds[":{$placeholder}_4"] = $binds[":{$placeholder}_0"]; - return "{$alias}.{$attribute} && ST_Expand(" . $this->getSpatialGeomFromText(":{$placeholder}_0") . ", :{$placeholder}_2, :{$placeholder}_3) AND {$distance}"; + return "{$alias}.{$attribute} && ST_Expand(" . $this->getSpatialGeomFromText(":{$placeholder}_4") . ", :{$placeholder}_2, :{$placeholder}_3) AND {$distance}"; } // Without meters, use the original SRID (e.g., 4326) @@ -1519,7 +1521,11 @@ protected function handleDistanceSpatialQueries(Query $query, array &$binds, str // ST_DWithin can use the GIST index; ST_Distance keeps the boundary exclusive if ($within) { - return "ST_DWithin({$alias}.{$attribute}, " . $this->getSpatialGeomFromText(":{$placeholder}_0") . ", :{$placeholder}_1) AND {$distance}"; + // Each use binds under its own name, as above + $binds[":{$placeholder}_2"] = $binds[":{$placeholder}_0"]; + $binds[":{$placeholder}_3"] = $binds[":{$placeholder}_1"]; + + return "ST_DWithin({$alias}.{$attribute}, " . $this->getSpatialGeomFromText(":{$placeholder}_2") . ", :{$placeholder}_3) AND {$distance}"; } return $distance; @@ -1892,20 +1898,35 @@ protected function getSQLPermissionsCondition( $column = "{$this->quote($alias)}.{$this->quote('_permissions')}"; - // Containment rather than jsonb's ?| key operator: PDO reads a lone ? as a positional - // placeholder, and doubling it to escape breaks once a named placeholder is repeated, - // which the cursor conditions do. Each role is its own @> so the index can answer them - // as a BitmapOr; jsonb_exists_any would express it in one call but is not indexable. - $permissions = \array_map( - fn ($role) => "{$column} @> {$this->getPDO()->quote(\json_encode(["{$type}(\"{$role}\")"]))}::jsonb", - $roles - ); + $conditions = []; + + // A lone ?| is priced like a single equality, so the planner checks it first on every + // row. Matching "any" on its own makes the check cost two conditions, and the reader's + // own filters run before it. + if (\in_array('any', $roles, true)) { + $conditions[] = "{$column} @> " . $this->getPDO()->quote(\json_encode(["{$type}(\"any\")"])) . '::jsonb'; + $roles = \array_values(\array_diff($roles, ['any'])); + } + + // One ?| for the other roles; a @> per role adds to the row estimate until the planner + // gives up on the GIN index. ?? is PDO's escape for a literal ?, which emulated prepares + // only accept while no named placeholder appears twice in the statement, so the queries + // this condition joins bind each value under its own name. jsonb_exists_any would avoid + // the ? but is not indexable. + if ($roles !== []) { + $permissions = \array_map( + fn ($role) => $this->getPDO()->quote("{$type}(\"{$role}\")"), + $roles + ); + + $conditions[] = "{$column} ??| ARRAY[" . \implode(', ', $permissions) . ']::text[]'; + } - if ($permissions === []) { + if ($conditions === []) { return 'FALSE'; } - return '(' . \implode(' OR ', $permissions) . ')'; + return '(' . \implode(' OR ', $conditions) . ')'; } /** diff --git a/src/Database/Adapter/SQL.php b/src/Database/Adapter/SQL.php index 9ca4c1aee..b73765bd5 100644 --- a/src/Database/Adapter/SQL.php +++ b/src/Database/Adapter/SQL.php @@ -3047,7 +3047,7 @@ public function find(Document $collection, array $queries = [], ?int $limit = 25 $prevOriginal = $orderAttributes[$j]; $prevAttr = $this->filter($this->getInternalKeyForAttribute($prevOriginal)); - $bindName = ":cursor_{$j}"; + $bindName = ":cursor_{$i}_{$j}"; $binds[$bindName] = $cursor[$prevOriginal]; $conditions[] = "{$this->quote($alias)}.{$this->quote($prevAttr)} = {$bindName}"; @@ -3118,7 +3118,9 @@ public function find(Document $collection, array $queries = [], ?int $limit = 25 $projection = $this->getAttributeProjection($selections, $alias); if (!empty($vectorDistances)) { - $readable = $this->getSQLReadableDistance($vectorDistances[0]); + // Built again so its vector binds under a name the ORDER BY doesn't use. + $distance = $this->getSQLVectorDistance($vectorQueries[0], $binds, $alias) ?? $vectorDistances[0]; + $readable = $this->getSQLReadableDistance($distance); $projection .= ", {$readable} AS {$this->quote(static::VECTOR_DISTANCE_COLUMN)}"; } diff --git a/tests/e2e/Adapter/PostgresTest.php b/tests/e2e/Adapter/PostgresTest.php index 14afc6db7..0b56924a7 100644 --- a/tests/e2e/Adapter/PostgresTest.php +++ b/tests/e2e/Adapter/PostgresTest.php @@ -128,6 +128,83 @@ public function testReadDoesNotTouchThePermissionsTable(): void $database->deleteCollection('permsPlan'); } + /** + * A reader holding many roles must still be answered from the permissions index when few + * documents are readable. Matching each role as its own clause made the planner add up a + * minimum estimate per role, so enough roles convinced it most of the collection was + * readable, and it walked the whole collection in order instead. + * + * Sequential scans are priced out of the session, as in the vector plan test, so the + * assertion measures the estimate rather than the size of the collection. + */ + public function testManyRolesUseThePermissionsIndex(): void + { + $database = $this->getDatabase(); + $authorization = $database->getAuthorization(); + + $database->createCollection('rolesPlan', permissions: [ + Permission::create(Role::any()), + ], documentSecurity: true); + + $database->createAttribute('rolesPlan', 'owner', Database::VAR_INTEGER, 0, true); + + $documents = []; + for ($i = 0; $i < 50000; $i++) { + $documents[] = new Document([ + '$permissions' => [Permission::read(Role::user("owner{$i}"))], + 'owner' => $i, + ]); + } + $database->createDocuments('rolesPlan', $documents, 1000); + + $table = $database->getNamespace() . '_rolesPlan'; + self::$pdo->exec("VACUUM ANALYZE \"{$database->getDatabase()}\".\"{$table}\""); + + $scans = function () use ($table): int { + self::$pdo->query('SELECT pg_stat_force_next_flush()'); + self::$pdo->query('SELECT pg_stat_clear_snapshot()'); + + $statement = self::$pdo->prepare(' + SELECT COALESCE(SUM(statistics.idx_scan), 0) + FROM pg_stat_user_indexes AS statistics + JOIN pg_class AS index ON index.oid = statistics.indexrelid + JOIN pg_am AS method ON method.oid = index.relam + WHERE statistics.relname = :table AND method.amname = \'gin\' + '); + $statement->execute([':table' => $table]); + + return (int)$statement->fetchColumn(); + }; + + $before = $scans(); + + self::$pdo->exec('SET enable_seqscan = off'); + + try { + for ($i = 0; $i < 60; $i++) { + $authorization->addRole(Role::user("stranger{$i}")->toString()); + } + $authorization->addRole(Role::user('owner7')->toString()); + + $results = $database->find('rolesPlan', [Query::limit(25)]); + } finally { + self::$pdo->exec('RESET enable_seqscan'); + $authorization->cleanRoles(); + $authorization->addRole(Role::any()->toString()); + } + + $this->assertCount(1, $results, 'Only the document owned by the reader may come back'); + $this->assertSame(7, $results[0]->getAttribute('owner')); + + $this->assertGreaterThan( + $before, + $scans(), + 'A selective read must be answered from the permissions index however many roles the reader holds' + ); + + $database->deleteCollection('rolesPlan'); + } + /** * A vector search must order by distance alone, because a vector index can answer exactly * one sort key. Adding a second one does not merely make the index look expensive, it makes