diff --git a/CLAUDE.md b/CLAUDE.md index 375e497f..0a7571d2 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -85,10 +85,12 @@ When modifying `__pgsqlite_*` tables: 2. Define migration with version, name, description, up/down SQL, and dependencies 3. Update Current Migrations list below -### Current Migrations (v1-v25) +### Current Migrations (v1-v28) - v1-v10: Initial schema, ENUM, DateTime, Arrays, Full-Text Search, catalog tables - v15-v19: pg_depend, pg_proc, pg_description, pg_roles/pg_user, pg_stats - v20-v25: information_schema support (routines, views, referential_constraints, check_constraints, triggers), pg_tablespace +- v26-v27: Enhanced pg_attribute, pg_proc type fixes +- v28: pg_class full column parity; internal relations moved to pg_catalog/information_schema namespaces ## Major Features diff --git a/docs/superpowers/plans/2026-08-10-pg-class-from-sqlite.md b/docs/superpowers/plans/2026-08-10-pg-class-from-sqlite.md new file mode 100644 index 00000000..4d76c61e --- /dev/null +++ b/docs/superpowers/plans/2026-08-10-pg-class-from-sqlite.md @@ -0,0 +1,977 @@ +# pg_class from SQLite Implementation Plan + +> **For agentic workers:** REQUIRED SUB-SKILL: Use superpowers:subagent-driven-development (recommended) or superpowers:executing-plans to implement this plan task-by-task. Steps use checkbox (`- [ ]`) syntax for tracking. + +**Goal:** Fix issue #87 (`\dt` reports "Did not find any tables") by deleting `PgClassHandler` and letting SQLite execute `pg_class` queries against an enriched view. + +**Architecture:** pgsqlite currently intercepts `pg_class` queries and evaluates them with a hand-rolled Rust engine that cannot handle `regexp()`, joined columns, or JOINs. The `pg_class` and `pg_namespace` SQLite views already exist, and `regexp`/`pg_table_is_visible`/`pg_get_userbyid` are already registered UDFs. We enrich the `pg_class` view to full column parity, assign internal relations to the `pg_catalog` namespace, then remove the interception branch so queries flow to SQLite. + +**Tech Stack:** Rust, rusqlite/SQLite, tokio, tokio-postgres (tests), sqlparser. + +**Spec:** `docs/superpowers/specs/2026-08-10-pg-class-sqlite-engine-design.md` + +## Global Constraints + +- Migration registry currently runs through **v27**. The new migration is **v28**. Do not reuse v26 — it exists and owns the current `pg_class` view. +- The canonical OID formula is the **unicode formula**: `((unicode(c1)*1000000) + (unicode(c2)*10000) + (unicode(c3)*100) + (len*7)) % 1000000 + 16384`, cast to TEXT. Do not introduce a new formula and do not migrate persisted OIDs. +- `pg_class.oid` is **TEXT**, matching `pg_constraint.conrelid` / `pg_index.indrelid` / `pg_attrdef.adrelid`. +- Namespace OIDs: `pg_catalog` = 11, `information_schema` = 13000, `public` = 2200. +- `relchecks` stays `0`. Do not derive it from `pg_constraint`. +- Do **not** modify `src/catalog/where_evaluator.rs`. Its `NOT`-inverts-unknown bug is filed separately (Task 8). +- Pre-commit checklist for every commit: `cargo check` (no warnings), `cargo clippy`, `cargo build`, `cargo test`. + +--- + +### Task 1: Failing regression test for #87 + +**Files:** +- Create: `tests/pg_class_dt_test.rs` + +**Interfaces:** +- Consumes: `common::setup_test_server_with_init` (existing harness; takes a closure returning a boxed future, yields a struct with a `.client` field that is a `tokio_postgres::Client`). +- Produces: nothing consumed by later tasks. This test is the acceptance gate for Task 4. + +- [ ] **Step 1: Write the failing test** + +Create `tests/pg_class_dt_test.rs`: + +```rust +mod common; +use common::setup_test_server_with_init; + +/// The exact query psql 18 expands `\dt` into. +const DT_QUERY: &str = r#" +SELECT n.nspname as "Schema", + c.relname as "Name", + CASE c.relkind WHEN 'r' THEN 'table' WHEN 'v' THEN 'view' WHEN 'm' THEN 'materialized view' WHEN 'i' THEN 'index' WHEN 'S' THEN 'sequence' WHEN 't' THEN 'TOAST table' WHEN 'f' THEN 'foreign table' WHEN 'p' THEN 'partitioned table' WHEN 'I' THEN 'partitioned index' END as "Type", + pg_catalog.pg_get_userbyid(c.relowner) as "Owner" +FROM pg_catalog.pg_class c + LEFT JOIN pg_catalog.pg_namespace n ON n.oid = c.relnamespace + LEFT JOIN pg_catalog.pg_am am ON am.oid = c.relam +WHERE c.relkind IN ('r','p','') + AND n.nspname <> 'pg_catalog' + AND n.nspname !~ '^pg_toast' + AND n.nspname <> 'information_schema' + AND pg_catalog.pg_table_is_visible(c.oid) +ORDER BY 1,2 +"#; + +#[tokio::test] +async fn test_dt_lists_user_tables() { + let _ = env_logger::builder().is_test(true).try_init(); + + let server = setup_test_server_with_init(|db| { + Box::pin(async move { + db.execute("CREATE TABLE customers (id INTEGER PRIMARY KEY, name TEXT)").await?; + Ok(()) + }) + }).await; + + let rows = server.client.query(DT_QUERY, &[]).await + .expect("\\dt query should succeed"); + + let names: Vec = rows.iter().map(|r| r.get::<_, String>("Name")).collect(); + + assert!(names.iter().any(|n| n == "customers"), + "issue #87: \\dt must list the user table, got {names:?}"); +} + +#[tokio::test] +async fn test_dt_hides_internal_relations() { + let _ = env_logger::builder().is_test(true).try_init(); + + let server = setup_test_server_with_init(|db| { + Box::pin(async move { + db.execute("CREATE TABLE customers (id INTEGER PRIMARY KEY, name TEXT)").await?; + Ok(()) + }) + }).await; + + let rows = server.client.query(DT_QUERY, &[]).await + .expect("\\dt query should succeed"); + + let names: Vec = rows.iter().map(|r| r.get::<_, String>("Name")).collect(); + + for internal in ["pg_constraint", "pg_attrdef", "pg_index", "pg_depend"] { + assert!(!names.iter().any(|n| n == internal), + "pgsqlite's own {internal} must not appear in \\dt, got {names:?}"); + } + + assert!(!names.is_empty(), + "must not pass vacuously against a 0-row result"); +} + +#[tokio::test] +async fn test_dt_reports_public_schema_and_owner() { + let _ = env_logger::builder().is_test(true).try_init(); + + let server = setup_test_server_with_init(|db| { + Box::pin(async move { + db.execute("CREATE TABLE customers (id INTEGER PRIMARY KEY, name TEXT)").await?; + Ok(()) + }) + }).await; + + let rows = server.client.query(DT_QUERY, &[]).await + .expect("\\dt query should succeed"); + + let row = rows.iter() + .find(|r| r.get::<_, String>("Name") == "customers") + .expect("customers row must be present"); + + assert_eq!(row.get::<_, String>("Schema"), "public"); + assert_eq!(row.get::<_, String>("Type"), "table"); +} +``` + +- [ ] **Step 2: Run the tests to verify they fail** + +Run: `cargo test --test pg_class_dt_test 2>&1 | tail -30` + +Expected: all three FAIL. `test_dt_lists_user_tables` fails on the `customers` assertion because the query returns 0 rows. `test_dt_hides_internal_relations` fails on the non-emptiness assertion ("must not pass vacuously against a 0-row result") because the query returns 0 rows. `test_dt_reports_public_schema_and_owner` fails on `expect("customers row must be present")`. + +If instead you see a panic about a missing `Schema` column, that is the same root cause (the interceptor drops projected columns) and still counts as RED. + +- [ ] **Step 3: Commit the failing test** + +```bash +git add tests/pg_class_dt_test.rs +git commit -m "test: failing regression test for \dt returning no tables (#87)" +``` + +--- + +### Task 2: Guarantee `trusted_schema=ON` on session connections + +**Files:** +- Modify: `src/session/db_handler.rs` (wherever `register_*_functions` are called on a new connection — locate with the grep in Step 1) +- Test: `tests/pg_class_dt_test.rs` (append) + +**Interfaces:** +- Consumes: nothing. +- Produces: the guarantee that `pragma_table_info()` may be used inside a view. Task 3's `relnatts` column depends on this. + +**Why:** `pragma_table_info` is a virtual table. SQLite refuses virtual tables inside views unless `trusted_schema` is ON. It is ON by default in the C API (so this works today), but the `sqlite3` CLI disables it and rejects the view with `unsafe use of virtual table "pragma_table_info"`. Making it explicit prevents a future default change from silently breaking `relnatts`. + +- [ ] **Step 1: Locate connection initialization** + +Run: `grep -rn "register_hash_functions\|register_regex_functions" src/session/db_handler.rs | head` + +Every site that opens a connection and registers UDFs needs the pragma. There may be more than one (in-memory vs file, pooled vs per-session). + +- [ ] **Step 2: Write the failing test** + +Append to `tests/pg_class_dt_test.rs`: + +```rust +#[tokio::test] +async fn test_trusted_schema_allows_pragma_in_views() { + let _ = env_logger::builder().is_test(true).try_init(); + + let server = setup_test_server_with_init(|db| { + Box::pin(async move { + db.execute("CREATE TABLE customers (id INTEGER PRIMARY KEY, name TEXT)").await?; + Ok(()) + }) + }).await; + + let rows = server.client.query("PRAGMA trusted_schema", &[]).await + .expect("PRAGMA trusted_schema should be readable"); + + assert_eq!(rows.len(), 1, "expected one row from PRAGMA trusted_schema"); + let enabled: i32 = rows[0].get(0); + assert_eq!(enabled, 1, "trusted_schema must be ON so views may call pragma_table_info()"); +} +``` + +- [ ] **Step 3: Run it** + +Run: `cargo test --test pg_class_dt_test test_trusted_schema_allows_pragma_in_views 2>&1 | tail -20` + +Expected: PASS if the C API default already applies. That is an acceptable outcome — the test is pinning existing behavior so it cannot regress. + +If it FAILS, or if `PRAGMA trusted_schema` does not round-trip through the protocol, replace the test body with a direct assertion instead: + +```rust + let rows = server.client + .query("SELECT COUNT(*) FROM pragma_table_info('customers')", &[]) + .await + .expect("pragma_table_info must be callable"); + let n: i64 = rows[0].get(0); + assert_eq!(n, 2, "customers has 2 columns"); +``` + +- [ ] **Step 4: Add the explicit pragma** + +At each connection-initialization site found in Step 1, immediately before the `register_*_functions` calls, add: + +```rust +conn.execute_batch("PRAGMA trusted_schema=ON;")?; +``` + +- [ ] **Step 5: Re-run the test** + +Run: `cargo test --test pg_class_dt_test test_trusted_schema_allows_pragma_in_views 2>&1 | tail -20` + +Expected: PASS. + +- [ ] **Step 6: Commit** + +```bash +git add src/session/db_handler.rs tests/pg_class_dt_test.rs +git commit -m "fix: set trusted_schema=ON so views may call pragma_table_info()" +``` + +--- + +### Task 3: Migration v28 — enriched `pg_class` view and `information_schema` namespace + +**Files:** +- Modify: `src/migration/registry.rs` (add the call after line 36 `register_v27_fix_pg_proc_types(&mut registry);`, and add the function at end of file) +- Test: `tests/pg_class_view_test.rs` (create) + +**Interfaces:** +- Consumes: `trusted_schema=ON` from Task 2. +- Produces: a `pg_class` view with these 33 columns, in this order — `oid, relname, relnamespace, reltype, reloftype, relowner, relam, relfilenode, reltablespace, relpages, reltuples, relallvisible, reltoastrelid, relhasindex, relisshared, relpersistence, relkind, relnatts, relchecks, relhasrules, relhastriggers, relhassubclass, relrowsecurity, relforcerowsecurity, relispopulated, relreplident, relispartition, relrewrite, relfrozenxid, relminmxid, relacl, reloptions, relpartbound`. Also a `pg_namespace` view with rows `(11, pg_catalog)`, `(2200, public)`, `(13000, information_schema)`. Task 4 relies on both. + +- [ ] **Step 1: Write the failing test** + +Create `tests/pg_class_view_test.rs`: + +```rust +mod common; +use common::setup_test_server_with_init; + +const ALL_PG_CLASS_COLUMNS: &str = "oid, relname, relnamespace, reltype, reloftype, \ +relowner, relam, relfilenode, reltablespace, relpages, reltuples, relallvisible, \ +reltoastrelid, relhasindex, relisshared, relpersistence, relkind, relnatts, relchecks, \ +relhasrules, relhastriggers, relhassubclass, relrowsecurity, relforcerowsecurity, \ +relispopulated, relreplident, relispartition, relrewrite, relfrozenxid, relminmxid, \ +relacl, reloptions, relpartbound"; + +#[tokio::test] +async fn test_pg_class_has_full_column_parity() { + let _ = env_logger::builder().is_test(true).try_init(); + + let server = setup_test_server_with_init(|db| { + Box::pin(async move { + db.execute("CREATE TABLE customers (id INTEGER PRIMARY KEY, name TEXT)").await?; + Ok(()) + }) + }).await; + + let sql = format!("SELECT {ALL_PG_CLASS_COLUMNS} FROM pg_catalog.pg_class WHERE relname = 'customers'"); + let rows = server.client.query(&sql, &[]).await + .expect("all 33 pg_class columns must be selectable"); + + assert_eq!(rows.len(), 1, "expected exactly one row for customers"); +} + +#[tokio::test] +async fn test_pg_class_namespace_assignment() { + let _ = env_logger::builder().is_test(true).try_init(); + + let server = setup_test_server_with_init(|db| { + Box::pin(async move { + db.execute("CREATE TABLE customers (id INTEGER PRIMARY KEY, name TEXT)").await?; + Ok(()) + }) + }).await; + + // Compare in SQL rather than binding integers in Rust: relnamespace is an + // INTEGER in the view, and its inferred wire type is not worth guessing. + let public_rows = server.client.query( + "SELECT relname FROM pg_catalog.pg_class WHERE relnamespace = 2200 AND relname = 'customers'", + &[] + ).await.expect("query should succeed"); + assert_eq!(public_rows.len(), 1, "user tables belong to the public namespace (2200)"); + + let catalog_rows = server.client.query( + "SELECT relname FROM pg_catalog.pg_class WHERE relnamespace = 11 AND relname = 'pg_constraint'", + &[] + ).await.expect("query should succeed"); + assert_eq!(catalog_rows.len(), 1, "internal pg_* relations belong to pg_catalog (11)"); + + let misfiled = server.client.query( + "SELECT relname FROM pg_catalog.pg_class WHERE relnamespace = 2200 AND relname LIKE 'pg\\_%'", + &[] + ).await.expect("query should succeed"); + assert!(misfiled.is_empty(), "no pg_* relation may remain in the public namespace"); +} + +#[tokio::test] +async fn test_pg_class_relnatts_is_real_column_count() { + let _ = env_logger::builder().is_test(true).try_init(); + + let server = setup_test_server_with_init(|db| { + Box::pin(async move { + db.execute("CREATE TABLE three_cols (a INTEGER, b TEXT, c REAL)").await?; + Ok(()) + }) + }).await; + + let rows = server.client.query( + "SELECT relname FROM pg_catalog.pg_class WHERE relname = 'three_cols' AND relnatts = 3", + &[] + ).await.expect("query should succeed"); + + assert_eq!(rows.len(), 1, "relnatts must reflect the real column count (3)"); +} + +#[tokio::test] +async fn test_pg_namespace_has_information_schema() { + let _ = env_logger::builder().is_test(true).try_init(); + + let server = setup_test_server_with_init(|db| { + Box::pin(async move { + db.execute("CREATE TABLE customers (id INTEGER PRIMARY KEY)").await?; + Ok(()) + }) + }).await; + + let rows = server.client.query( + "SELECT nspname FROM pg_catalog.pg_namespace ORDER BY nspname", &[] + ).await.expect("query should succeed"); + + let names: Vec = rows.iter().map(|r| r.get::<_, String>(0)).collect(); + + for expected in ["pg_catalog", "public", "information_schema"] { + assert!(names.iter().any(|n| n == expected), + "v28 must provide the {expected} namespace, got {names:?}"); + } + + // Confirm the oid values via SQL, avoiding an integer wire-type binding. + let is_row = server.client.query( + "SELECT nspname FROM pg_catalog.pg_namespace WHERE oid = 13000", &[] + ).await.expect("query should succeed"); + assert_eq!(is_row.len(), 1, "information_schema must have oid 13000"); +} +``` + +- [ ] **Step 2: Run to verify failure** + +Run: `cargo test --test pg_class_view_test 2>&1 | tail -30` + +Expected: FAIL. `test_pg_class_has_full_column_parity` fails because `reltype`/`relnatts`/etc. do not exist; `test_pg_namespace_has_information_schema` fails because there is no 13000 row. + +- [ ] **Step 3: Register the migration** + +In `src/migration/registry.rs`, directly after line 36 (`register_v27_fix_pg_proc_types(&mut registry);`), add: + +```rust + register_v28_pg_class_full_columns(&mut registry); +``` + +- [ ] **Step 4: Add the migration function** + +Append to the end of `src/migration/registry.rs`. The view SQL below was verified against SQLite before being written into this plan. + +```rust +/// Version 28: Serve pg_class from SQLite with full column parity. +/// Adds the nine columns PgClassHandler used to synthesize, assigns internal +/// pg_*/information_schema_* relations to their proper namespaces, and fixes +/// three pre-existing view bugs (relkind_full is not a real PostgreSQL column; +/// relreplident should be 'd'; relispartition should be 'f'). +fn register_v28_pg_class_full_columns(registry: &mut BTreeMap) { + registry.insert(28, Migration { + version: 28, + name: "pg_class_full_columns", + description: "Enrich pg_class view to full 33-column parity and namespace internal relations so SQLite can serve pg_class directly", + up: MigrationAction::SqlBatch(&[ + r#"DROP VIEW IF EXISTS pg_class"#, + r#"DROP VIEW IF EXISTS pg_namespace"#, + + r#" + CREATE VIEW pg_namespace AS + SELECT 11 as oid, 'pg_catalog' as nspname, 10 as nspowner, NULL as nspacl + UNION ALL + SELECT 2200 as oid, 'public' as nspname, 10 as nspowner, NULL as nspacl + UNION ALL + SELECT 13000 as oid, 'information_schema' as nspname, 10 as nspowner, NULL as nspacl + "#, + + r#" + CREATE VIEW pg_class AS + WITH base AS ( + SELECT name, type, + ((unicode(substr(name, 1, 1)) * 1000000) + + (unicode(substr(name || ' ', 2, 1)) * 10000) + + (unicode(substr(name || ' ', 3, 1)) * 100) + + (length(name) * 7)) % 1000000 + 16384 AS oid_num + FROM sqlite_master + WHERE type IN ('table', 'view', 'index') + AND name NOT LIKE 'sqlite_%' + AND name NOT LIKE '__pgsqlite_%' + ) + SELECT + CAST(oid_num AS TEXT) as oid, + name as relname, + CASE + WHEN name LIKE 'pg\_%' ESCAPE '\' THEN 11 + WHEN name LIKE 'information\_schema\_%' ESCAPE '\' THEN 13000 + ELSE 2200 + END as relnamespace, + CAST(oid_num + 1 AS TEXT) as reltype, + 0 as reloftype, + 10 as relowner, + CASE WHEN type = 'index' THEN 403 ELSE 0 END as relam, + 0 as relfilenode, + 0 as reltablespace, + 0 as relpages, + -1 as reltuples, + 0 as relallvisible, + 0 as reltoastrelid, + CASE WHEN type = 'table' THEN 't' ELSE 'f' END as relhasindex, + 'f' as relisshared, + 'p' as relpersistence, + CASE type + WHEN 'table' THEN 'r' + WHEN 'view' THEN 'v' + WHEN 'index' THEN 'i' + END as relkind, + (SELECT COUNT(*) FROM pragma_table_info(base.name)) as relnatts, + 0 as relchecks, + 'f' as relhasrules, + CASE WHEN EXISTS( + SELECT 1 FROM sqlite_master t + WHERE t.type = 'trigger' AND t.tbl_name = base.name + ) THEN 't' ELSE 'f' END as relhastriggers, + 'f' as relhassubclass, + 'f' as relrowsecurity, + 'f' as relforcerowsecurity, + 't' as relispopulated, + 'd' as relreplident, + 'f' as relispartition, + 0 as relrewrite, + 0 as relfrozenxid, + 0 as relminmxid, + NULL as relacl, + NULL as reloptions, + NULL as relpartbound + FROM base + "#, + + r#" + UPDATE __pgsqlite_metadata + SET value = '28', updated_at = strftime('%s', 'now') + WHERE key = 'schema_version'; + "#, + ]), + down: Some(MigrationAction::SqlBatch(&[ + r#"DROP VIEW IF EXISTS pg_class"#, + r#"DROP VIEW IF EXISTS pg_namespace"#, + + // Restore the v26 pg_class view: copy the CREATE VIEW block verbatim + // from register_v26_enhanced_pg_attribute_support's `up` + // (registry.rs lines 2544-2586, the block ending just before line 2587's `"#,`). + r#" + + "#, + + // Restore the two-row pg_namespace view: copy from registry.rs lines 270-281. + r#" + + "#, + + r#" + UPDATE __pgsqlite_metadata + SET value = '27', updated_at = strftime('%s', 'now') + WHERE key = 'schema_version'; + "#, + ])), + dependencies: vec![27], + }); +} +``` + +The two paste markers above are the only place in this plan where you supply +content: copy the exact SQL from the line ranges given. `register_v26_enhanced_pg_attribute_support` +restores its previous view in `down` (`registry.rs:138-146` of that function), so +v28 follows the same convention rather than leaving the database view-less on rollback. + +- [ ] **Step 5: Run the tests** + +Run: `cargo test --test pg_class_view_test 2>&1 | tail -30` + +Expected: `test_pg_namespace_has_information_schema` PASSES. The other three may still FAIL, because `PgClassHandler` still intercepts and shadows the view — that is removed in Task 4. If they still fail with "column does not exist", that confirms interception is still active and is the expected state at this point. + +- [ ] **Step 6: Verify the view directly, bypassing interception** + +Run: + +```bash +cargo test --test pg_class_view_test test_pg_namespace_has_information_schema 2>&1 | tail -5 +``` + +Expected: PASS. This is the one assertion in this task that does not depend on Task 4. + +- [ ] **Step 7: Commit** + +```bash +git add src/migration/registry.rs tests/pg_class_view_test.rs +git commit -m "feat: migration v28 enriches pg_class view to full column parity (#87)" +``` + +--- + +### Task 4: Remove `pg_class` and `pg_namespace` interception, delete `PgClassHandler` + +**Files:** +- Modify: `src/catalog/query_interceptor.rs:550-553` (remove the pg_class branch), `:515-517` (remove the pg_namespace branch), `:1063-1098` (delete `handle_pg_namespace_query`), `:12` (remove the import) +- Modify: `src/catalog/mod.rs` (remove the `pg_class` module declaration) +- Delete: `src/catalog/pg_class.rs` + +**Scope amendment (human ruling, made during execution):** the original plan +removed only the `pg_class` branch. Implementation of Task 3 revealed that +`pg_namespace` is *also* intercepted, by `handle_pg_namespace_query`, which +returns a hardcoded two-row set (`pg_catalog`, `public`) and therefore shadows +v28's new `information_schema` namespace row. Task 4 now removes both branches. +Issue #87 itself does not depend on this — `\dt`'s FROM table is `pg_class`, so +removing that branch alone lets the whole joined query fall through to SQLite — +but without it, a direct `pg_namespace` query still returns stale rows and +Task 3's `test_pg_namespace_has_information_schema` cannot pass. + +**Interfaces:** +- Consumes: the v28 views from Task 3. +- Produces: `pg_class` and `pg_namespace` queries reaching SQLite. This is what makes Task 1's tests pass. + +- [ ] **Step 1: Remove the interception branch** + +In `src/catalog/query_interceptor.rs`, delete these four lines at 550-553: + +```rust + // Handle pg_class queries + if table_name.contains("pg_class") || table_name.contains("pg_catalog.pg_class") { + return Some(PgClassHandler::handle_query(select, &db).await); + } +``` + +- [ ] **Step 2: Remove the pg_namespace branch and its handler** + +In `src/catalog/query_interceptor.rs`, delete the branch at 515-517: + +```rust + // Handle pg_namespace queries + if table_name.contains("pg_namespace") || table_name.contains("pg_catalog.pg_namespace") { + return Some(Ok(Self::handle_pg_namespace_query(select))); + } +``` + +Then delete the now-unused `handle_pg_namespace_query` function (around `:1063-1098`). Confirm nothing else calls it: + +```bash +grep -n "handle_pg_namespace_query" src/catalog/query_interceptor.rs +``` + +Expected after deletion: no matches. + +- [ ] **Step 3: Remove the import** + +In `src/catalog/query_interceptor.rs:12`, remove `pg_class::PgClassHandler, ` from the `use super::{...}` list. Leave the other handlers untouched. + +- [ ] **Step 4: Delete the handler and its module declaration** + +```bash +git rm src/catalog/pg_class.rs +grep -n "pub mod pg_class;\|mod pg_class;" src/catalog/mod.rs +``` + +Remove the matching line from `src/catalog/mod.rs`. + +- [ ] **Step 5: Build** + +Run: `cargo check 2>&1 | grep -E "^error|^warning: unused" | head -20` + +Expected: clean. If you see `unused import` or a dead-code warning for something that was only used by the deleted branches, remove it too. + +- [ ] **Step 6: Run the acceptance tests from Task 1** + +Run: `cargo test --test pg_class_dt_test 2>&1 | tail -30` + +Expected: all PASS. This closes #87. + +If `test_dt_lists_user_tables` still returns 0 rows, the query is being caught by an earlier interception branch rather than the one just removed. Diagnose with: + +```bash +RUST_LOG=pgsqlite=debug cargo test --test pg_class_dt_test test_dt_lists_user_tables 2>&1 | grep -i "INTERCEPT\|CHECK_TABLE_FACTOR" | head -20 +``` + +The candidates are the system-function branch at `query_interceptor.rs:334` (entered because `\dt` contains `pg_table_is_visible`) and the catalog-JOIN branch at `:402`. Neither should claim this query — `:341` requires the literal `pg_class.relname` and `:402` requires a join to `pg_attribute`/`pg_type` — but if the log shows one of them returning `Some`, that branch needs the same treatment. + +- [ ] **Step 7: Run the view tests from Task 3** + +Run: `cargo test --test pg_class_view_test 2>&1 | tail -30` + +Expected: all four PASS now that the views are no longer shadowed. Note that `test_pg_class_has_full_column_parity` and `test_pg_class_relnatts_is_real_column_count` already passed before this task (the deleted Rust handler also served 33 columns), so the meaningful changes here are `test_pg_class_namespace_assignment` and `test_pg_namespace_has_information_schema` going green. + +- [ ] **Step 8: Commit** + +```bash +git add -A src/catalog/ +git commit -m "fix: serve pg_class and pg_namespace from SQLite instead of Rust handlers (#87)" +``` + +--- + +### Task 5: Regression tests for the regex operators + +**Files:** +- Create: `tests/catalog_regex_operators_test.rs` + +**Interfaces:** +- Consumes: working `pg_class` from Task 4. +- Produces: nothing. + +**Why:** Investigation found two distinct regex bugs. `!~` excluded every row (the #87 cause) and `~` was silently *ignored*, returning unfiltered results with no error. The second is a wrong-answer bug that no existing test covers. + +- [ ] **Step 1: Write the tests** + +Create `tests/catalog_regex_operators_test.rs`: + +```rust +mod common; +use common::setup_test_server_with_init; + +async fn server_with_two_tables() -> common::TestServer { + setup_test_server_with_init(|db| { + Box::pin(async move { + db.execute("CREATE TABLE customers (id INTEGER PRIMARY KEY)").await?; + db.execute("CREATE TABLE orders (id INTEGER PRIMARY KEY)").await?; + Ok(()) + }) + }).await +} + +#[tokio::test] +async fn test_regex_match_actually_filters() { + let _ = env_logger::builder().is_test(true).try_init(); + let server = server_with_two_tables().await; + + let rows = server.client.query( + "SELECT relname FROM pg_catalog.pg_class WHERE relname ~ '^cust'", &[] + ).await.expect("~ query should succeed"); + + let names: Vec = rows.iter().map(|r| r.get::<_, String>(0)).collect(); + + assert!(names.iter().any(|n| n == "customers"), "~ must match customers, got {names:?}"); + assert!(!names.iter().any(|n| n == "orders"), + "~ must actually filter -- 'orders' does not match '^cust', got {names:?}"); +} + +#[tokio::test] +async fn test_regex_not_match_does_not_exclude_everything() { + let _ = env_logger::builder().is_test(true).try_init(); + let server = server_with_two_tables().await; + + let rows = server.client.query( + "SELECT relname FROM pg_catalog.pg_class WHERE relname !~ '^pg_toast'", &[] + ).await.expect("!~ query should succeed"); + + let names: Vec = rows.iter().map(|r| r.get::<_, String>(0)).collect(); + + assert!(names.iter().any(|n| n == "customers"), + "issue #87: !~ must not exclude non-matching rows, got {names:?}"); + assert!(names.iter().any(|n| n == "orders"), + "!~ must not exclude non-matching rows, got {names:?}"); +} + +#[tokio::test] +async fn test_regex_not_match_still_excludes_matches() { + let _ = env_logger::builder().is_test(true).try_init(); + let server = server_with_two_tables().await; + + let rows = server.client.query( + "SELECT relname FROM pg_catalog.pg_class WHERE relname !~ '^cust'", &[] + ).await.expect("!~ query should succeed"); + + let names: Vec = rows.iter().map(|r| r.get::<_, String>(0)).collect(); + + assert!(!names.iter().any(|n| n == "customers"), + "!~ '^cust' must exclude customers, got {names:?}"); + assert!(names.iter().any(|n| n == "orders"), + "!~ '^cust' must keep orders, got {names:?}"); +} +``` + +- [ ] **Step 2: Run** + +Run: `cargo test --test catalog_regex_operators_test 2>&1 | tail -30` + +Expected: all three PASS. + +- [ ] **Step 3: Commit** + +```bash +git add tests/catalog_regex_operators_test.rs +git commit -m "test: cover ~ and !~ operators on catalog tables (#87)" +``` + +--- + +### Task 6: Cross-catalog JOIN test + +**Files:** +- Create: `tests/catalog_join_test.rs` + +**Interfaces:** +- Consumes: working `pg_class` from Task 4. +- Produces: nothing. + +**Why:** Before this work, `pg_class JOIN pg_constraint ON con.conrelid = c.oid` returned all `pg_class` rows with no `conname` column — the join predicate was never evaluated. It also could not have matched, because `pg_class` served `hash31` OIDs while `pg_constraint` persists unicode-formula OIDs. Both are fixed by serving `pg_class` from the view. + +**Prerequisite discovered during execution — a fourth OID formula.** Task 4 unified the runtime OID producers onto `crate::utils::generate_table_oid` (the canonical unicode formula), but `src/catalog/pg_constraint.rs:256` has its own `generate_table_oid` using Rust's `DefaultHasher`, and it *synthesizes* `conrelid`/`confrelid` at query time (call sites `:171`, `:215`, `:219`), overriding the correct persisted values. Measured on a fresh database: `pg_class.oid` for `customers` is `197947`, while `pg_constraint.conrelid` is `51945`. The join returns zero rows. + +This must be fixed for the test below to pass. `DefaultHasher` is additionally unsuitable for OIDs because it is not guaranteed stable across Rust releases. The comment at `:257` claiming the formula is "same as pg_class handler" was never true. + +- [ ] **Step 0: Port `pg_constraint.rs` to the canonical OID function** + +Replace the body of `generate_table_oid` in `src/catalog/pg_constraint.rs:256-265` so it delegates to the canonical implementation, exactly as `pg_attribute.rs` and `constraint_populator.rs` now do: + +```rust + fn generate_table_oid(table_name: &str) -> u32 { + crate::utils::generate_table_oid(table_name) + } +``` + +Then confirm no other formula survives anywhere: + +```bash +grep -rn "DefaultHasher" src/catalog/ +grep -rn "wrapping_mul(31)" src/ +``` + +Expected: no OID-generating hits. (`src/functions/hash_functions.rs`'s `oid_hash` UDF is a separate, known-deferred issue — leave it.) + +- [ ] **Step 1: Write the test** + +Create `tests/catalog_join_test.rs`: + +```rust +mod common; +use common::setup_test_server_with_init; + +#[tokio::test] +async fn test_pg_class_joins_pg_constraint_on_oid() { + let _ = env_logger::builder().is_test(true).try_init(); + + let server = setup_test_server_with_init(|db| { + Box::pin(async move { + db.execute("CREATE TABLE customers (id INTEGER PRIMARY KEY, name TEXT)").await?; + Ok(()) + }) + }).await; + + let rows = server.client.query( + "SELECT c.relname, con.conname \ + FROM pg_catalog.pg_class c \ + JOIN pg_catalog.pg_constraint con ON con.conrelid = c.oid \ + WHERE c.relname = 'customers'", + &[] + ).await.expect("cross-catalog join should succeed"); + + assert!(!rows.is_empty(), + "pg_class.oid must match the persisted pg_constraint.conrelid"); + + for row in &rows { + let relname: String = row.get(0); + assert_eq!(relname, "customers"); + let _conname: String = row.get(1); + } +} + +#[tokio::test] +async fn test_pg_class_oid_matches_persisted_formula() { + let _ = env_logger::builder().is_test(true).try_init(); + + let server = setup_test_server_with_init(|db| { + Box::pin(async move { + db.execute("CREATE TABLE customers (id INTEGER PRIMARY KEY, name TEXT)").await?; + Ok(()) + }) + }).await; + + let rows = server.client.query( + "SELECT relname FROM pg_catalog.pg_class WHERE relname = 'customers' AND oid = '197947'", + &[] + ).await.expect("query should succeed"); + + // unicode formula for 'customers': + // c=99, u=117, s=115, len=9 + // (99*1000000 + 117*10000 + 115*100 + 63) % 1000000 + 16384 = 197947 + assert_eq!(rows.len(), 1, + "pg_class must use the canonical unicode OID formula that constraint_populator persists"); +} +``` + +- [ ] **Step 2: Run** + +Run: `cargo test --test catalog_join_test 2>&1 | tail -30` + +Expected: both PASS. + +If `test_pg_class_joins_pg_constraint_on_oid` returns zero rows, check whether `constraint_populator` actually ran for this table: `SELECT conrelid, conname FROM pg_constraint` via a direct SQLite read of the test database. A table with only a PRIMARY KEY should still produce a `customers_pkey` row. + +- [ ] **Step 3: Commit** + +```bash +git add tests/catalog_join_test.rs +git commit -m "test: cover cross-catalog joins through pg_class.oid (#87)" +``` + +--- + +### Task 7: Full suite, existing-test fallout, and docs + +**Files:** +- Modify: whichever existing tests assert literal `pg_class` OID values (identified in Step 2) +- Modify: `CLAUDE.md` (migration list) + +**Interfaces:** +- Consumes: everything above. +- Produces: a green suite. + +- [ ] **Step 1: Run the full suite** + +Run: `cargo test 2>&1 | tail -40` + +- [ ] **Step 2: Triage failures** + +Expected fallout, per the spec: tests asserting a literal `pg_class` OID now see unicode-formula values instead of `hash31`. Find them: + +```bash +cargo test 2>&1 | grep -E "^test .* FAILED|panicked at" | head -30 +grep -rn "generate_oid_from_name\|16384" tests/ --include=*.rs | head -20 +``` + +For each, update the expected OID to the unicode-formula value. Do **not** change the view to match an old test — the new values are the ones `pg_constraint`/`pg_index`/`pg_attrdef` already use on disk. + +Any other failure is *not* expected fallout. Stop and investigate before changing the test. + +- [ ] **Step 3: Update the stale migration list in CLAUDE.md** + +`CLAUDE.md` says "Current Migrations (v1-v25)", which was already wrong (v26 and v27 exist). Update that section to v28 and add a line describing v28: + +```markdown +### Current Migrations (v1-v28) +- v1-v10: Initial schema, ENUM, DateTime, Arrays, Full-Text Search, catalog tables +- v15-v19: pg_depend, pg_proc, pg_description, pg_roles/pg_user, pg_stats +- v20-v25: information_schema support (routines, views, referential_constraints, check_constraints, triggers), pg_tablespace +- v26-v27: Enhanced pg_attribute, pg_proc type fixes +- v28: pg_class full column parity; internal relations moved to pg_catalog/information_schema namespaces +``` + +- [ ] **Step 4: Pre-commit checklist** + +Run each, and fix what it reports: + +```bash +cargo check +cargo clippy +cargo build +cargo test +``` + +- [ ] **Step 5: Commit** + +```bash +git add -A +git commit -m "chore: update tests and docs for pg_class OID and migration v28 (#87)" +``` + +--- + +### Task 8: File the follow-up issues + +**Files:** none — this task creates GitHub issues. + +**Interfaces:** +- Consumes: nothing. +- Produces: nothing. + +The spec deliberately deferred three problems. File them so they are not lost. + +- [ ] **Step 1: File the OID collision issue** + +```bash +gh issue create --title "pg_class OIDs collide for names sharing first 3 chars and length" --body "$(cat <<'EOF' +The canonical OID formula reads only the first three characters and the length: + +``` +((unicode(c1)*1000000) + (unicode(c2)*10000) + (unicode(c3)*100) + (len*7)) % 1000000 + 16384 +``` + +Any two relations sharing those produce one OID. Demonstrated: + +| Names | Shared OID | +| --- | --- | +| `orders` / `orderz` | 166426 | +| `user_roles` / `user_rolez` | 176554 | +| `customers` / `customerz` | 197947 | + +OID is the join key from `pg_class` to `pg_attribute`, `pg_index`, `pg_constraint`, and `pg_depend`, so an ORM introspecting `orders` can receive `orderz`'s columns. + +Pre-existing, not introduced by #87. Fixing it means one canonical low-collision UDF used by every view and Rust site, plus a data migration rewriting persisted OIDs in `pg_constraint.conrelid`, `pg_index.indrelid`, `pg_attrdef.adrelid`, and `pg_depend.objid`/`refobjid`. + +Related: `oid_hash` (`src/functions/hash_functions.rs:25`) uses Rust's `DefaultHasher`, which is not guaranteed stable across Rust releases, so OIDs derived from it could change on rebuild. It should be retired in the same work. +EOF +)" +``` + +- [ ] **Step 2: File the WhereEvaluator issue** + +```bash +gh issue create --title "WhereEvaluator: unknown predicates flip to exclusion under NOT" --body "$(cat <<'EOF' +`WhereEvaluator::evaluate` returns `true` for anything it cannot evaluate — "default to including the row" (`src/catalog/where_evaluator.rs:53-56`). Under `NOT`, `!true` becomes `false` (`:151`), so an unevaluable predicate silently *excludes* every row. + +This was the root cause of #87 for `pg_class`, which no longer uses this code path. The ~20 remaining catalog handlers still do. + +Demonstrated with an arbitrary unknown function: + +| Query | Result | +| --- | --- | +| `WHERE foobar(c.relname)` | all rows | +| `WHERE NOT foobar(c.relname)` | 0 rows | + +Suggested fix: make evaluation tri-state (`Option`, `None` = cannot evaluate). `NOT None` stays `None`; `None AND x` is `x`; the top level treats `None` as include. Keep the current `evaluate() -> bool` as a thin `evaluate_opt().unwrap_or(true)` wrapper so the 22 call sites are untouched. + +A related gap: joined columns do not resolve at all. `n.nspname = 'public'` returns 0 rows and `n.nspname <> 'public'` returns everything — both wrong. +EOF +)" +``` + +- [ ] **Step 3: File the next-catalog migration issue** + +```bash +gh issue create --title "Migrate remaining catalog handlers to SQLite views" --body "$(cat <<'EOF' +#87 established the pattern: enrich the SQLite view to full column parity, then delete the Rust handler and its interception branch, so SQLite executes joins, WHERE, regex, and projection. + +`src/catalog/` is ~9,300 lines of hand-rolled query engine over relations that mostly already exist as views. Each remaining handler is a source of silent wrong-answer bugs of the kind #87 documented. + +Suggested order, most-used first: `pg_attribute`, `pg_proc`, `pg_description`, `pg_roles`/`pg_user`, `pg_stats`. + +Prerequisite for handlers whose views do not yet exist: create the view first. + +See `docs/superpowers/specs/2026-08-10-pg-class-sqlite-engine-design.md`. +EOF +)" +``` + +- [ ] **Step 4: Comment on #88** + +Moving internal relations into the `pg_catalog` namespace (v28) is the same mechanism #88 needs. + +```bash +gh issue comment 88 --body "Migration v28 (from #87) assigns \`pg_%\` relations to the \`pg_catalog\` namespace (oid 11) and \`information_schema_%\` to a new \`information_schema\` namespace (oid 13000) in the \`pg_class\` view. Clients filtering on \`nspname\` — which \`\\dt\` and most ORMs do — no longer see them in \`public\`. + +This does not close #88 on its own: \`information_schema.tables\` builds its own row set and needs the same namespace awareness." +``` + +- [ ] **Step 5: Close #87** + +```bash +gh issue close 87 --comment "Fixed by serving pg_class from SQLite instead of PgClassHandler. Root cause: RegexTranslator rewrote \`n.nspname !~ '^pg_toast'\` into \`NOT regexp(...)\` before catalog interception; WhereEvaluator defaulted the unknown function to true; NOT inverted that into a universal row filter. + +Also fixed: \`~\` was silently ignored on catalog tables (returning unfiltered results), cross-catalog joins through \`pg_class.oid\` never matched because pg_class served hash31 OIDs while pg_constraint/pg_index/pg_attrdef persist unicode-formula OIDs, and \`\\dt\` no longer lists pgsqlite's own pg_* relations. + +Design: \`docs/superpowers/specs/2026-08-10-pg-class-sqlite-engine-design.md\`" +``` diff --git a/docs/superpowers/specs/2026-08-10-pg-class-sqlite-engine-design.md b/docs/superpowers/specs/2026-08-10-pg-class-sqlite-engine-design.md new file mode 100644 index 00000000..5f97e16c --- /dev/null +++ b/docs/superpowers/specs/2026-08-10-pg-class-sqlite-engine-design.md @@ -0,0 +1,257 @@ +# Serve `pg_class` from SQLite instead of a Rust handler + +Issue: [#87](https://github.com/erans/pgsqlite/issues/87) +Date: 2026-08-10 +Status: Approved, ready for implementation planning + +## Problem + +`\dt` reports "Did not find any tables" on a database that has tables. The table +exists and is visible through other routes — `SELECT name FROM sqlite_master` +returns it, and so does `information_schema.tables`. + +### Root cause + +psql expands `\dt` to a query containing `n.nspname !~ '^pg_toast'`. Three +mechanisms combine to turn that predicate into a universal row filter: + +1. `RegexTranslator` runs *before* catalog interception + (`src/catalog/query_interceptor.rs:101`) and rewrites the regex operator into + a function call. Confirmed in the debug log: + + ``` + INTERCEPT: RegexTranslator changed query to: + 'SELECT c.relname FROM pg_class AS c WHERE NOT regexp('^pg_toast', c.relname)' + ``` + +2. `WhereEvaluator` has no case for `regexp()`. Unknown functions fall through to + `true` — "default to including the row" + (`src/catalog/where_evaluator.rs:53-56`). + +3. `NOT` inverts that default into an exclusion + (`src/catalog/where_evaluator.rs:151`): `!true` is `false`. Every row fails. + +The dedicated `PGRegexMatch` / `PGRegexNotMatch` arms at +`where_evaluator.rs:131-136` are dead code on this path — the operator is gone +before the AST reaches them. + +The mechanism was confirmed by prediction rather than by reading alone. An +arbitrary unknown function behaves identically: + +| Query | Predicted | Actual | +| --- | --- | --- | +| `WHERE foobar(c.relname)` | 13 rows | 13 rows | +| `WHERE NOT foobar(c.relname)` | 0 rows | 0 rows | +| `WHERE c.relname ~ '^cust'` | all rows (ignored) | 13 rows | +| `WHERE c.relname !~ '^pg_toast'` | 0 rows | 0 rows | + +The third row is a second, unreported bug: **`~` is silently ignored on catalog +tables**, returning unfiltered results with no error. + +### Two further defects behind it + +A narrow fix to the regex handling alone would not fix `\dt`. + +**Joined columns do not resolve.** `PgClassHandler` synthesizes only `pg_class` +columns, so the `LEFT JOIN pg_namespace` is never materialized and `n.nspname` +evaluates to `None`: + +| Query | Correct | Actual | +| --- | --- | --- | +| `n.nspname = 'public'` | 5 rows | 0 rows | +| `n.nspname <> 'public'` | 0 rows | 5 rows | + +Both are wrong. `\dt`'s `n.nspname <> 'pg_catalog'` passes only by accident +(`None != Some(..)` is true). Once `regexp` were understood, +`evaluate_regex_match` returns `false` for an unresolvable value +(`where_evaluator.rs:267-269`) and `\dt` would still return zero rows. + +**JOINs are not executed at all.** `pg_class JOIN pg_constraint ON con.conrelid += c.oid` returns all 13 `pg_class` rows with no `conname` column. The join +predicate is never evaluated. + +### The underlying shape + +`src/catalog/` is ~9,300 lines of hand-rolled query engine — WHERE evaluation, +projection, join handling — sitting on top of catalog relations that already +exist as real SQLite views. Every gap in that reimplementation is a silent +wrong-answer bug. #87 is one symptom. + +## Approach + +Delete `PgClassHandler` and let SQLite execute `pg_class` queries. Joins, `WHERE`, +`regexp()`, `ORDER BY`, and projection all become the engine's job. + +Everything required is already present, verified end-to-end: + +- `pg_class` and `pg_namespace` exist as SQLite views (`migration/registry.rs`). +- `regexp`, `pg_table_is_visible`, and `pg_get_userbyid` are registered UDFs + (`functions/regex_functions.rs:11`, `functions/catalog_functions.rs:11`, + `functions/system_functions.rs:373`). +- `SchemaPrefixTranslator` and `RegexTranslator` already run on the + non-intercepted path (`query/unified_processor.rs:390,430`). + +Run against the raw views, the `\dt` query returns the correct answer today: + +``` +Schema Name kind +------ ------------- ---- +public customers r +``` + +Rejected alternatives: + +- **Bail out of interception on hard queries** — the bail-list is itself a + guess-list, and any gap means interception happens when it shouldn't, i.e. the + wrong answers persist. +- **Try SQLite, fall back to Rust on error** — a query that *succeeds* but + returns wrong data never triggers the fallback, so silent bugs survive. + +Deleting the handler leaves one code path and no decision logic to drift. A +missing column becomes a loud SQLite error rather than silent wrong rows. + +## Scope + +`pg_class` and `pg_namespace`. The other catalog handlers keep working +untouched. This establishes the pattern for later per-catalog specs. + +`pg_namespace` was added to the scope during execution: it is intercepted +separately by `handle_pg_namespace_query`, which returns a hardcoded two-row set +and would otherwise shadow the new `information_schema` namespace row. Issue #87 +does not depend on this — `\dt` enters through `pg_class` — but the namespace +assignment below is unreachable without it. + +## Design + +### Migration v28: recreate the `pg_class` view + +The migration registry currently runs through v27 (`register_v27_fix_pg_proc_types`), +so this work lands as **v28**. The current `pg_class` view is owned by v26 +(`register_v26_enhanced_pg_attribute_support`, `registry.rs:2544`), so v28 must +`DROP VIEW IF EXISTS pg_class` before recreating it. Note that CLAUDE.md's +"Current Migrations (v1-v25)" list is stale and should be corrected to v28 as +part of this work. + +The current view has 25 columns; the Rust handler serves 33. Add the nine +missing: `reltype`, `reloftype`, `relnatts`, `relchecks`, `relhasrules`, +`relhastriggers`, `relhassubclass`, `relrowsecurity`, `relforcerowsecurity`. + +Computed rather than hardcoded: + +| Column | Source | +| --- | --- | +| `relnatts` | `(SELECT COUNT(*) FROM pragma_table_info(m.name))` | +| `relhastriggers` | `EXISTS(SELECT 1 FROM sqlite_master t WHERE t.type='trigger' AND t.tbl_name = m.name)` | +| `reltype` | `oid + 1`, matching current Rust behavior | +| `relkind` | `table` → `r`, `view` → `v`, `index` → `i` | + +`relchecks` stays `0`, matching today's Rust behavior. Deriving it from +`pg_constraint` is out of scope. + +Three existing view bugs are fixed in passing: + +- `relkind_full` is not a real PostgreSQL column — drop it. +- `relreplident` should be `'d'`, not `'v'`. +- `relispartition` should be `'f'`, not `'t'`. + +### Namespace assignment + +`relnamespace` becomes conditional so psql's own `n.nspname <> 'pg_catalog'` +predicate hides internal relations, with no special-casing anywhere in pgsqlite: + +- `pg_%` → `11` (`pg_catalog`) +- `information_schema_%` → `13000` (`information_schema`) +- everything else → `2200` (`public`) + +`information_schema` is a new row in the `pg_namespace` view — one extra +`UNION ALL`, and more correct than lumping those relations into `pg_catalog`. + +`sqlite_%` and `__pgsqlite_%` remain excluded from the view entirely, as today. + +Accepted limitation: a user table legitimately named `pg_foo` is misfiled into +`pg_catalog`. PostgreSQL reserves the prefix, so this is acceptable. + +### Remove the interception branch + +Delete the `pg_class` branch from `query_interceptor.rs`, and delete +`src/catalog/pg_class.rs` (372 lines) including its `generate_oid_from_name`. + +### OID consistency + +No formula change, and no data migration. + +Three OID formulas exist in the codebase today: + +| Formula | Used by | Stable? | +| --- | --- | --- | +| unicode of first 3 chars + length | views, `constraint_populator.rs:97` | yes | +| `hash31` | `pg_class.rs`, `pg_attribute.rs`, `pg_sequence.rs`, `pg_trigger.rs` | yes | +| `oid_hash` UDF (Rust `DefaultHasher`) | registered, little used | **no** | + +OIDs are persisted: `pg_constraint.conrelid`, `pg_index.indrelid`, +`pg_attrdef.adrelid`, and `pg_depend.objid`/`refobjid` all store table OIDs. The +persisted values use the unicode formula, while `PgClassHandler` serves `hash31` +— so `pg_class` and the stored catalogs already disagree today. Measured on a +fresh database, `pg_class` reports `customers` as `578453`, while the unicode +formula used for persisted OIDs yields `197947`. + +Serving `pg_class` from the view therefore *removes* an existing inconsistency. +The unicode formula becomes canonical because it is already the truth on disk. + +Because `pg_class` OIDs change value (from `hash31` to unicode), any existing +test asserting a specific `pg_class` OID must be updated. This is expected fallout, +not a regression — the new values are the ones the rest of the catalog already uses. + +Two known problems are deliberately left alone and filed separately: + +- The unicode formula collides whenever two names share their first three + characters and length. Demonstrated: `orders`/`orderz`, `user_roles`/`user_rolez`, + and `customers`/`customerz` each produce one OID. Pre-existing, not introduced + here. +- `oid_hash` uses `DefaultHasher`, which is not stable across Rust releases, so + OIDs could change on rebuild. + +### Deliberately not fixed + +`WhereEvaluator`'s `NOT`-inverts-unknown bug (`where_evaluator.rs:53-56,151`) +still affects the ~20 remaining handlers. Fixing it in this change would be +unverifiable — with `pg_class` gone, the `\dt` test cannot exercise it. It +belongs to whichever catalog migrates next, or to its own issue. + +## Failure modes + +**A column the view lacks.** Surfaces as a loud SQLite `no such column` error, +not silent wrong rows — the property that makes this approach preferable to the +rejected alternatives. Guarded by a test asserting all 33 columns are selectable. + +**`trusted_schema`.** `pragma_table_info` inside a view requires +`trusted_schema=ON`. That is the C API default and pgsqlite does not override +it, but the dependency is implicit: the `sqlite3` CLI disables it and rejects the +view with `unsafe use of virtual table "pragma_table_info"`. Set the pragma +explicitly where the per-session connection registers its UDFs, and cover it with +a test. + +**Performance.** Two correlated subqueries per row. `pg_class` is small; +acceptable. + +## Testing + +Written test-first, beginning with a failing test for #87. + +1. Regression test for #87: psql's exact `\dt` query returns `customers` and not + `pg_constraint`, `pg_attrdef`, `pg_index`, or `pg_depend`. +2. `~` filters correctly, and `!~` does not zero out results — the two bugs found + during investigation. +3. Cross-catalog join `pg_class JOIN pg_constraint ON conrelid = oid` returns + rows. +4. All 33 columns are selectable; `relnatts` matches real column counts. +5. The 34 existing catalog test files pass, with the sole expected exception of + assertions on literal `pg_class` OID values, which move to the unicode formula. + +## Follow-up issues to file + +- Collision-resistant, stable OID function shared by every view and Rust site; + retire `oid_hash`'s `DefaultHasher`. +- `WhereEvaluator` unknown-predicate handling under `NOT`, for the remaining + handlers. +- Migrate the next catalog handler to SQLite using this pattern. diff --git a/src/catalog/constraint_populator.rs b/src/catalog/constraint_populator.rs index 6ee30da3..2cd9fe75 100644 --- a/src/catalog/constraint_populator.rs +++ b/src/catalog/constraint_populator.rs @@ -82,20 +82,10 @@ fn get_create_table_sql(conn: &Connection, table_name: &str) -> Result { /// Generate table OID using the same algorithm as the pg_class view fn generate_table_oid(name: &str) -> String { - // Must match the formula in pg_class view for JOIN compatibility: - // (unicode(substr(name, 1, 1)) * 1000000) + - // (unicode(substr(name || ' ', 2, 1)) * 10000) + - // (unicode(substr(name || ' ', 3, 1)) * 100) + - // (length(name) * 7) - let name_with_padding = format!("{} ", name); - let chars: Vec = name_with_padding.chars().collect(); - let char1 = chars.get(0).copied().unwrap_or(' ') as u32; - let char2 = chars.get(1).copied().unwrap_or(' ') as u32; - let char3 = chars.get(2).copied().unwrap_or(' ') as u32; - let length = name.len() as u32; - - let oid = ((char1 * 1000000) + (char2 * 10000) + (char3 * 100) + (length * 7)) % 1000000 + 16384; - oid.to_string() + // Must match the formula in pg_class view for JOIN compatibility. Delegates to the + // shared canonical implementation in crate::utils so every OID producer in the tree + // (views, catalog handlers, and this populator) agrees. + crate::utils::generate_table_oid(name).to_string() } /// Generate constraint OID with better collision avoidance diff --git a/src/catalog/mod.rs b/src/catalog/mod.rs index 43d343f3..23e312cc 100644 --- a/src/catalog/mod.rs +++ b/src/catalog/mod.rs @@ -1,6 +1,5 @@ // Module for system catalog implementation pub mod query_interceptor; -pub mod pg_class; pub mod pg_attribute; pub mod pg_constraint; pub mod pg_depend; diff --git a/src/catalog/pg_attribute.rs b/src/catalog/pg_attribute.rs index b0ea1fd9..f8ed24c7 100644 --- a/src/catalog/pg_attribute.rs +++ b/src/catalog/pg_attribute.rs @@ -624,11 +624,7 @@ fn map_sqlite_to_pg_type(sqlite_type: &str) -> (i32, i16, i32) { } fn generate_oid_from_name(name: &str) -> u32 { - // Generate a stable OID from name using a simple hash - // Start at 16384 to avoid conflicts with system OIDs - let mut hash = 0u32; - for byte in name.bytes() { - hash = hash.wrapping_mul(31).wrapping_add(byte as u32); - } - 16384 + (hash % 1000000) + // Must match the formula used by the pg_class view (migration v28) and + // constraint_populator::generate_table_oid, so that attrelid joins to pg_class.oid. + crate::utils::generate_table_oid(name) } \ No newline at end of file diff --git a/src/catalog/pg_class.rs b/src/catalog/pg_class.rs deleted file mode 100644 index 65c304be..00000000 --- a/src/catalog/pg_class.rs +++ /dev/null @@ -1,373 +0,0 @@ -use crate::session::db_handler::{DbHandler, DbResponse}; -use crate::PgSqliteError; -use sqlparser::ast::{Select, SelectItem, Expr}; -use tracing::debug; -use std::collections::HashMap; -use super::where_evaluator::WhereEvaluator; - -pub struct PgClassHandler; - -impl PgClassHandler { - pub async fn handle_query( - select: &Select, - db: &DbHandler, - ) -> Result { - debug!("Handling pg_class query"); - - // Get list of tables from SQLite - let tables_response = db.query("SELECT name FROM sqlite_master WHERE type='table' AND name NOT LIKE 'sqlite_%' AND name NOT LIKE '__pgsqlite_%'").await?; - - // Define all available columns - PostgreSQL has 33 columns in pg_class - let all_columns = vec![ - "oid".to_string(), - "relname".to_string(), - "relnamespace".to_string(), - "reltype".to_string(), - "reloftype".to_string(), - "relowner".to_string(), - "relam".to_string(), - "relfilenode".to_string(), - "reltablespace".to_string(), - "relpages".to_string(), - "reltuples".to_string(), - "relallvisible".to_string(), - "reltoastrelid".to_string(), - "relhasindex".to_string(), - "relisshared".to_string(), - "relpersistence".to_string(), - "relkind".to_string(), - "relnatts".to_string(), - "relchecks".to_string(), - "relhasrules".to_string(), - "relhastriggers".to_string(), - "relhassubclass".to_string(), - "relrowsecurity".to_string(), - "relforcerowsecurity".to_string(), - "relispopulated".to_string(), - "relreplident".to_string(), - "relispartition".to_string(), - "relrewrite".to_string(), - "relfrozenxid".to_string(), - "relminmxid".to_string(), - "relacl".to_string(), - "reloptions".to_string(), - "relpartbound".to_string(), - ]; - - // Determine which columns to return based on projection - let (columns, column_indices) = Self::get_projected_columns(select, &all_columns); - - // Create column mapping for WHERE evaluation (uses all columns) - let column_mapping: HashMap = all_columns - .iter() - .enumerate() - .map(|(i, name)| (name.clone(), i)) - .collect(); - - let mut rows = Vec::new(); - - // Process each table - for table_row in &tables_response.rows { - if let Some(Some(table_name_bytes)) = table_row.first() { - let table_name = String::from_utf8_lossy(table_name_bytes); - - // Get column count for this table - let col_count_query = format!("PRAGMA table_info({table_name})"); - let col_info = db.query(&col_count_query).await?; - let relnatts = col_info.rows.len() as i16; - - // Generate a stable OID from table name - let oid = generate_oid_from_name(&table_name); - - // Check if table has indexes - let index_query = format!("PRAGMA index_list({table_name})"); - let index_info = db.query(&index_query).await?; - let relhasindex = !index_info.rows.is_empty(); - - // Build row data for WHERE evaluation - let mut row_data = HashMap::new(); - row_data.insert("oid".to_string(), oid.to_string()); - row_data.insert("relname".to_string(), table_name.to_string()); - row_data.insert("relnamespace".to_string(), "2200".to_string()); - row_data.insert("reltype".to_string(), (oid + 1).to_string()); - row_data.insert("reloftype".to_string(), "0".to_string()); - row_data.insert("relowner".to_string(), "10".to_string()); - row_data.insert("relam".to_string(), "0".to_string()); - row_data.insert("relfilenode".to_string(), oid.to_string()); - row_data.insert("reltablespace".to_string(), "0".to_string()); - row_data.insert("relpages".to_string(), "0".to_string()); - row_data.insert("reltuples".to_string(), "-1".to_string()); - row_data.insert("relallvisible".to_string(), "0".to_string()); - row_data.insert("reltoastrelid".to_string(), "0".to_string()); - row_data.insert("relhasindex".to_string(), if relhasindex { "t" } else { "f" }.to_string()); - row_data.insert("relisshared".to_string(), "f".to_string()); - row_data.insert("relpersistence".to_string(), "p".to_string()); - row_data.insert("relkind".to_string(), "r".to_string()); - row_data.insert("relnatts".to_string(), relnatts.to_string()); - row_data.insert("relchecks".to_string(), "0".to_string()); - row_data.insert("relhasrules".to_string(), "f".to_string()); - row_data.insert("relhastriggers".to_string(), "f".to_string()); - row_data.insert("relhassubclass".to_string(), "f".to_string()); - row_data.insert("relrowsecurity".to_string(), "f".to_string()); - row_data.insert("relforcerowsecurity".to_string(), "f".to_string()); - row_data.insert("relispopulated".to_string(), "t".to_string()); - row_data.insert("relreplident".to_string(), "d".to_string()); - row_data.insert("relispartition".to_string(), "f".to_string()); - row_data.insert("relrewrite".to_string(), "0".to_string()); - row_data.insert("relfrozenxid".to_string(), "0".to_string()); - row_data.insert("relminmxid".to_string(), "0".to_string()); - row_data.insert("relacl".to_string(), "".to_string()); - row_data.insert("reloptions".to_string(), "".to_string()); - row_data.insert("relpartbound".to_string(), "".to_string()); - - // Evaluate WHERE clause if present - let include_row = if let Some(selection) = &select.selection { - let result = WhereEvaluator::evaluate(selection, &row_data, &column_mapping); - debug!("WHERE evaluation for table '{}': {} (selection: {:?})", table_name, result, selection); - result - } else { - true - }; - - if include_row { - // Build full row with all columns (33 total) - let full_row = vec![ - Some(oid.to_string().into_bytes()), // oid - Some(table_name.to_string().into_bytes()), // relname - Some("2200".to_string().into_bytes()), // relnamespace (public schema) - Some((oid + 1).to_string().into_bytes()), // reltype - Some("0".to_string().into_bytes()), // reloftype - Some("10".to_string().into_bytes()), // relowner (postgres user) - Some("0".to_string().into_bytes()), // relam (0 for tables) - Some(oid.to_string().into_bytes()), // relfilenode - Some("0".to_string().into_bytes()), // reltablespace - Some("0".to_string().into_bytes()), // relpages - Some("-1".to_string().into_bytes()), // reltuples - Some("0".to_string().into_bytes()), // relallvisible - Some("0".to_string().into_bytes()), // reltoastrelid - Some(if relhasindex { b"t".to_vec() } else { b"f".to_vec() }), // relhasindex - Some(b"f".to_vec()), // relisshared - Some(b"p".to_vec()), // relpersistence (permanent) - Some(b"r".to_vec()), // relkind (regular table) - Some(relnatts.to_string().into_bytes()), // relnatts - Some("0".to_string().into_bytes()), // relchecks - Some(b"f".to_vec()), // relhasrules - Some(b"f".to_vec()), // relhastriggers - Some(b"f".to_vec()), // relhassubclass - Some(b"f".to_vec()), // relrowsecurity - Some(b"f".to_vec()), // relforcerowsecurity - Some(b"t".to_vec()), // relispopulated - Some(b"d".to_vec()), // relreplident (default) - Some(b"f".to_vec()), // relispartition - Some("0".to_string().into_bytes()), // relrewrite - Some("0".to_string().into_bytes()), // relfrozenxid - Some("0".to_string().into_bytes()), // relminmxid - None, // relacl (NULL) - None, // reloptions (NULL) - None, // relpartbound (NULL) - ]; - - // Project only the requested columns - let projected_row: Vec>> = column_indices.iter() - .map(|&idx| full_row[idx].clone()) - .collect(); - - rows.push(projected_row); - } - } - } - - // Also add indexes to pg_class - let indexes_response = db.query("SELECT name, tbl_name FROM sqlite_master WHERE type='index' AND name NOT LIKE 'sqlite_%'").await?; - - for index_row in &indexes_response.rows { - if let (Some(Some(index_name_bytes)), Some(Some(table_name_bytes))) = - (index_row.first(), index_row.get(1)) { - let index_name = String::from_utf8_lossy(index_name_bytes); - let table_name = String::from_utf8_lossy(table_name_bytes); - - let index_oid = generate_oid_from_name(&index_name); - let _table_oid = generate_oid_from_name(&table_name); - - // Build row data for WHERE evaluation - let mut row_data = HashMap::new(); - row_data.insert("oid".to_string(), index_oid.to_string()); - row_data.insert("relname".to_string(), index_name.to_string()); - row_data.insert("relnamespace".to_string(), "2200".to_string()); - row_data.insert("reltype".to_string(), "0".to_string()); - row_data.insert("reloftype".to_string(), "0".to_string()); - row_data.insert("relowner".to_string(), "10".to_string()); - row_data.insert("relam".to_string(), "403".to_string()); - row_data.insert("relfilenode".to_string(), index_oid.to_string()); - row_data.insert("reltablespace".to_string(), "0".to_string()); - row_data.insert("relpages".to_string(), "0".to_string()); - row_data.insert("reltuples".to_string(), "0".to_string()); - row_data.insert("relallvisible".to_string(), "0".to_string()); - row_data.insert("reltoastrelid".to_string(), "0".to_string()); - row_data.insert("relhasindex".to_string(), "f".to_string()); - row_data.insert("relisshared".to_string(), "f".to_string()); - row_data.insert("relpersistence".to_string(), "p".to_string()); - row_data.insert("relkind".to_string(), "i".to_string()); - row_data.insert("relnatts".to_string(), "0".to_string()); - row_data.insert("relchecks".to_string(), "0".to_string()); - row_data.insert("relhasrules".to_string(), "f".to_string()); - row_data.insert("relhastriggers".to_string(), "f".to_string()); - row_data.insert("relhassubclass".to_string(), "f".to_string()); - row_data.insert("relrowsecurity".to_string(), "f".to_string()); - row_data.insert("relforcerowsecurity".to_string(), "f".to_string()); - row_data.insert("relispopulated".to_string(), "t".to_string()); - row_data.insert("relreplident".to_string(), "n".to_string()); - row_data.insert("relispartition".to_string(), "f".to_string()); - row_data.insert("relrewrite".to_string(), "0".to_string()); - row_data.insert("relfrozenxid".to_string(), "0".to_string()); - row_data.insert("relminmxid".to_string(), "0".to_string()); - row_data.insert("relacl".to_string(), "".to_string()); - row_data.insert("reloptions".to_string(), "".to_string()); - row_data.insert("relpartbound".to_string(), "".to_string()); - - // Evaluate WHERE clause if present - let include_row = if let Some(selection) = &select.selection { - let result = WhereEvaluator::evaluate(selection, &row_data, &column_mapping); - debug!("WHERE evaluation for table '{}': {} (selection: {:?})", table_name, result, selection); - result - } else { - true - }; - - if include_row { - // Build full row with all columns (33 total) - let full_row = vec![ - Some(index_oid.to_string().into_bytes()), // oid - Some(index_name.to_string().into_bytes()), // relname - Some("2200".to_string().into_bytes()), // relnamespace (public schema) - Some("0".to_string().into_bytes()), // reltype (0 for indexes) - Some("0".to_string().into_bytes()), // reloftype - Some("10".to_string().into_bytes()), // relowner (postgres user) - Some("403".to_string().into_bytes()), // relam (btree) - Some(index_oid.to_string().into_bytes()), // relfilenode - Some("0".to_string().into_bytes()), // reltablespace - Some("0".to_string().into_bytes()), // relpages - Some("0".to_string().into_bytes()), // reltuples - Some("0".to_string().into_bytes()), // relallvisible - Some("0".to_string().into_bytes()), // reltoastrelid - Some(b"f".to_vec()), // relhasindex - Some(b"f".to_vec()), // relisshared - Some(b"p".to_vec()), // relpersistence (permanent) - Some(b"i".to_vec()), // relkind (index) - Some("0".to_string().into_bytes()), // relnatts - Some("0".to_string().into_bytes()), // relchecks - Some(b"f".to_vec()), // relhasrules - Some(b"f".to_vec()), // relhastriggers - Some(b"f".to_vec()), // relhassubclass - Some(b"f".to_vec()), // relrowsecurity - Some(b"f".to_vec()), // relforcerowsecurity - Some(b"t".to_vec()), // relispopulated - Some(b"n".to_vec()), // relreplident (nothing) - Some(b"f".to_vec()), // relispartition - Some("0".to_string().into_bytes()), // relrewrite - Some("0".to_string().into_bytes()), // relfrozenxid - Some("0".to_string().into_bytes()), // relminmxid - None, // relacl (NULL) - None, // reloptions (NULL) - None, // relpartbound (NULL) - ]; - - // Project only the requested columns - let projected_row: Vec>> = column_indices.iter() - .map(|&idx| full_row[idx].clone()) - .collect(); - - rows.push(projected_row); - } - } - } - - let rows_affected = rows.len(); - - Ok(DbResponse { - columns, - rows, - rows_affected, - }) - } - - /// Determine which columns to return based on the SELECT projection - fn get_projected_columns(select: &Select, all_columns: &[String]) -> (Vec, Vec) { - let mut columns = Vec::new(); - let mut column_indices = Vec::new(); - - - // Check if it's SELECT * - let is_select_star = select.projection.len() == 1 && matches!(&select.projection[0], SelectItem::Wildcard(_)); - - if is_select_star { - // Return all columns - columns = all_columns.to_vec(); - column_indices = (0..all_columns.len()).collect(); - } else { - // Process each projection item - for item in &select.projection { - match item { - SelectItem::UnnamedExpr(expr) => { - if let Some(col_name) = Self::extract_column_name(expr) { - // Find the index of this column - if let Some(idx) = all_columns.iter().position(|c| c == &col_name) { - columns.push(col_name); - column_indices.push(idx); - } - } - } - SelectItem::ExprWithAlias { expr, alias } => { - if let Some(col_name) = Self::extract_column_name(expr) { - // Find the index of this column - if let Some(idx) = all_columns.iter().position(|c| c == &col_name) { - columns.push(alias.value.clone()); - column_indices.push(idx); - } - } - } - SelectItem::QualifiedWildcard(_, _) => { - // For table.*, return all columns - columns = all_columns.to_vec(); - column_indices = (0..all_columns.len()).collect(); - break; - } - SelectItem::Wildcard(_) => { - // SELECT * - return all columns - columns = all_columns.to_vec(); - column_indices = (0..all_columns.len()).collect(); - break; - } - } - } - } - - (columns, column_indices) - } - - /// Extract column name from an expression - fn extract_column_name(expr: &Expr) -> Option { - match expr { - Expr::Identifier(ident) => Some(ident.value.to_lowercase()), - Expr::CompoundIdentifier(parts) => { - // For table.column, return just the column name - parts.last().map(|ident| ident.value.to_lowercase()) - } - Expr::Cast { expr, .. } => { - // Handle CAST expressions like CAST(oid AS TEXT) - Self::extract_column_name(expr) - } - _ => None, - } - } -} - -fn generate_oid_from_name(name: &str) -> u32 { - // Generate a stable OID from name using a simple hash - // Start at 16384 to avoid conflicts with system OIDs - let mut hash = 0u32; - for byte in name.bytes() { - hash = hash.wrapping_mul(31).wrapping_add(byte as u32); - } - 16384 + (hash % 1000000) -} \ No newline at end of file diff --git a/src/catalog/pg_constraint.rs b/src/catalog/pg_constraint.rs index 31923196..bd2b319d 100644 --- a/src/catalog/pg_constraint.rs +++ b/src/catalog/pg_constraint.rs @@ -254,14 +254,7 @@ impl PgConstraintHandler { } fn generate_table_oid(table_name: &str) -> u32 { - // Generate deterministic OID from table name (same as pg_class handler) - use std::collections::hash_map::DefaultHasher; - use std::hash::{Hash, Hasher}; - - let mut hasher = DefaultHasher::new(); - table_name.hash(&mut hasher); - let hash = hasher.finish(); - 16384 + ((hash % 65536) as u32) // Keep OIDs in reasonable range + crate::utils::generate_table_oid(table_name) } fn constraint_to_row(constraint: &ConstraintInfo) -> Vec>> { diff --git a/src/catalog/pg_sequence.rs b/src/catalog/pg_sequence.rs index 3a410b3a..4c0cb6f1 100644 --- a/src/catalog/pg_sequence.rs +++ b/src/catalog/pg_sequence.rs @@ -147,11 +147,10 @@ impl PgSequenceHandler { } fn generate_table_oid(table_name: &str) -> u32 { - let mut hash = 0u32; - for byte in table_name.bytes() { - hash = hash.wrapping_mul(31).wrapping_add(byte as u32); - } - 16384 + (hash % 65536) + // Must match the formula used by the pg_class view (migration v28) and + // constraint_populator::generate_table_oid, so that seqrelid-adjacent joins to + // pg_class.oid resolve correctly. + crate::utils::generate_table_oid(table_name) } fn apply_where_filter( diff --git a/src/catalog/pg_trigger.rs b/src/catalog/pg_trigger.rs index 30e5e785..3d71f3c5 100644 --- a/src/catalog/pg_trigger.rs +++ b/src/catalog/pg_trigger.rs @@ -223,19 +223,13 @@ impl PgTriggerHandler { } fn generate_trigger_oid(trigger_name: &str) -> u32 { - let mut hash = 0u32; - for byte in trigger_name.bytes() { - hash = hash.wrapping_mul(31).wrapping_add(byte as u32); - } - 16384 + (hash % 65536) + crate::utils::generate_table_oid(trigger_name) } fn generate_table_oid(table_name: &str) -> u32 { - let mut hash = 0u32; - for byte in table_name.bytes() { - hash = hash.wrapping_mul(31).wrapping_add(byte as u32); - } - 16384 + (hash % 65536) + // Must match the formula used by the pg_class view (migration v28) and + // constraint_populator::generate_table_oid, so that tgrelid joins to pg_class.oid. + crate::utils::generate_table_oid(table_name) } fn apply_where_filter( diff --git a/src/catalog/query_interceptor.rs b/src/catalog/query_interceptor.rs index b38d4dce..c095ed6f 100644 --- a/src/catalog/query_interceptor.rs +++ b/src/catalog/query_interceptor.rs @@ -9,7 +9,7 @@ use sqlparser::dialect::PostgreSqlDialect; use sqlparser::parser::Parser; use sqlparser::tokenizer::{Location, Span}; use tracing::{debug, info}; -use super::{pg_class::PgClassHandler, pg_attribute::PgAttributeHandler, pg_constraint::PgConstraintHandler, pg_depend::PgDependHandler, pg_enum::PgEnumHandler, pg_description::PgDescriptionHandler, pg_roles::PgRolesHandler, pg_user::PgUserHandler, pg_stats::PgStatsHandler, pg_sequence::PgSequenceHandler, pg_trigger::PgTriggerHandler, pg_settings::PgSettingsHandler, system_functions::SystemFunctions, where_evaluator::WhereEvaluator}; +use super::{pg_attribute::PgAttributeHandler, pg_constraint::PgConstraintHandler, pg_depend::PgDependHandler, pg_enum::PgEnumHandler, pg_description::PgDescriptionHandler, pg_roles::PgRolesHandler, pg_user::PgUserHandler, pg_stats::PgStatsHandler, pg_sequence::PgSequenceHandler, pg_trigger::PgTriggerHandler, pg_settings::PgSettingsHandler, system_functions::SystemFunctions, where_evaluator::WhereEvaluator}; use std::sync::Arc; use std::pin::Pin; use std::future::Future; @@ -24,10 +24,10 @@ pub struct CatalogInterceptor; impl CatalogInterceptor { /// Check if a query is targeting pg_catalog and handle it pub async fn intercept_query(query: &str, db: Arc, session: Option>) -> Option> { - println!("INTERCEPT_QUERY: {}", query); + debug!("INTERCEPT_QUERY: {}", query); // Quick check to avoid parsing if not a catalog query let lower_query = query.to_lowercase(); - println!("INTERCEPT: lower_query = {}", lower_query); + debug!("INTERCEPT: lower_query = {}", lower_query); // Check for cache status query if lower_query.contains("select * from pgsqlite_cache_status") { @@ -73,47 +73,47 @@ impl CatalogInterceptor { lower_query.contains("pg_get_userbyid") || lower_query.contains("pg_get_indexdef") || lower_query.contains("pg_size_pretty"); - println!("INTERCEPT: has_catalog_tables = {}, has_system_functions = {}", has_catalog_tables, has_system_functions); + debug!("INTERCEPT: has_catalog_tables = {}, has_system_functions = {}", has_catalog_tables, has_system_functions); if !has_catalog_tables && !has_system_functions { - println!("INTERCEPT: Returning None (no catalog tables or system functions)"); + debug!("INTERCEPT: Returning None (no catalog tables or system functions)"); return None; } debug!("Intercepting catalog query: {}", query); - println!("INTERCEPT: After debug, about to check LIMIT 0"); + debug!("INTERCEPT: After debug, about to check LIMIT 0"); // Special handling for LIMIT 0 queries used for metadata if query.contains("LIMIT 0") { - println!("INTERCEPT: Found LIMIT 0, returning None"); + debug!("INTERCEPT: Found LIMIT 0, returning None"); // Skipping LIMIT 0 catalog query return None; } - println!("INTERCEPT: No LIMIT 0, continuing"); + debug!("INTERCEPT: No LIMIT 0, continuing"); // First, remove schema prefixes from catalog tables - println!("INTERCEPT: About to call SchemaPrefixTranslator"); + debug!("INTERCEPT: About to call SchemaPrefixTranslator"); let schema_translated = SchemaPrefixTranslator::translate_query(query); - println!("INTERCEPT: schema_translated = '{}'", schema_translated); + debug!("INTERCEPT: schema_translated = '{}'", schema_translated); // Then, try to translate regex operators if present - println!("INTERCEPT: About to call RegexTranslator"); + debug!("INTERCEPT: About to call RegexTranslator"); let query_to_parse = match RegexTranslator::translate_query(&schema_translated) { Ok(translated) => { if translated != query { - println!("INTERCEPT: RegexTranslator changed query to: '{}'", translated); + debug!("INTERCEPT: RegexTranslator changed query to: '{}'", translated); } else { - println!("INTERCEPT: RegexTranslator made no changes"); + debug!("INTERCEPT: RegexTranslator made no changes"); } translated } Err(e) => { - println!("INTERCEPT: RegexTranslator failed: {:?}", e); + debug!("INTERCEPT: RegexTranslator failed: {:?}", e); // Failed to translate regex operators query.to_string() } }; - println!("INTERCEPT: query_to_parse = '{}'", query_to_parse); + debug!("INTERCEPT: query_to_parse = '{}'", query_to_parse); // Parse the query (keep JSON path placeholders for now) let dialect = PostgreSqlDialect {}; @@ -185,12 +185,12 @@ impl CatalogInterceptor { } // Normal catalog table handling - println!("INTERCEPT: About to call handle_catalog_query"); + debug!("INTERCEPT: About to call handle_catalog_query"); if let Some(response) = Self::handle_catalog_query(query_stmt, db.clone(), session.clone()).await { - println!("INTERCEPT: handle_catalog_query returned Some(response), columns: {}, rows: {}", response.columns.len(), response.rows.len()); + debug!("INTERCEPT: handle_catalog_query returned Some(response), columns: {}, rows: {}", response.columns.len(), response.rows.len()); return Some(Ok(response)); } - println!("INTERCEPT: handle_catalog_query returned None"); + debug!("INTERCEPT: handle_catalog_query returned None"); } // If we translated the query but it's not a special catalog query, @@ -208,12 +208,25 @@ impl CatalogInterceptor { None } + /// Catalog tables that exist as real SQLite views/tables (populated by migrations and + /// constraint_populator) and can therefore be queried with plain SQL once we return `None` + /// to let the query fall through. `pg_stats` and `pg_tablespace` are deliberately excluded: + /// per migrations v19 and v24 (src/migration/registry.rs), both are handled entirely by the + /// catalog interceptor and have no backing SQLite view/table, so routing a JOIN into them to + /// raw SQL would fail with "no such table". + fn is_sqlite_backed_catalog(name: &str) -> bool { + name.contains("pg_constraint") || name.contains("pg_index") || + name.contains("pg_depend") || name.contains("pg_proc") || + name.contains("pg_description") || name.contains("pg_roles") || + name.contains("pg_user") + } + async fn handle_catalog_query(query: &sqlparser::ast::Query, db: Arc, session: Option>) -> Option { debug!("handle_catalog_query called"); - println!("HANDLE_CATALOG_QUERY: called with query"); + debug!("HANDLE_CATALOG_QUERY: called with query"); // Check if this is a SELECT from pg_catalog tables if let SetExpr::Select(select) = &*query.body { - println!("HANDLE_CATALOG_QUERY: Is SELECT query, from.len()={}, has_joins={}", + debug!("HANDLE_CATALOG_QUERY: Is SELECT query, from.len()={}, has_joins={}", select.from.len(), !select.from.is_empty() && !select.from[0].joins.is_empty()); debug!("Is SELECT query, from.len()={}, has_joins={}", @@ -222,30 +235,30 @@ impl CatalogInterceptor { // Check if this is a JOIN query involving catalog tables if !select.from.is_empty() && !select.from[0].joins.is_empty() { debug!("Detected as JOIN query"); - println!("HANDLE_CATALOG_QUERY: Detected as JOIN query"); + debug!("HANDLE_CATALOG_QUERY: Detected as JOIN query"); // Check if this is a JOIN between information_schema tables if let TableFactor::Table { name: main_table, .. } = &select.from[0].relation { let main_table_name = main_table.to_string().to_lowercase(); - println!("HANDLE_CATALOG_QUERY: Main table name: '{}'", main_table_name); + debug!("HANDLE_CATALOG_QUERY: Main table name: '{}'", main_table_name); // Check if main table and all JOINs are information_schema tables let is_information_schema_join = main_table_name.contains("information_schema") && select.from[0].joins.iter().all(|j| { if let TableFactor::Table { name: join_table, .. } = &j.relation { let join_table_name = join_table.to_string().to_lowercase(); - println!("HANDLE_CATALOG_QUERY: Join table name: '{}'", join_table_name); + debug!("HANDLE_CATALOG_QUERY: Join table name: '{}'", join_table_name); join_table_name.contains("information_schema") } else { false } }); - println!("HANDLE_CATALOG_QUERY: is_information_schema_join = {}", is_information_schema_join); + debug!("HANDLE_CATALOG_QUERY: is_information_schema_join = {}", is_information_schema_join); if is_information_schema_join { debug!("Detected information_schema JOIN query - translating and executing"); - println!("HANDLE_CATALOG_QUERY: Detected information_schema JOIN query"); + debug!("HANDLE_CATALOG_QUERY: Detected information_schema JOIN query"); // Information_schema tables exist as views with underscores, not dots // e.g., information_schema_table_constraints instead of information_schema.table_constraints @@ -262,7 +275,7 @@ impl CatalogInterceptor { query_str = query_str.replace("information_schema.tables", "information_schema_tables"); query_str = query_str.replace("information_schema.schemata", "information_schema_schemata"); - println!("HANDLE_CATALOG_QUERY: Translated query: {}", query_str); + debug!("HANDLE_CATALOG_QUERY: Translated query: {}", query_str); match db.connection_manager().execute_with_session(&session_id, |conn| { debug!("Executing translated information_schema JOIN query: {}", query_str); @@ -310,13 +323,13 @@ impl CatalogInterceptor { Ok(response) => { debug!("Successfully executed translated JOIN query, returning {} rows with {} columns", response.rows_affected, response.columns.len()); - println!("HANDLE_CATALOG_QUERY: Successfully executed translated JOIN, {} rows, {} columns", + debug!("HANDLE_CATALOG_QUERY: Successfully executed translated JOIN, {} rows, {} columns", response.rows_affected, response.columns.len()); return Some(response); } Err(e) => { debug!("Failed to execute translated JOIN: {}", e); - println!("HANDLE_CATALOG_QUERY: Failed to execute translated JOIN: {}", e); + debug!("HANDLE_CATALOG_QUERY: Failed to execute translated JOIN: {}", e); // Fall through to try other methods } } @@ -459,12 +472,28 @@ impl CatalogInterceptor { } } + // pg_class is now served by a real SQLite view (see pg_class migration). + // JOINs from pg_class into other SQLite-backed catalog tables (pg_constraint, + // pg_index, pg_depend, ...) must be executed as real SQL so the join predicate + // is actually evaluated, instead of being routed to a single-table handler that + // can't see columns from the other side of the join. pg_stats and pg_tablespace + // are excluded — see is_sqlite_backed_catalog. + let has_view_backed_catalog_joins = select.from[0].joins.iter().any(|j| { + if let TableFactor::Table { name, .. } = &j.relation { + let join_table = name.to_string().to_lowercase(); + Self::is_sqlite_backed_catalog(&join_table) + } else { + false + } + }); + + if table_name.contains("pg_class") && has_view_backed_catalog_joins { + debug!("Passing pg_class JOIN against SQLite-backed catalog table to SQLite views"); + return None; + } + // For other catalog table JOINs, still return None to let SQLite handle - if table_name.contains("pg_") && (table_name.contains("pg_constraint") || - table_name.contains("pg_index") || table_name.contains("pg_depend") || - table_name.contains("pg_proc") || table_name.contains("pg_description") || - table_name.contains("pg_roles") || table_name.contains("pg_user") || - table_name.contains("pg_stats") || table_name.contains("pg_tablespace")) { + if table_name.contains("pg_") && Self::is_sqlite_backed_catalog(&table_name) { debug!("Passing other catalog JOIN query to SQLite views"); return None; } @@ -504,7 +533,7 @@ impl CatalogInterceptor { async fn check_table_factor(table_factor: &TableFactor, select: &Select, db: Arc, session: Option>) -> Option> { if let TableFactor::Table { name, .. } = table_factor { let table_name = name.to_string().to_lowercase(); - println!("CHECK_TABLE_FACTOR: Processing table name: '{}'", table_name); + debug!("CHECK_TABLE_FACTOR: Processing table name: '{}'", table_name); debug!("check_table_factor: Processing table name: {}", table_name); // Handle pg_type queries @@ -512,11 +541,6 @@ impl CatalogInterceptor { return Some(Ok(Self::handle_pg_type_query(select, db.clone(), session.clone()).await)); } - // Handle pg_namespace queries - if table_name.contains("pg_namespace") || table_name.contains("pg_catalog.pg_namespace") { - return Some(Ok(Self::handle_pg_namespace_query(select))); - } - // Handle pg_range queries (usually empty) if table_name.contains("pg_range") || table_name.contains("pg_catalog.pg_range") { return Some(Ok(Self::handle_pg_range_query(select))); @@ -547,11 +571,6 @@ impl CatalogInterceptor { return Some(Ok(Self::handle_pg_statistic_query(select))); } - // Handle pg_class queries - if table_name.contains("pg_class") || table_name.contains("pg_catalog.pg_class") { - return Some(PgClassHandler::handle_query(select, &db).await); - } - // Handle pg_attribute queries if table_name.contains("pg_attribute") || table_name.contains("pg_catalog.pg_attribute") { info!("Routing to PgAttributeHandler for table: {}", table_name); @@ -770,7 +789,7 @@ impl CatalogInterceptor { // Note: pg_index is a SQLite view that will be executed normally // It doesn't need special interception since it exists in the database } - println!("INTERCEPT: Reached end of intercept_query, returning None"); + debug!("INTERCEPT: Reached end of intercept_query, returning None"); None } @@ -1060,43 +1079,6 @@ impl CatalogInterceptor { } } - fn handle_pg_namespace_query(select: &Select) -> DbResponse { - let all_columns = vec!["oid".to_string(), "nspname".to_string()]; - let (columns, column_indices) = Self::extract_selected_columns(select, &all_columns); - - let full_rows = vec![ - vec![ - Some("11".to_string().into_bytes()), - Some("pg_catalog".to_string().into_bytes()), - ], - vec![ - Some("2200".to_string().into_bytes()), - Some("public".to_string().into_bytes()), - ], - ]; - if columns.is_empty() && Self::is_count_star_projection(&select.projection) { - return Self::count_response(full_rows.len()); - } - - let rows: Vec>>> = full_rows - .into_iter() - .map(|full_row| { - column_indices - .iter() - .map(|&idx| full_row[idx].clone()) - .collect() - }) - .collect(); - - let rows_affected = rows.len(); - debug!("Returning {} rows for pg_namespace query with {} columns: {:?}", rows_affected, columns.len(), columns); - DbResponse { - columns, - rows, - rows_affected, - } - } - fn handle_pg_range_query(_select: &Select) -> DbResponse { // pg_range is typically empty for basic types let columns = vec!["rngtypid".to_string(), "rngsubtype".to_string()]; @@ -1808,44 +1790,6 @@ impl CatalogInterceptor { .map(|ident| ident.value.to_lowercase()) } - fn is_count_star_projection(projection: &[SelectItem]) -> bool { - if projection.len() != 1 { - return false; - } - - let expr = match &projection[0] { - SelectItem::UnnamedExpr(expr) => expr, - SelectItem::ExprWithAlias { expr, .. } => expr, - _ => return false, - }; - - let Expr::Function(function) = expr else { - return false; - }; - - if Self::function_name(function).as_deref() != Some("count") { - return false; - } - - matches!( - &function.args, - sqlparser::ast::FunctionArguments::List(arg_list) - if arg_list.args.len() == 1 - && matches!( - &arg_list.args[0], - FunctionArg::Unnamed(FunctionArgExpr::Wildcard) - ) - ) - } - - fn count_response(count: usize) -> DbResponse { - DbResponse { - columns: vec!["count".to_string()], - rows: vec![vec![Some(count.to_string().into_bytes())]], - rows_affected: 1, - } - } - async fn handle_information_schema_schemata_query(select: &Select, _db: &DbHandler) -> DbResponse { debug!("Handling information_schema.schemata query"); diff --git a/src/config.rs b/src/config.rs index b24af98d..c9b1d866 100644 --- a/src/config.rs +++ b/src/config.rs @@ -164,7 +164,30 @@ pub struct Config { pub migrate: bool, } +/// SQLite database name used for `--in-memory` mode. +pub const IN_MEMORY_DB_NAME: &str = "pgsqlite_mem"; + +/// Build the URI for an in-memory SQLite database. +/// +/// A bare `:memory:` gives every connection its *own* private database, so migrations +/// run on one connection are invisible to the connections opened later for each client +/// session. The shared-cache URI form makes every connection in the process address one +/// database. This lives here rather than inline at the call site so tests can pin the +/// format instead of duplicating the literal. +pub fn in_memory_db_uri(name: &str) -> String { + format!("file:{name}?mode=memory&cache=shared") +} + impl Config { + /// Resolve the database path this configuration should open. + pub fn resolve_db_path(&self) -> String { + if self.in_memory { + in_memory_db_uri(IN_MEMORY_DB_NAME) + } else { + self.database.clone() + } + } + /// Get a configuration instance with all values resolved from CLI args and environment variables pub fn load() -> Self { let config = Config::parse(); diff --git a/src/main.rs b/src/main.rs index 6da4a955..3b4ebc5c 100644 --- a/src/main.rs +++ b/src/main.rs @@ -39,12 +39,10 @@ async fn main() -> Result<()> { info!("pgsqlite v{}", env!("CARGO_PKG_VERSION")); // Determine database path based on --in-memory flag - let db_path = if config.in_memory { + if config.in_memory { info!("Using in-memory SQLite database (testing mode)"); - ":memory:".to_string() - } else { - config.database.clone() - }; + } + let db_path = config.resolve_db_path(); // Handle migration command if config.migrate { @@ -53,7 +51,13 @@ async fn main() -> Result<()> { // Open connection directly for migration let conn = rusqlite::Connection::open(&db_path) .map_err(|e| anyhow::anyhow!("Failed to open database: {}", e))?; - + + // pragma_table_info() is a virtual table; views (e.g. pg_class.relnatts) that call it + // require trusted_schema=ON. It defaults to ON in the C API, but pin it explicitly + // so a future default change or defensive-mode setting can't silently break those views. + conn.execute_batch("PRAGMA trusted_schema=ON;") + .map_err(|e| anyhow::anyhow!("Failed to set trusted_schema pragma: {}", e))?; + // Register functions needed for migrations pgsqlite::functions::register_all_functions(&conn) .map_err(|e| anyhow::anyhow!("Failed to register functions: {}", e))?; diff --git a/src/metadata/object_resolver.rs b/src/metadata/object_resolver.rs index 4e0a7372..70dca7b6 100644 --- a/src/metadata/object_resolver.rs +++ b/src/metadata/object_resolver.rs @@ -8,7 +8,10 @@ use tracing::debug; pub struct ObjectResolver; impl ObjectResolver { - /// Resolve table name to OID using the same algorithm as pg_class view + /// Resolve table name to OID for comment storage (`__pgsqlite_comments.object_oid`). + /// NOTE: this uses a `DefaultHasher`-based formula, not the pg_class view's unicode + /// formula (see `crate::utils::generate_table_oid`), so these OIDs do not match + /// `pg_class.oid`. Changing this requires a data migration for already-persisted rows. pub fn resolve_table_oid(table_name: &str) -> i32 { generate_table_oid(table_name) } @@ -80,7 +83,10 @@ impl ObjectResolver { } } -/// Generate a stable OID from table name using the same algorithm as pg_class view +/// Generate a stable OID from a table name using a `DefaultHasher`. This is NOT the same +/// formula as the pg_class view (`crate::utils::generate_table_oid`); these OIDs do not +/// join against `pg_class.oid`. Kept as-is because `__pgsqlite_comments.object_oid` rows +/// already persist values from this formula (see issue #87 follow-up for a data migration). fn generate_table_oid(name: &str) -> i32 { let mut hasher = DefaultHasher::new(); name.hash(&mut hasher); diff --git a/src/migration/registry.rs b/src/migration/registry.rs index cde093f2..9f40b424 100644 --- a/src/migration/registry.rs +++ b/src/migration/registry.rs @@ -34,6 +34,7 @@ lazy_static! { register_v25_information_schema_triggers_support(&mut registry); register_v26_enhanced_pg_attribute_support(&mut registry); register_v27_fix_pg_proc_types(&mut registry); + register_v28_pg_class_full_columns(&mut registry); registry }; @@ -2859,4 +2860,173 @@ fn register_v27_fix_pg_proc_types(registry: &mut BTreeMap) { ])), dependencies: vec![26], }); -} \ No newline at end of file +} + +/// Version 28: Serve pg_class from SQLite with full column parity. +/// Adds the nine columns the Rust pg_class handler used to synthesize, assigns internal +/// pg_*/information_schema_* relations to their proper namespaces, and fixes +/// three pre-existing view bugs (relkind_full is not a real PostgreSQL column; +/// relreplident should be 'd'; relispartition should be 'f'). +fn register_v28_pg_class_full_columns(registry: &mut BTreeMap) { + registry.insert(28, Migration { + version: 28, + name: "pg_class_full_columns", + description: "Enrich pg_class view to full 33-column parity and namespace internal relations so SQLite can serve pg_class directly", + up: MigrationAction::SqlBatch(&[ + r#"DROP VIEW IF EXISTS pg_class"#, + r#"DROP VIEW IF EXISTS pg_namespace"#, + + r#" + CREATE VIEW pg_namespace AS + SELECT 11 as oid, 'pg_catalog' as nspname, 10 as nspowner, NULL as nspacl + UNION ALL + SELECT 2200 as oid, 'public' as nspname, 10 as nspowner, NULL as nspacl + UNION ALL + SELECT 13000 as oid, 'information_schema' as nspname, 10 as nspowner, NULL as nspacl + "#, + + r#" + CREATE VIEW pg_class AS + WITH base AS ( + SELECT name, type, + ((unicode(substr(name, 1, 1)) * 1000000) + + (unicode(substr(name || ' ', 2, 1)) * 10000) + + (unicode(substr(name || ' ', 3, 1)) * 100) + + (length(name) * 7)) % 1000000 + 16384 AS oid_num + FROM sqlite_master + WHERE type IN ('table', 'view', 'index') + AND name NOT LIKE 'sqlite_%' + AND name NOT LIKE '__pgsqlite_%' + ) + SELECT + CAST(oid_num AS TEXT) as oid, + name as relname, + CASE + WHEN name LIKE 'pg\_%' ESCAPE '\' THEN 11 + WHEN name LIKE 'information\_schema\_%' ESCAPE '\' THEN 13000 + ELSE 2200 + END as relnamespace, + CAST(oid_num + 1 AS TEXT) as reltype, + 0 as reloftype, + 10 as relowner, + CASE WHEN type = 'index' THEN 403 ELSE 0 END as relam, + 0 as relfilenode, + 0 as reltablespace, + 0 as relpages, + -1 as reltuples, + 0 as relallvisible, + 0 as reltoastrelid, + CASE WHEN type = 'table' THEN 't' ELSE 'f' END as relhasindex, + 'f' as relisshared, + 'p' as relpersistence, + CASE type + WHEN 'table' THEN 'r' + WHEN 'view' THEN 'v' + WHEN 'index' THEN 'i' + END as relkind, + (SELECT COUNT(*) FROM pragma_table_info(base.name)) as relnatts, + 0 as relchecks, + 'f' as relhasrules, + CASE WHEN EXISTS( + SELECT 1 FROM sqlite_master t + WHERE t.type = 'trigger' AND t.tbl_name = base.name + ) THEN 't' ELSE 'f' END as relhastriggers, + 'f' as relhassubclass, + 'f' as relrowsecurity, + 'f' as relforcerowsecurity, + 't' as relispopulated, + 'd' as relreplident, + 'f' as relispartition, + 0 as relrewrite, + 0 as relfrozenxid, + 0 as relminmxid, + NULL as relacl, + NULL as reloptions, + NULL as relpartbound + FROM base + "#, + + r#" + UPDATE __pgsqlite_metadata + SET value = '28', updated_at = strftime('%s', 'now') + WHERE key = 'schema_version'; + "#, + ]), + down: Some(MigrationAction::SqlBatch(&[ + r#"DROP VIEW IF EXISTS pg_class"#, + r#"DROP VIEW IF EXISTS pg_namespace"#, + + // Restore the v26 pg_class view: copied verbatim from + // register_v26_enhanced_pg_attribute_support's `up`. + r#" + CREATE VIEW IF NOT EXISTS pg_class AS + SELECT + -- Use SQLite built-in functions for consistent OID generation + CAST( + ( + (unicode(substr(name, 1, 1)) * 1000000) + + (unicode(substr(name || ' ', 2, 1)) * 10000) + + (unicode(substr(name || ' ', 3, 1)) * 100) + + (length(name) * 7) + ) % 1000000 + 16384 + AS TEXT) as oid, + name as relname, + 2200 as relnamespace, -- public schema + CASE + WHEN type = 'table' THEN 'r' + WHEN type = 'view' THEN 'v' + WHEN type = 'index' THEN 'i' + END as relkind, + 10 as relowner, + CASE WHEN type = 'index' THEN 403 ELSE 0 END as relam, + 0 as relfilenode, + 0 as reltablespace, + 0 as relpages, + -1 as reltuples, + 0 as relallvisible, + 0 as reltoastrelid, + CASE WHEN type = 'table' THEN 't' ELSE 'f' END as relhasindex, + 'f' as relisshared, + 'p' as relpersistence, + 'h' as relkind_full, + 't' as relispopulated, + 'v' as relreplident, + 't' as relispartition, + 0 as relrewrite, + 0 as relfrozenxid, + 0 as relminmxid, + NULL as relacl, + NULL as reloptions, + NULL as relpartbound + FROM sqlite_master + WHERE type IN ('table', 'view', 'index') + AND name NOT LIKE 'sqlite_%' + AND name NOT LIKE '__pgsqlite_%'; + "#, + + // Restore the two-row pg_namespace view: copied verbatim from + // register_v5_pg_catalog_tables's `up`. + r#" + CREATE VIEW IF NOT EXISTS pg_namespace AS + SELECT + 11 as oid, + 'pg_catalog' as nspname, + 10 as nspowner, + NULL as nspacl + UNION ALL + SELECT + 2200 as oid, + 'public' as nspname, + 10 as nspowner, + NULL as nspacl; + "#, + + r#" + UPDATE __pgsqlite_metadata + SET value = '27', updated_at = strftime('%s', 'now') + WHERE key = 'schema_version'; + "#, + ])), + dependencies: vec![27], + }); +} diff --git a/src/session/connection_manager.rs b/src/session/connection_manager.rs index 7c52920d..d31f8add 100644 --- a/src/session/connection_manager.rs +++ b/src/session/connection_manager.rs @@ -69,7 +69,13 @@ impl ConnectionManager { ); conn.execute_batch(&pragma_sql) .map_err(PgSqliteError::Sqlite)?; - + + // pragma_table_info() is a virtual table; views (e.g. pg_class.relnatts) that call it + // require trusted_schema=ON. It defaults to ON in the C API, but pin it explicitly + // so a future default change or defensive-mode setting can't silently break those views. + conn.execute_batch("PRAGMA trusted_schema=ON;") + .map_err(PgSqliteError::Sqlite)?; + // Register functions crate::functions::register_all_functions(&conn) .map_err(PgSqliteError::Sqlite)?; diff --git a/src/session/db_handler.rs b/src/session/db_handler.rs index 7f6afd8f..bf3f2c73 100644 --- a/src/session/db_handler.rs +++ b/src/session/db_handler.rs @@ -57,6 +57,12 @@ pub struct DbHandler { statement_cache_optimizer: Arc, sql_injection_detector: Arc, pub(crate) db_path: String, + /// Keeps a shared-cache in-memory SQLite database alive for the lifetime of this + /// handler. SQLite destroys a shared-cache `:memory:` database as soon as its last + /// connection closes, so without this the migrated schema (including the pg_class / + /// pg_namespace views) would vanish the moment the initial migration connection is + /// dropped, before any session connection is opened. + _memory_keepalive: Option>>, } impl DbHandler { @@ -285,22 +291,33 @@ impl DbHandler { // Create a temporary connection for migrations let temp_conn = Self::create_initial_connection(db_path, config)?; - + // Run migrations if needed - Self::run_migrations_if_needed(temp_conn, db_path)?; - + let temp_conn = Self::run_migrations_if_needed(temp_conn, db_path)?; + // Initialize optimization components let optimization_manager = Arc::new(OptimizationManager::new(true)); let statement_cache_optimizer = Arc::new(StatementCacheOptimizer::new(200, optimization_manager)); - + // Create connection manager let connection_manager = Arc::new(ConnectionManager::new( db_path.to_string(), Arc::new(config.clone()) )); - + + // Shared-cache in-memory databases are destroyed as soon as their last connection + // closes. Keep the migration connection alive for the lifetime of this DbHandler so + // the migrated schema survives until the first real session connection is created. + let is_memory_db = db_path == ":memory:" || db_path.contains("mode=memory"); + let memory_keepalive = if is_memory_db { + Some(Arc::new(parking_lot::Mutex::new(temp_conn))) + } else { + drop(temp_conn); + None + }; + // DbHandler initialized - + Ok(Self { connection_manager, schema_cache: Arc::new(SchemaCache::new(config.schema_cache_ttl)), @@ -308,6 +325,7 @@ impl DbHandler { statement_cache_optimizer, sql_injection_detector: Arc::new(SqlInjectionDetector::new()), db_path: db_path.to_string(), + _memory_keepalive: memory_keepalive, }) } @@ -340,18 +358,23 @@ impl DbHandler { config.pragma_mmap_size ); conn.execute_batch(&pragma_sql)?; - + + // pragma_table_info() is a virtual table; views (e.g. pg_class.relnatts) that call it + // require trusted_schema=ON. It defaults to ON in the C API, but pin it explicitly + // so a future default change or defensive-mode setting can't silently break those views. + conn.execute_batch("PRAGMA trusted_schema=ON;")?; + Ok(conn) } - fn run_migrations_if_needed(conn: rusqlite::Connection, db_path: &str) -> Result<(), rusqlite::Error> { + fn run_migrations_if_needed(conn: rusqlite::Connection, db_path: &str) -> Result { // Skip all checks for in-memory databases - if db_path.contains(":memory:") { + if db_path.contains(":memory:") || db_path.contains("mode=memory") { debug!("Running initial migrations for in-memory database..."); - + // Register functions before migrations crate::functions::register_all_functions(&conn)?; - + let mut runner = MigrationRunner::new(conn); match runner.run_pending_migrations() { Ok(applied) => { @@ -366,7 +389,7 @@ impl DbHandler { )); } } - return Ok(()); + return Ok(runner.into_connection()); } // For file-based databases, first check for schema drift @@ -405,10 +428,10 @@ impl DbHandler { if needs_migrations { debug!("Running initial migrations..."); - + // Register functions before migrations crate::functions::register_all_functions(&conn)?; - + let mut runner = MigrationRunner::new(conn); match runner.run_pending_migrations() { Ok(applied) => { @@ -423,16 +446,18 @@ impl DbHandler { )); } } + Ok(runner.into_connection()) } else { // Check if we need to run any pending migrations // Register functions first crate::functions::register_all_functions(&conn)?; - + let runner = MigrationRunner::new(conn); match runner.check_schema_version() { Ok(()) => { // Schema is up to date debug!("Schema version check passed"); + Ok(runner.into_connection()) } Err(e) => { // Schema is outdated, run migrations @@ -451,11 +476,10 @@ impl DbHandler { )); } } + Ok(runner.into_connection()) } } } - - Ok(()) } /// Create a connection for a new session @@ -1274,7 +1298,7 @@ impl DbHandler { /// Query with session-specific connection pub async fn query_with_session(&self, query: &str, session_id: &Uuid) -> Result { - eprintln!("🔍 query_with_session called with query: {}", query); + debug!("query_with_session called with query: {}", query); // Check if this is a catalog query that should be intercepted // We need to do this before applying translations let lower_query = query.to_lowercase(); diff --git a/src/utils/mod.rs b/src/utils/mod.rs index d1f0c272..a1de435f 100644 --- a/src/utils/mod.rs +++ b/src/utils/mod.rs @@ -1,3 +1,3 @@ pub mod oid_generator; -pub use oid_generator::{generate_oid, generate_oid_i32, generate_oid_string}; \ No newline at end of file +pub use oid_generator::{generate_oid, generate_oid_i32, generate_oid_string, generate_table_oid}; \ No newline at end of file diff --git a/src/utils/oid_generator.rs b/src/utils/oid_generator.rs index 521cff24..bcf5676f 100644 --- a/src/utils/oid_generator.rs +++ b/src/utils/oid_generator.rs @@ -1,8 +1,9 @@ /// Central OID generation module to ensure consistency across the codebase -/// Uses the same formula as the pg_class view in migrations -/// Generate a stable OID from a name using the same formula as SQLite views -/// This matches: (unicode(substr(name, 1, 1)) * 1000000) + (unicode(substr(name || ' ', 2, 1)) * 10000) + ... +/// Generate a stable OID from a name by sampling six character positions. +/// +/// This is the *constraint/sequence* OID formula. It is deliberately NOT the table-identity +/// formula — see `generate_table_oid` for the one that must match the pg_class view. pub fn generate_oid(name: &str) -> u32 { // For better uniqueness, sample characters from different positions let chars: Vec = name.chars().collect(); @@ -10,18 +11,22 @@ pub fn generate_oid(name: &str) -> u32 { // Sample characters from different positions for better distribution // Use first, middle, and last characters to avoid collisions - let char1 = chars.get(0).copied().unwrap_or(' ') as u32; - let char2 = chars.get(1).copied().unwrap_or(' ') as u32; - let char3 = chars.get(len / 3).copied().unwrap_or(' ') as u32; // 1/3 position - let char4 = chars.get(2 * len / 3).copied().unwrap_or(' ') as u32; // 2/3 position - let char5 = chars.get(len.saturating_sub(1)).copied().unwrap_or(' ') as u32; // Last char - let char6 = chars.get(len / 2).copied().unwrap_or(' ') as u32; // Middle char - let length = name.len() as u32; + // Widened to u64 for the same reason as generate_table_oid: a high-codepoint + // character times 1_000_000 overflows u32. The final `% 1_000_000 + 16384` + // keeps the result far below u32::MAX, so the round trip is safe, and every + // value that did not previously overflow is unchanged. + let char1 = chars.first().copied().unwrap_or(' ') as u64; + let char2 = chars.get(1).copied().unwrap_or(' ') as u64; + let char3 = chars.get(len / 3).copied().unwrap_or(' ') as u64; // 1/3 position + let char4 = chars.get(2 * len / 3).copied().unwrap_or(' ') as u64; // 2/3 position + let char5 = chars.get(len.saturating_sub(1)).copied().unwrap_or(' ') as u64; // Last char + let char6 = chars.get(len / 2).copied().unwrap_or(' ') as u64; // Middle char + let length = name.len() as u64; // Include characters from different positions for better uniqueness // This helps distinguish constraints with the same prefix - ((char1 * 1000000) + (char2 * 10000) + (char3 * 100) + - (char4 * 37) + (char5 * 23) + (char6 * 19) + (length * 7)) % 1000000 + 16384 + (((char1 * 1000000) + (char2 * 10000) + (char3 * 100) + + (char4 * 37) + (char5 * 23) + (char6 * 19) + (length * 7)) % 1000000 + 16384) as u32 } /// Generate OID as i32 (for functions that need signed integers) @@ -34,6 +39,26 @@ pub fn generate_oid_string(name: &str) -> String { generate_oid(name).to_string() } +/// Generate a stable table-identity OID from a name using the 3-character-prefix formula. +/// This is the canonical formula used by the pg_class/pg_namespace views (migration v28) and +/// by `constraint_populator::generate_table_oid`. Every producer of table-identity OIDs +/// (attrelid, tgrelid, seqrelid, etc.) must use this exact formula so that joins against +/// pg_class.oid resolve correctly. +pub fn generate_table_oid(name: &str) -> u32 { + let name_with_padding = format!("{name} "); + let chars: Vec = name_with_padding.chars().collect(); + // Widen to u64 for the arithmetic: a high-codepoint leading character (e.g. from + // "日本語") times 1_000_000 overflows u32. The final `% 1_000_000 + 16_384` keeps + // the result far below u32::MAX, so the u32 -> u64 -> u32 round trip is safe. + let char1 = chars.first().copied().unwrap_or(' ') as u64; + let char2 = chars.get(1).copied().unwrap_or(' ') as u64; + let char3 = chars.get(2).copied().unwrap_or(' ') as u64; + // Match SQLite's `length(name)`, which counts characters, not UTF-8 bytes. + let length = name.chars().count() as u64; + + (((char1 * 1_000_000) + (char2 * 10_000) + (char3 * 100) + (length * 7)) % 1_000_000 + 16384) as u32 +} + #[cfg(test)] mod tests { use super::*; @@ -50,6 +75,30 @@ mod tests { assert_ne!(oid1, oid3); } + /// The u64 widening of `generate_oid` must be strictly panic-eliminating: every + /// input that already produced a value must still produce the same value. These + /// were measured against the pre-widening body (with checked arithmetic to detect + /// the overflow) and are unchanged by the widening, non-ASCII names included. + #[test] + fn test_generate_oid_values_unchanged_by_widening() { + assert_eq!(generate_oid("users"), 186701); + assert_eq!(generate_oid("customers"), 206538); + assert_eq!(generate_oid("orders"), 175208); + assert_eq!(generate_oid("a"), 353754); + assert_eq!(generate_oid("ab"), 1013840); + assert_eq!(generate_oid("café"), 1007190); + } + + /// A high-codepoint leading character overflowed `char1 * 1_000_000` in u32, + /// panicking in debug builds and silently wrapping in release. `generate_oid` + /// reaches persisted catalog OIDs through migration v5's `populate_catalog_tables`, + /// so this was a panic on the upgrade path for such a name. + #[test] + fn test_generate_oid_high_codepoint_no_panic() { + assert_eq!(generate_oid("日本語"), 408635); + assert_eq!(generate_oid("\u{10FFFF}x"), 636999); + } + #[test] fn test_oid_formats() { let name = "users"; @@ -60,4 +109,33 @@ mod tests { assert_eq!(oid_u32 as i32, oid_i32); assert_eq!(oid_u32.to_string(), oid_string); } + + /// Pinned ASCII value: `customers` is persisted on disk today in + /// `pg_constraint.conrelid` / `pg_index.indrelid` / `pg_attrdef.adrelid` / + /// `pg_depend.objid` and `refobjid`, and `tests/catalog_join_test.rs:47` pins it too. + /// This value must never change for ASCII input. + #[test] + fn test_generate_table_oid_ascii_pinned() { + assert_eq!(generate_table_oid("customers"), 197947); + assert_eq!(generate_table_oid("orders"), 166426); + } + + /// Non-ASCII names must agree with the v28 SQL expression, which uses + /// `length(name)` (character count). These expected values were computed by + /// running the identical SQL expression through the `sqlite3` CLI: + /// `café` -> 996612, `naïve_tbl` -> 1010347. + #[test] + fn test_generate_table_oid_non_ascii_matches_sqlite() { + assert_eq!(generate_table_oid("café"), 996612); + assert_eq!(generate_table_oid("naïve_tbl"), 1010347); + } + + /// A high-codepoint leading character must not overflow/panic. Before the u64 + /// widening, `generate_table_oid("日本語")` panicked with "attempt to multiply + /// with overflow" in a debug build. Expected value computed via the `sqlite3` + /// CLI running the identical v28 SQL expression: 685005. + #[test] + fn test_generate_table_oid_high_codepoint_no_panic() { + assert_eq!(generate_table_oid("日本語"), 685005); + } } \ No newline at end of file diff --git a/tests/catalog_alias_test.rs b/tests/catalog_alias_test.rs index 1047d2c5..984c3ed2 100644 --- a/tests/catalog_alias_test.rs +++ b/tests/catalog_alias_test.rs @@ -36,35 +36,43 @@ async fn test_pg_catalog_roles_alias_projection_uses_source_column() { #[tokio::test] async fn test_pg_catalog_namespace_alias_projection() { - let response = catalog_query("SELECT oid AS did FROM pg_catalog.pg_namespace").await; + // pg_namespace is now served directly from SQLite (migration v28) instead of being + // intercepted in Rust, so this alias-projection behavior is exercised against pg_roles + // instead, which remains a Rust-intercepted catalog. + let response = catalog_query("SELECT oid AS did FROM pg_catalog.pg_roles").await; assert_eq!(response.columns, vec!["did"]); - assert_eq!(response.rows.len(), 2); - assert_eq!(text_cell(&response.rows[0], 0), "11"); - assert_eq!(text_cell(&response.rows[1], 0), "2200"); + assert_eq!(response.rows.len(), 3); + assert_eq!(text_cell(&response.rows[0], 0), "10"); + assert_eq!(text_cell(&response.rows[1], 0), "0"); + assert_eq!(text_cell(&response.rows[2], 0), "100"); } #[tokio::test] async fn test_catalog_unquoted_aliases_fold_to_lowercase_and_quoted_aliases_preserve_case() { let response = catalog_query( - "SELECT oid AS MixedAlias, nspname AS \"SchemaName\" FROM pg_catalog.pg_namespace", + "SELECT oid AS MixedAlias, rolname AS \"SchemaName\" FROM pg_catalog.pg_roles", ) .await; assert_eq!(response.columns, vec!["mixedalias", "SchemaName"]); - assert_eq!(response.rows.len(), 2); - assert_eq!(text_cell(&response.rows[0], 0), "11"); - assert_eq!(text_cell(&response.rows[0], 1), "pg_catalog"); + assert_eq!(response.rows.len(), 3); + assert_eq!(text_cell(&response.rows[0], 0), "10"); + assert_eq!(text_cell(&response.rows[0], 1), "postgres"); } #[tokio::test] async fn test_catalog_wildcard_keeps_trailing_projection_items() { - let namespace = catalog_query("SELECT *, oid FROM pg_catalog.pg_namespace").await; + // pg_namespace is now served directly from SQLite (migration v28), so the wildcard half + // of this test is exercised against pg_database instead, which remains Rust-intercepted. + let database = catalog_query("SELECT *, oid FROM pg_catalog.pg_database").await; - assert_eq!(namespace.columns, vec!["oid", "nspname", "oid"]); - assert_eq!(namespace.rows.len(), 2); - assert_eq!(text_cell(&namespace.rows[0], 0), "11"); - assert_eq!(text_cell(&namespace.rows[0], 2), "11"); + assert_eq!(database.columns.len(), 19); + assert_eq!(database.columns[0], "oid"); + assert_eq!(database.columns[18], "oid"); + assert_eq!(database.rows.len(), 1); + assert_eq!(text_cell(&database.rows[0], 0), "1"); + assert_eq!(text_cell(&database.rows[0], 18), "1"); let roles = catalog_query("SELECT *, oid FROM pg_catalog.pg_roles").await; @@ -78,11 +86,14 @@ async fn test_catalog_wildcard_keeps_trailing_projection_items() { #[tokio::test] async fn test_catalog_count_star_returns_static_dataset_count() { - let namespace = catalog_query("SELECT count(*) FROM pg_catalog.pg_namespace").await; + // pg_namespace is now served directly from SQLite (migration v28), so the unfiltered + // count(*) half of this test is exercised against pg_roles instead (still Rust-intercepted), + // using a distinct query from the filtered pg_roles count below. + let roles_all = catalog_query("SELECT count(*) FROM pg_catalog.pg_roles").await; - assert_eq!(namespace.columns, vec!["count"]); - assert_eq!(namespace.rows.len(), 1); - assert_eq!(text_cell(&namespace.rows[0], 0), "2"); + assert_eq!(roles_all.columns, vec!["count"]); + assert_eq!(roles_all.rows.len(), 1); + assert_eq!(text_cell(&roles_all.rows[0], 0), "3"); let roles = catalog_query("SELECT count(*) FROM pg_catalog.pg_roles WHERE rolcanlogin = 't'").await; @@ -106,32 +117,41 @@ async fn test_pg_roles_unquoted_aliases_fold_to_lowercase_and_quoted_aliases_pre } #[tokio::test] -async fn test_pg_namespace_cast_projection_uses_inner_source_column() { - let response = catalog_query("SELECT CAST(oid AS text) AS o FROM pg_catalog.pg_namespace").await; +async fn test_pg_roles_cast_projection_uses_inner_source_column() { + // pg_namespace is now served directly from SQLite (migration v28), so cast-projection + // behavior is exercised against pg_roles instead, which remains Rust-intercepted. + let response = catalog_query("SELECT CAST(oid AS text) AS o FROM pg_catalog.pg_roles").await; assert_eq!(response.columns, vec!["o"]); - assert_eq!(response.rows.len(), 2); - assert_eq!(text_cell(&response.rows[0], 0), "11"); - assert_eq!(text_cell(&response.rows[1], 0), "2200"); + assert_eq!(response.rows.len(), 3); + assert_eq!(text_cell(&response.rows[0], 0), "10"); + assert_eq!(text_cell(&response.rows[1], 0), "0"); + assert_eq!(text_cell(&response.rows[2], 0), "100"); } #[tokio::test] -async fn test_pg_namespace_nested_projection_uses_inner_source_column() { - let response = catalog_query("SELECT (oid) AS o FROM pg_catalog.pg_namespace").await; +async fn test_pg_roles_nested_projection_uses_inner_source_column() { + // pg_namespace is now served directly from SQLite (migration v28), so nested-projection + // behavior is exercised against pg_roles instead, which remains Rust-intercepted. + let response = catalog_query("SELECT (oid) AS o FROM pg_catalog.pg_roles").await; assert_eq!(response.columns, vec!["o"]); - assert_eq!(response.rows.len(), 2); - assert_eq!(response.rows[0][0].as_deref(), Some(b"11".as_ref())); + assert_eq!(response.rows.len(), 3); + assert_eq!(response.rows[0][0].as_deref(), Some(b"10".as_ref())); } #[tokio::test] -async fn test_pg_namespace_compound_identifier_projection_uses_leaf_column() { - let response = catalog_query("SELECT n.nspname FROM pg_catalog.pg_namespace AS n").await; +async fn test_pg_roles_compound_identifier_projection_uses_leaf_column() { + // pg_namespace is now served directly from SQLite (migration v28), so + // compound-identifier projection is exercised against pg_roles instead, which remains + // Rust-intercepted. + let response = catalog_query("SELECT r.rolname FROM pg_catalog.pg_roles AS r").await; - assert_eq!(response.columns, vec!["nspname"]); - assert_eq!(response.rows.len(), 2); - assert_eq!(text_cell(&response.rows[0], 0), "pg_catalog"); + assert_eq!(response.columns, vec!["rolname"]); + assert_eq!(response.rows.len(), 3); + assert_eq!(text_cell(&response.rows[0], 0), "postgres"); assert_eq!(text_cell(&response.rows[1], 0), "public"); + assert_eq!(text_cell(&response.rows[2], 0), "pgsqlite_user"); } #[tokio::test] @@ -144,11 +164,14 @@ async fn test_pg_roles_where_filter_with_alias_free_projection() { } #[tokio::test] -async fn test_pg_namespace_unknown_source_column_projects_no_columns() { - let response = catalog_query("SELECT nonexistent FROM pg_catalog.pg_namespace").await; +async fn test_pg_roles_unknown_source_column_projects_no_columns() { + // pg_namespace is now served directly from SQLite (migration v28), so unknown-column + // projection behavior is exercised against pg_roles instead, which remains + // Rust-intercepted. + let response = catalog_query("SELECT nonexistent FROM pg_catalog.pg_roles").await; assert!(response.columns.is_empty()); - assert_eq!(response.rows.len(), 2); + assert_eq!(response.rows.len(), 3); assert!(response.rows.iter().all(Vec::is_empty)); } diff --git a/tests/catalog_join_test.rs b/tests/catalog_join_test.rs new file mode 100644 index 00000000..753f1009 --- /dev/null +++ b/tests/catalog_join_test.rs @@ -0,0 +1,54 @@ +mod common; +use common::setup_test_server_with_init; + +#[tokio::test] +async fn test_pg_class_joins_pg_constraint_on_oid() { + let _ = env_logger::builder().is_test(true).try_init(); + + let server = setup_test_server_with_init(|db| { + Box::pin(async move { + db.execute("CREATE TABLE customers (id INTEGER PRIMARY KEY, name TEXT)").await?; + Ok(()) + }) + }).await; + + let rows = server.client.query( + "SELECT c.relname, con.conname \ + FROM pg_catalog.pg_class c \ + JOIN pg_catalog.pg_constraint con ON con.conrelid = c.oid \ + WHERE c.relname = 'customers'", + &[] + ).await.expect("cross-catalog join should succeed"); + + assert!(!rows.is_empty(), + "pg_class.oid must match the persisted pg_constraint.conrelid"); + + for row in &rows { + let relname: String = row.get(0); + assert_eq!(relname, "customers"); + let _conname: String = row.get(1); + } +} + +#[tokio::test] +async fn test_pg_class_oid_matches_persisted_formula() { + let _ = env_logger::builder().is_test(true).try_init(); + + let server = setup_test_server_with_init(|db| { + Box::pin(async move { + db.execute("CREATE TABLE customers (id INTEGER PRIMARY KEY, name TEXT)").await?; + Ok(()) + }) + }).await; + + let rows = server.client.query( + "SELECT relname FROM pg_catalog.pg_class WHERE relname = 'customers' AND oid = '197947'", + &[] + ).await.expect("query should succeed"); + + // unicode formula for 'customers': + // c=99, u=117, s=115, len=9 + // (99*1000000 + 117*10000 + 115*100 + 63) % 1000000 + 16384 = 197947 + assert_eq!(rows.len(), 1, + "pg_class must use the canonical unicode OID formula that constraint_populator persists"); +} diff --git a/tests/catalog_regex_operators_test.rs b/tests/catalog_regex_operators_test.rs new file mode 100644 index 00000000..b493cd20 --- /dev/null +++ b/tests/catalog_regex_operators_test.rs @@ -0,0 +1,62 @@ +mod common; +use common::setup_test_server_with_init; + +async fn server_with_two_tables() -> common::TestServer { + setup_test_server_with_init(|db| { + Box::pin(async move { + db.execute("CREATE TABLE customers (id INTEGER PRIMARY KEY)").await?; + db.execute("CREATE TABLE orders (id INTEGER PRIMARY KEY)").await?; + Ok(()) + }) + }).await +} + +#[tokio::test] +async fn test_regex_match_actually_filters() { + let _ = env_logger::builder().is_test(true).try_init(); + let server = server_with_two_tables().await; + + let rows = server.client.query( + "SELECT relname FROM pg_catalog.pg_class WHERE relname ~ '^cust'", &[] + ).await.expect("~ query should succeed"); + + let names: Vec = rows.iter().map(|r| r.get::<_, String>(0)).collect(); + + assert!(names.iter().any(|n| n == "customers"), "~ must match customers, got {names:?}"); + assert!(!names.iter().any(|n| n == "orders"), + "~ must actually filter -- 'orders' does not match '^cust', got {names:?}"); +} + +#[tokio::test] +async fn test_regex_not_match_does_not_exclude_everything() { + let _ = env_logger::builder().is_test(true).try_init(); + let server = server_with_two_tables().await; + + let rows = server.client.query( + "SELECT relname FROM pg_catalog.pg_class WHERE relname !~ '^pg_toast'", &[] + ).await.expect("!~ query should succeed"); + + let names: Vec = rows.iter().map(|r| r.get::<_, String>(0)).collect(); + + assert!(names.iter().any(|n| n == "customers"), + "issue #87: !~ must not exclude non-matching rows, got {names:?}"); + assert!(names.iter().any(|n| n == "orders"), + "!~ must not exclude non-matching rows, got {names:?}"); +} + +#[tokio::test] +async fn test_regex_not_match_still_excludes_matches() { + let _ = env_logger::builder().is_test(true).try_init(); + let server = server_with_two_tables().await; + + let rows = server.client.query( + "SELECT relname FROM pg_catalog.pg_class WHERE relname !~ '^cust'", &[] + ).await.expect("!~ query should succeed"); + + let names: Vec = rows.iter().map(|r| r.get::<_, String>(0)).collect(); + + assert!(!names.iter().any(|n| n == "customers"), + "!~ '^cust' must exclude customers, got {names:?}"); + assert!(names.iter().any(|n| n == "orders"), + "!~ '^cust' must keep orders, got {names:?}"); +} diff --git a/tests/catalog_where_simple_test.rs b/tests/catalog_where_simple_test.rs index 9c0c27be..999530a4 100644 --- a/tests/catalog_where_simple_test.rs +++ b/tests/catalog_where_simple_test.rs @@ -14,7 +14,12 @@ async fn test_catalog_where_simple() { eprintln!("Test server listening on port {port}"); let server_handle = tokio::spawn(async move { - let db_handler = Arc::new(DbHandler::new(":memory:").unwrap()); + // Use the same shared-cache in-memory URI as the `--in-memory` server flag + // (src/main.rs). A bare ":memory:" gives every SQLite connection its own private, + // empty database, so the pg_class/pg_namespace views created by migrations on the + // initial connection would be invisible to the per-session connections opened by + // ConnectionManager. + let db_handler = Arc::new(DbHandler::new("file:pgsqlite_mem_where_simple?mode=memory&cache=shared").unwrap()); // Create test table db_handler.execute("CREATE TABLE test_table1 (id INTEGER PRIMARY KEY, name TEXT)").await.unwrap(); diff --git a/tests/in_memory_pg_class_regression_test.rs b/tests/in_memory_pg_class_regression_test.rs new file mode 100644 index 00000000..ecbf2234 --- /dev/null +++ b/tests/in_memory_pg_class_regression_test.rs @@ -0,0 +1,83 @@ +use clap::Parser; +use pgsqlite::config::Config; +use pgsqlite::session::db_handler::DbHandler; +use std::sync::Arc; +use uuid::Uuid; + +fn text_cell(row: &[Option>], index: usize) -> String { + String::from_utf8(row[index].clone().expect("cell should not be NULL")).unwrap() +} + +/// Regression test for the `--in-memory` fix in `src/main.rs` (issue #87 fix round). +/// +/// `src/main.rs` opens `file:pgsqlite_mem?mode=memory&cache=shared` (not bare +/// `:memory:`) so that migrations run on one SQLite connection stay visible to the +/// *other* SQLite connections opened for each client session. `DbHandler` keeps a +/// `_memory_keepalive` connection alive for exactly this reason +/// (`src/session/db_handler.rs`): a shared-cache memory database is destroyed the +/// moment its last connection closes, so without a keepalive connection, or without +/// the shared-cache URI, a second session's connection would open its own private, +/// empty in-memory database and never see the migrated `pg_class`/`pg_namespace` +/// views or any table created by another session. +/// +/// This test opens a `DbHandler` on the URI that `--in-memory` actually resolves to +/// -- obtained from `Config::resolve_db_path()`, the same call `src/main.rs` makes, so +/// reverting that decision to a bare `:memory:` breaks this test rather than leaving it +/// green. It creates a table on one (temporary, memory-mode) connection via +/// `DbHandler::execute`, then queries `pg_class` from a second, independently created +/// session connection and asserts the table shows up. Before the shared-cache fix this +/// hard-errored with "no such table: pg_class" because the second connection's database +/// was empty. +#[tokio::test] +async fn test_in_memory_pg_class_visible_across_sessions() { + // Exactly what `pgsqlite --in-memory` resolves its database path to. + let db_path = Config::parse_from(["pgsqlite", "--in-memory"]).resolve_db_path(); + assert_ne!( + db_path, ":memory:", + "--in-memory must not resolve to a bare :memory:; that gives every connection \ + its own private database and hides the migrated catalog views from sessions" + ); + + let db_handler = Arc::new( + DbHandler::new(&db_path) + .expect("DbHandler::new should succeed on the shared-cache in-memory URI"), + ); + + // Create a user table. For memory databases, DbHandler::execute opens its own + // temporary session connection, runs the statement, and tears the connection + // down again -- so by the time this returns, no connection this test controls + // is the one that created the table. + db_handler + .execute("CREATE TABLE regression_users (id INTEGER PRIMARY KEY, name TEXT)") + .await + .expect("CREATE TABLE should succeed"); + + // Open a brand-new session connection, independent of the one used above, and + // query pg_class through it -- this is the exact scenario a second psql/ORM + // connection to `pgsqlite --in-memory` exercises. + let session_id = Uuid::new_v4(); + db_handler + .create_session_connection(session_id) + .await + .expect("creating a second session connection should succeed"); + + let response = db_handler + .query_with_session( + "SELECT relname FROM pg_class WHERE relname = 'regression_users'", + &session_id, + ) + .await + .expect( + "querying pg_class from a second session must not hard-error; if this fails, \ + the shared-cache in-memory URI regressed back to per-connection private databases", + ); + + assert_eq!( + response.rows.len(), + 1, + "expected the table created on another session to be visible via pg_class" + ); + assert_eq!(text_cell(&response.rows[0], 0), "regression_users"); + + db_handler.remove_session_connection(&session_id); +} diff --git a/tests/migration_test.rs b/tests/migration_test.rs index bb069232..25b2239c 100644 --- a/tests/migration_test.rs +++ b/tests/migration_test.rs @@ -23,8 +23,8 @@ fn test_fresh_database_migration() { // Should apply all migrations assert_eq!(applied.len(), MIGRATIONS.len()); - assert_eq!(applied, vec![1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15, 16, 17, 18, 19, 20, 21, 22, 23, 24, 25, 26, 27]); - + assert_eq!(applied, vec![1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15, 16, 17, 18, 19, 20, 21, 22, 23, 24, 25, 26, 27, 28]); + // Verify schema version let conn = runner.into_connection(); let version: String = conn.query_row( @@ -32,12 +32,12 @@ fn test_fresh_database_migration() { [], |row| row.get(0) ).unwrap(); - assert_eq!(version, "27"); - + assert_eq!(version, "28"); + // Now check should pass let runner2 = MigrationRunner::new(conn); assert!(runner2.check_schema_version().is_ok()); - + // Verify all tables exist let conn = runner2.into_connection(); let tables: Vec = conn.prepare( @@ -68,7 +68,7 @@ fn test_idempotent_migrations() { let conn = Connection::open(&db_path).unwrap(); let mut runner = MigrationRunner::new(conn); let applied = runner.run_pending_migrations().unwrap(); - assert_eq!(applied.len(), 27); + assert_eq!(applied.len(), 28); drop(runner); // Second run - should apply nothing @@ -109,8 +109,8 @@ fn test_existing_schema_detection() { let mut runner = MigrationRunner::new(conn); let applied = runner.run_pending_migrations().unwrap(); - // Should recognize existing schema as version 1 and only apply versions 2-27 - assert_eq!(applied.len(), 26); + // Should recognize existing schema as version 1 and only apply versions 2-28 + assert_eq!(applied.len(), 27); assert_eq!(applied[0], 2); assert_eq!(applied[1], 3); assert_eq!(applied[2], 4); @@ -123,7 +123,8 @@ fn test_existing_schema_detection() { assert_eq!(applied[9], 11); assert_eq!(applied[10], 12); assert_eq!(applied[25], 27); - + assert_eq!(applied[26], 28); + // Verify final version let conn = runner.into_connection(); let version: String = conn.query_row( @@ -131,7 +132,7 @@ fn test_existing_schema_detection() { [], |row| row.get(0) ).unwrap(); - assert_eq!(version, "27"); + assert_eq!(version, "28"); // Now check should pass let runner2 = MigrationRunner::new(conn); @@ -156,7 +157,7 @@ fn test_migration_history() { .unwrap() .collect::, _>>().unwrap(); - assert_eq!(migrations.len(), 27); + assert_eq!(migrations.len(), 28); assert_eq!(migrations[0], (1, "initial_schema".to_string(), "completed".to_string())); assert_eq!(migrations[1], (2, "enum_type_support".to_string(), "completed".to_string())); assert_eq!(migrations[2], (3, "datetime_timezone_support".to_string(), "completed".to_string())); diff --git a/tests/pg_class_dt_test.rs b/tests/pg_class_dt_test.rs new file mode 100644 index 00000000..6b1f9b78 --- /dev/null +++ b/tests/pg_class_dt_test.rs @@ -0,0 +1,118 @@ +mod common; +use common::setup_test_server_with_init; + +/// The exact query psql 18 expands `\dt` into. +const DT_QUERY: &str = r#" +SELECT n.nspname as "Schema", + c.relname as "Name", + CASE c.relkind WHEN 'r' THEN 'table' WHEN 'v' THEN 'view' WHEN 'm' THEN 'materialized view' WHEN 'i' THEN 'index' WHEN 'S' THEN 'sequence' WHEN 't' THEN 'TOAST table' WHEN 'f' THEN 'foreign table' WHEN 'p' THEN 'partitioned table' WHEN 'I' THEN 'partitioned index' END as "Type", + pg_catalog.pg_get_userbyid(c.relowner) as "Owner" +FROM pg_catalog.pg_class c + LEFT JOIN pg_catalog.pg_namespace n ON n.oid = c.relnamespace + LEFT JOIN pg_catalog.pg_am am ON am.oid = c.relam +WHERE c.relkind IN ('r','p','') + AND n.nspname <> 'pg_catalog' + AND n.nspname !~ '^pg_toast' + AND n.nspname <> 'information_schema' + AND pg_catalog.pg_table_is_visible(c.oid) +ORDER BY 1,2 +"#; + +#[tokio::test] +async fn test_dt_lists_user_tables() { + let _ = env_logger::builder().is_test(true).try_init(); + + let server = setup_test_server_with_init(|db| { + Box::pin(async move { + db.execute("CREATE TABLE customers (id INTEGER PRIMARY KEY, name TEXT)").await?; + Ok(()) + }) + }).await; + + let rows = server.client.query(DT_QUERY, &[]).await + .expect("\\dt query should succeed"); + + let names: Vec = rows.iter().map(|r| r.get::<_, String>("Name")).collect(); + + assert!(names.iter().any(|n| n == "customers"), + "issue #87: \\dt must list the user table, got {names:?}"); +} + +#[tokio::test] +async fn test_dt_hides_internal_relations() { + let _ = env_logger::builder().is_test(true).try_init(); + + let server = setup_test_server_with_init(|db| { + Box::pin(async move { + db.execute("CREATE TABLE customers (id INTEGER PRIMARY KEY, name TEXT)").await?; + Ok(()) + }) + }).await; + + let rows = server.client.query(DT_QUERY, &[]).await + .expect("\\dt query should succeed"); + + let names: Vec = rows.iter().map(|r| r.get::<_, String>("Name")).collect(); + + for internal in ["pg_constraint", "pg_attrdef", "pg_index", "pg_depend"] { + assert!(!names.iter().any(|n| n == internal), + "pgsqlite's own {internal} must not appear in \\dt, got {names:?}"); + } + + assert!(!names.is_empty(), + "must not pass vacuously against a 0-row result"); +} + +#[tokio::test] +async fn test_dt_reports_public_schema_and_owner() { + let _ = env_logger::builder().is_test(true).try_init(); + + let server = setup_test_server_with_init(|db| { + Box::pin(async move { + db.execute("CREATE TABLE customers (id INTEGER PRIMARY KEY, name TEXT)").await?; + Ok(()) + }) + }).await; + + let rows = server.client.query(DT_QUERY, &[]).await + .expect("\\dt query should succeed"); + + let row = rows.iter() + .find(|r| r.get::<_, String>("Name") == "customers") + .expect("customers row must be present"); + + assert_eq!(row.get::<_, String>("Schema"), "public"); + assert_eq!(row.get::<_, String>("Type"), "table"); +} + +#[tokio::test] +async fn test_trusted_schema_allows_pragma_in_views() { + let _ = env_logger::builder().is_test(true).try_init(); + + let server = setup_test_server_with_init(|db| { + Box::pin(async move { + db.execute("CREATE TABLE customers (id INTEGER PRIMARY KEY, name TEXT)").await?; + Ok(()) + }) + }).await; + + // `PRAGMA trusted_schema` does not round-trip through the wire protocol: pgsqlite's + // query-vs-execute classifier routes it to the execute path, which rejects it because + // it returns rows ("Execute returned results - did you mean to call query?"). It's also + // not a strong enough probe on its own: verified manually with the sqlite3 CLI, a bare + // top-level `SELECT ... FROM pragma_table_info(...)` succeeds regardless of trusted_schema + // -- SQLite only enforces the restriction on virtual tables referenced from within a VIEW + // (or trigger/check constraint) definition. That's exactly what Task 3's pg_class.relnatts + // column does, so assert the real guarantee: a VIEW that calls pragma_table_info() must be + // creatable and queryable. + server.client.execute( + "CREATE VIEW v_trusted_schema_probe AS \ + SELECT (SELECT COUNT(*) FROM pragma_table_info('customers')) AS n", + &[], + ).await.expect("CREATE VIEW using pragma_table_info() must succeed when trusted_schema is ON"); + + let rows = server.client.query("SELECT n FROM v_trusted_schema_probe", &[]).await + .expect("SELECT from a view that calls pragma_table_info() must succeed"); + let n: i32 = rows[0].get(0); + assert_eq!(n, 2, "customers has 2 columns"); +} diff --git a/tests/pg_class_view_test.rs b/tests/pg_class_view_test.rs new file mode 100644 index 00000000..5774ea54 --- /dev/null +++ b/tests/pg_class_view_test.rs @@ -0,0 +1,107 @@ +mod common; +use common::setup_test_server_with_init; + +const ALL_PG_CLASS_COLUMNS: &str = "oid, relname, relnamespace, reltype, reloftype, \ +relowner, relam, relfilenode, reltablespace, relpages, reltuples, relallvisible, \ +reltoastrelid, relhasindex, relisshared, relpersistence, relkind, relnatts, relchecks, \ +relhasrules, relhastriggers, relhassubclass, relrowsecurity, relforcerowsecurity, \ +relispopulated, relreplident, relispartition, relrewrite, relfrozenxid, relminmxid, \ +relacl, reloptions, relpartbound"; + +#[tokio::test] +async fn test_pg_class_has_full_column_parity() { + let _ = env_logger::builder().is_test(true).try_init(); + + let server = setup_test_server_with_init(|db| { + Box::pin(async move { + db.execute("CREATE TABLE customers (id INTEGER PRIMARY KEY, name TEXT)").await?; + Ok(()) + }) + }).await; + + let sql = format!("SELECT {ALL_PG_CLASS_COLUMNS} FROM pg_catalog.pg_class WHERE relname = 'customers'"); + let rows = server.client.query(&sql, &[]).await + .expect("all 33 pg_class columns must be selectable"); + + assert_eq!(rows.len(), 1, "expected exactly one row for customers"); +} + +#[tokio::test] +async fn test_pg_class_namespace_assignment() { + let _ = env_logger::builder().is_test(true).try_init(); + + let server = setup_test_server_with_init(|db| { + Box::pin(async move { + db.execute("CREATE TABLE customers (id INTEGER PRIMARY KEY, name TEXT)").await?; + Ok(()) + }) + }).await; + + // Compare in SQL rather than binding integers in Rust: relnamespace is an + // INTEGER in the view, and its inferred wire type is not worth guessing. + let public_rows = server.client.query( + "SELECT relname FROM pg_catalog.pg_class WHERE relnamespace = 2200 AND relname = 'customers'", + &[] + ).await.expect("query should succeed"); + assert_eq!(public_rows.len(), 1, "user tables belong to the public namespace (2200)"); + + let catalog_rows = server.client.query( + "SELECT relname FROM pg_catalog.pg_class WHERE relnamespace = 11 AND relname = 'pg_constraint'", + &[] + ).await.expect("query should succeed"); + assert_eq!(catalog_rows.len(), 1, "internal pg_* relations belong to pg_catalog (11)"); + + let misfiled = server.client.query( + "SELECT relname FROM pg_catalog.pg_class WHERE relnamespace = 2200 AND relname LIKE 'pg\\_%'", + &[] + ).await.expect("query should succeed"); + assert!(misfiled.is_empty(), "no pg_* relation may remain in the public namespace"); +} + +#[tokio::test] +async fn test_pg_class_relnatts_is_real_column_count() { + let _ = env_logger::builder().is_test(true).try_init(); + + let server = setup_test_server_with_init(|db| { + Box::pin(async move { + db.execute("CREATE TABLE three_cols (a INTEGER, b TEXT, c REAL)").await?; + Ok(()) + }) + }).await; + + let rows = server.client.query( + "SELECT relname FROM pg_catalog.pg_class WHERE relname = 'three_cols' AND relnatts = 3", + &[] + ).await.expect("query should succeed"); + + assert_eq!(rows.len(), 1, "relnatts must reflect the real column count (3)"); +} + +#[tokio::test] +async fn test_pg_namespace_has_information_schema() { + let _ = env_logger::builder().is_test(true).try_init(); + + let server = setup_test_server_with_init(|db| { + Box::pin(async move { + db.execute("CREATE TABLE customers (id INTEGER PRIMARY KEY)").await?; + Ok(()) + }) + }).await; + + let rows = server.client.query( + "SELECT nspname FROM pg_catalog.pg_namespace ORDER BY nspname", &[] + ).await.expect("query should succeed"); + + let names: Vec = rows.iter().map(|r| r.get::<_, String>(0)).collect(); + + for expected in ["pg_catalog", "public", "information_schema"] { + assert!(names.iter().any(|n| n == expected), + "v28 must provide the {expected} namespace, got {names:?}"); + } + + // Confirm the oid values via SQL, avoiding an integer wire-type binding. + let is_row = server.client.query( + "SELECT nspname FROM pg_catalog.pg_namespace WHERE oid = 13000", &[] + ).await.expect("query should succeed"); + assert_eq!(is_row.len(), 1, "information_schema must have oid 13000"); +}