launchbadge / sqlx

🧰 The Rust SQL Toolkit. An async, pure Rust SQL crate featuring compile-time checked queries without a DSL. Supports PostgreSQL, MySQL, and SQLite.
Apache License 2.0
13.42k stars 1.28k forks source link

SQLite: Alter table column error #2517

Open wyhaya opened 1 year ago

wyhaya commented 1 year ago

Bug Description

When the column name is adjusted, it is not possible to get the exact column name, it may be old or new

Minimal Reproduction

use sqlx::{query, Column, Pool, Row, Sqlite};

#[tokio::main]
async fn main() {
    let pool = Pool::<Sqlite>::connect("sqlite://./test.db").await.unwrap();
    query(
        r#"
            DROP TABLE user;
            CREATE TABLE "user" (age INTEGER NOT NULL);
            INSERT INTO user (age) VALUES (18);
        "#,
    )
    .execute(&pool)
    .await
    .unwrap();

    let print_column = || async {
        let row = query(r#"SELECT * from user"#)
            .fetch_one(&pool)
            .await
            .unwrap();
        dbg!(row.column(0).name());
    };

    print_column().await;
    // [src/main.rs:22] row.column(0).name() = "age"

    query(r#"ALTER TABLE user RENAME COLUMN age TO new_age"#)
        .execute(&pool)
        .await
        .unwrap();

    for _ in 0..6 {
        print_column().await;
        // [src/main.rs:22] row.column(0).name() = "age"
        // [src/main.rs:22] row.column(0).name() = "new_age"
        // [src/main.rs:22] row.column(0).name() = "age"
        // [src/main.rs:22] row.column(0).name() = "new_age"
        // [src/main.rs:22] row.column(0).name() = "age"
        // [src/main.rs:22] row.column(0).name() = "new_age"
    }
}

I don't know if I'm missing something? It seems like it's always going back and forth between the new and old values.

Info

pymongo commented 1 year ago

https://github.com/launchbadge/sqlx/blob/253d8c9f696a3a2c7aa837b04cc93605a1376694/sqlx-sqlite/src/connection/worker.rs#L135-L141

Reproduced in my Linux machine, sqlx-sqlite statement cache bug, I try your code and fetch PRAGMA schema_version inside print_column, all schema_version is same after alter table

I edit sqlx source match execute::iter(&mut conn, &query, arguments, false) to make persistent always false, and print_column works as expected after alter table

wyhaya commented 1 year ago
CREATE TABLE "user" (age INTEGER NOT NULL, name TEXT);
INSERT INTO "user" (age, name) VALUES (18, 'Alex');

Add column error

query(r#"ALTER TABLE "user" ADD COLUMN "aaaa" INTEGER"#)
        .execute(&pool)
        .await
        .unwrap();
// Error
query(r#"SELECT * from user"#)
        .fetch_one(&pool)
        .await
        .unwrap();
thread 'sqlx-sqlite-worker-0' panicked at 'index out of bounds: the len is 2 but the index is 2', /Users/.../.cargo/registry/src/index.crates.io-6f17d22bba15001f/sqlx-sqlite-0.7.1/src/row.rs:43:39
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
thread 'main' panicked at 'called `Result::unwrap()` on an `Err` value: RowNotFound', src/main.rs:40:10

drop column error

query(r#"ALTER TABLE "user" DROP COLUMN "name""#)
        .execute(&pool)
        .await
        .unwrap();
let row = query(r#"SELECT * from user"#)
        .fetch_one(&pool)
        .await
        .unwrap();
dbg!(row.columns());

I've got age and name, which normally should not contain name.

rm-dr commented 4 months ago

Indeed, something is broken. I'm getting the same index panic when reading the database after adding a column (all in one connection).

sampullman commented 2 months ago

In our apps we always close the pool and open a new one after any ALTER TABLE command to avoid this. We only verified the behavior for renaming a column, but do it after add/drop as well to be safe.

rm-dr commented 2 months ago

I figured out how to fix this a while ago. The root cause is sqlx's statement cache. ALTER TABLE changes the schema, which makes cached statements invalid.

The solution is to call clear_cached_statements after changing the schema. No need to reset the pool (which might not even be possible if your system is complex enough)

Edit: You may also need to create your pool like this.

let pool = SqlitePool::connect_with(
    SqliteConnectOptions::from_str(&db_addr).unwrap()
    // Disable statement cache. Each connection in this pool will have its own statement cache,
    // so the cache-clearing we do via `clear_cached_statements` won't clear all statement caches.
    .statement_cache_capacity(0)
    ...
rm-dr commented 2 months ago

I'd still consider this a bug, though. We need to document this behavior where it's easy to find.

It might also be a good idea to clear cached statements automatically when the schema changes, but that's a decision for someone more deeply involved in sqlx.