Ability to disable the prepared statement cache

[fbecart]

I'm encountering an issue while doing schema changes on Postgres:

cached plan must not change result type

Some examples of cases when this can happen:

• Adding a variant to an enum
• Changing the type of a column (e.g. from vachar(30) to varchar)

It is currently complicated to perform these schema changes without breaking the connections. My understanding is that each connection holds a prepared statement cache. When some schema changes modify the signature of the prepared statements, Postgres refuses to execute previously cached prepared statements.

I've evaluated a few options:

• Implement a more complex migration path over multiple deployments, paying attention never to update any type in the schema (also considering the complexity of testing these constraints, and the impact on shipping speed).
• Stop the impacted services during deployment. However, for some services, high availability is more critical than high performance, and this option is not viable.
• Disable the prepared statement cache. We've successfully experimented with this option in my team (see ignaskar/diesel@ba2f567...c2cb6d3). Ideally, we would be able to opt-out of the prepared statement cache at service startup or at runtime -- so we could disable the cache while we run our migrations.

Are there other options than these?

If this is in line with the vision of the maintainers, we'd like to contribute, by making the prepared statement cache optional (enabled by default).

[fbecart]

An even superior solution would be to replicate what https://github.com/jackc/pgx does: the prepared statement cache is an LRU cache with a configurable size (statement_cache_capacity, with default size 512). By setting it to 0, it is possible to disable the cache.

[weiznich]

Thanks for opening this discussion. For future reference, I'm able to recreate that problem by using the following code:

    #[test]
    fn statement_cache() {
        let conn = &mut crate::test_helpers::pg_connection();

        table! {
            users {
                id -> Integer,
                name -> Text,
            }
        }

        diesel::sql_query("CREATE TABLE users(id SERIAL PRIMARY KEY, name VARCHAR(20) NOT NULL)")
            .execute(conn)
            .unwrap();

        // execute it one to cache the statement
        let r = users::table.load::<(i32, String)>(conn).unwrap();
        assert!(r.is_empty());

        // change the type
        diesel::sql_query("ALTER TABLE users ALTER COLUMN name TYPE TEXT")
            .execute(conn)
            .unwrap();

        // execute it again to break the cache
        let r = users::table.load::<(i32, String)>(conn).unwrap();
        assert!(r.is_empty());
    }

The second query then fails with DatabaseError(Unknown, "cached plan must not change result type").

Now with that written, first let me elaborate additional ways to workaround this specific issue in your application:

• Obviously you know when you execute such a potential schema changing operation, as that could only be done via sql_query, migrations (or a custom query DSL node). You could drop and recreate all connections at that point. This would also drop the cache, but that wouldn't require stopping the service.
• You could have a connection wrapper that just injects a query new type wrapper that unconditionally opts out prepared statement caching for all queries. See the statement cache documentation for details on which traits/methods are relevant for this.

That said these are workarounds that would be applied now, without patching diesel.

I think the general issue is something that we might want to resolve at some point, ideally without affecting performance for those that want to use the prepared statement cache. I'm not sure if that's possible by changing it to a LRU cache as we would then need to somehow clean the cache at some point, which is just additional work.
Another potential way to solve this that came into my mind was that we could try to catch this specific error ("cached plan must not change result type") and use that information to just drop the old cached statement, re-prepare a new one and rerun the query afterwards. This would introduce additional work for cases where this happens, but it would transparently allow applications to not care about this particular kind of issue.

[fbecart]

Thanks a lot @weiznich for your time looking into this issue, writing this code snippet to reproduce it, and contributing to the discussion.

About the workarounds:

You could drop and recreate all connections at that point. This would also drop the cache, but that wouldn't require stopping the service.

We use diesel_migrations binaries to update our schema. Right after running the migrations, we could signal all services to recreate all connections, and this would indeed reduce the likelihood to encounter these errors. This still leaves a gap between the schema update and signal handling, as well as the handling of ongoing transactions.

You could have a connection wrapper that just injects a query new type wrapper that unconditionally opts out prepared statement caching for all queries.

Thanks for pointing this solution and providing a link to the relevant documentation. I will look into this possibility, as this could be a better workaround than the patch we use at the moment.

we could try to catch this specific error ("cached plan must not change result type") and use that information to just drop the old cached statement, re-prepare a new one and rerun the query afterwards. This would introduce additional work for cases where this happens, but it would transparently allow applications to not care about this particular kind of issue.

Yes, this would solve just this problem, without any other downsides. We'll look into this solution.

[weiznich]

To be clear here: I consider that as something that should be fixed in diesel at some point. It's just not my first priority right now, as dealing with the usual release after-match is currently more important.

I'm happy to accept a contribution for the retrying and I'm also ready to provide some guidance implementing that if someone is interested in addressing that "now". For me it's something that might be on my timeline somewhen later this year.

[fraillt]

I hate to be the bearer of bad news, but retry mechanism will for all databases.
This is not universal behavior. I have tested it with PostgreSQL, MySQL and SQLite, and it seams that only PostgreSQL is affected.
So, the problem with PostgreSQL is, that once there were any unhandled errors within transaction, no further queries can run inside that transaction.
Here's the relevant snippet.

    // execute it to cache the statement
    let r = users::table.load::<(i32, String)>(conn).unwrap();
    assert!(r.is_empty());
    let r = seats::table.load::<(i32, i32, i32)>(conn).unwrap();
    assert!(r.is_empty());

    // change the type
    diesel::sql_query("ALTER TABLE users ALTER COLUMN name TYPE TEXT")
        .execute(conn)
        .unwrap();

    conn.transaction::<_, Error, _>(|conn| {
        // works just fine
        seats::table.load::<(i32, i32, i32)>(conn).unwrap();
        let r = users::table.load::<(i32, String)>(conn);
        assert_unknown_err_message(r.err().unwrap(), "cached plan must not change result type");
        // no other query can run successfully if there were any errors in transaction
        let r = seats::table.load::<(i32, i32, i32)>(conn);
        assert_unknown_err_message(
            r.err().unwrap(),
            "current transaction is aborted, commands ignored until end of transaction block",
        );
        Ok(())
    })
    .unwrap();
    // once we're out of transaction everything works as usual
    seats::table.load::<(i32, i32, i32)>(conn).unwrap();