[Ideas Wanted] Generically accepting a &Pool or &mut Connection and allowing the argument to be used more than once

[mehcode]

Sometimes I find it easier to start with how this should look to the user:

// async fn foo(executor: impl Executor<'_, Database = Postgres>) -> anyhow::Result<()> { 
async fn foo(executor: impl PgExecutor<'_>) -> anyhow::Result<()> { 
  assert!(executor.fetch_one("SELECT 2").await?.get(0) == 2_i32);
  assert!(executor.fetch_one("SELECT 5").await?.get(0) == 5_i32);

  Ok(())
}

Anything goes, no matter ugly it makes the SQLx side look as long as it doesn't cause the compiler to ICE if you look at it funny (so try to avoid too many HRTBs).

I can think of a few different ways to do this given full access to the nightly compiler (GATs, arbitrary self types) but we do need this to work on stable.

The key issue is fetch_one is defined as fetch_one(self, query: impl Execute). Executor is then implemented for &Pool and &mut PgConnection, &mut MssqlConnection, etc.

Note that this example works if instead of the generic argument it is &mut PgConnection or &Pool. This works because of the implicit re-borrowing that happens outside of a generic context. I'm reasonably sure we can't mimic re-borrowing in the trait system due to lack of GATs.

[mehcode]

We now ( on master ) have an Acquire trait. This is not the clean API I was hoping for but it does work and that may be all you need for an interface:

async fn make_things_happen<'c, A>(conn: A) -> anyhow::Result<()>
where
    A: Acquire<'c>,
    for<'e> &'e mut <A::Database as Database>::Connection: Executor<'e>,
{
    let mut conn = conn.acquire().await?;

    conn.execute("SELECT 1").await?;
    conn.execute("SELECT 1").await?;
    conn.execute("SELECT 1").await?;

    Ok(())
}

We use this internally with the programmatic migrations.

[bitglue]

I just learned Rust this week so this could very well be a terrible idea, but I think the solution is to implement Executor not on &Pool and &mut Transaction but on Pool and Transaction.

The root of the issue is that any function which accepts an Executor doesn't know that the things implementing that trait are actually references, and so now the value must be moved or copied. This can kinda be fixed for &Pool by adding Executor<...> + Copy to the trait bounds since &Pool is copiable, but &mut Transaction is not so you're still stuck with a function that can't work for any kind of Executor.

Here's a simplified example of the problem:

trait Speak {
    fn speak(self);
}

struct Cat {
    name: &'static str,
}

impl Speak for &Cat {
    fn speak(self) {
        println!("{} says meow", self.name);
    }
}

struct Dog {
    name: &'static str,
    times_spoken: i32,
}

impl Speak for &mut Dog {
    fn speak(self) {
        self.times_spoken += 1;
        println!(
            "{} says woof, and has woofed {} times",
            self.name, self.times_spoken
        );
    }
}

fn speak_twice<T>(speaker: T)
where
    T: Speak,
{
    speaker.speak(); // this moves speaker
    speaker.speak(); // compilation error: use of moved value: `speaker`
}

fn main() {
    let cat = Cat { name: "Oreo" };
    let mut dog = Dog {
        name: "Spot",
        times_spoken: 0,
    };

    cat.speak();
    dog.speak();

    speak_twice(&cat);
    speak_twice(&mut dog);
}

Here, Cat is like Pool, and Dog is like Transaction (and Connection). Even though the things being passed around are in fact references, we run into trouble with speak_twice because at that point it isn't known that these things are references.

Moving the implementations of Speak to the types, rather than references to the types, resolves the issue:

trait Speak {
    fn speak(&mut self);
}

struct Cat {
    name: &'static str,
}

impl Speak for Cat {
    fn speak(&mut self) {
        println!("{} says meow", self.name);
    }
}

struct Dog {
    name: &'static str,
    times_spoken: i32,
}

impl Speak for Dog {
    fn speak(&mut self) {
        self.times_spoken += 1;
        println!(
            "{} says woof, and has woofed {} times",
            self.name, self.times_spoken
        );
    }
}

fn speak_twice<T>(speaker: &mut T)
where
    T: Speak,
{
    speaker.speak();
    speaker.speak();
}

fn main() {
    let mut cat = Cat { name: "Oreo" };
    let mut dog = Dog {
        name: "Spot",
        times_spoken: 0,
    };

    cat.speak();
    dog.speak();

    speak_twice(&mut cat);
    speak_twice(&mut dog);
}

Now the compiler knows within speak_twice() that we're dealing with a reference, and that reference can be passed around according to the usual rules.

[franleplant]

@bitglue the main problem is that the Executor trait is a consuming trait (all methods consume self) that seems designed as a use and drop object that doesn't care about what is the concrete type implementing it.
That is why it works pretty well with references, because references are really safe to consume and drop in a one-off manner.

One possible solution is to make Executor implement / depend on the Copy trait, since all the current implementors of Executors are reference types, then it looks doable to add that change. That would enable you to do executor.copy() and pass more references around while the compiler will take care of you with the usual borrowing rules.

EDIT: this solution wouldn't work because there are a bunch of &mut references that cannot be copied AFAIK.

The other solution is to use Acquire which works pretty well AFAIK, but I didn't need the for in the trait bound, because I don't need to be generic on the DB type.

async fn make_things_happen<'c, C>(conn: C) -> anyhow::Result<()>
where
    C: sqlx::Acquire<'c, Connection = sqlx::PgConnection>
{
    let mut conn = conn.acquire().await?;

    conn.execute("SELECT 1").await?;
    conn.execute("SELECT 1").await?;
    conn.execute("SELECT 1").await?;

    Ok(())
}

This worked for sqlx version: 0.5.5

[bitglue]

Yeah Acquire works, but it seems like a rather clunky workaround. If the problem is in speak_twice we don't know that we're dealing with a reference, then what we're doing with Acquire is turning the Executor (which is a reference, but the compiler doesn't know that) back into a reference (now the compiler knows). Seems a rather silly thing to have to do, when by moving the implementation of Executor to be on the structs, not refs to the structs, and having the methods take a parameter of &mut self rather than self, the problem (of having a reference but not knowing it) can be avoided altogether.

[mehcode]

We used to do as you're suggesting with impl Executor for Pool. However, that would require &mut access to a Pool. Which, arguably isn't hard to get as you can clone them freely. However, it was found to be far more confusing to more users than the current situation so we changed it.

It's possible to fix the Executor trait completely with GATs which we'll be doing as soon as GATs hit stable.

[bitglue]

I kinda wonder this "problem" of needing mut access to a Pool is a smell suggesting Pools should not be Executors. For example, consider something like

fn f<E: sqlx::Executor>(e: E) {
    sqlx::query("set statement_timeout=10s").execute(e).await.unwrap();

    // this will take a little while, good thing we increased the timeout!
    sqlx::query("update some_table set foo='bar'").execute(e).await.unwrap();
}

This works fine for a connection or a transaction, but is broken for a pool because a pool does not guarantee the statements will be executed on the same connection. A pool does not guarantee read-after-write consistency and so writing a function to execute a series of statements on a pool is very different from executing on a connection. This generalizes to all kinds of problems. A pool might have multiple connections to multiple replicas, each with a different replication delay. Or, a function might make use of other functionality which is scoped to the connection, besides set there's also prepare and create temporary table.

And yet, it's very easy to forget about that and want to write a function which compiles for any Executor but which fails nondeterministically when the Executor happens to be a Pool.

[emwalker]

It's possible to fix the Executor trait completely with GATs which we'll be doing as soon as GATs hit stable.

Just curious whether this has happened. I've been trying to create a function parameter that allows both a &Pool and a &mut Transaction, and I've been running into the following forced choices:

1. Use a type of Executor + Copy, which allows you to call the executor multiple times, but you can't pass a &mut Transaction
2. Use a type of Executor (with no Copy), which allows you to pass a &mut Transaction, but you can only call the executor once

I'm new to Rust, and I might be doing something wrong.

[jplatte]

This has not happened. GATs are not stable yet (though approaching stabilization). See rust-lang/rust#44265.

[Kinrany]

Bumping this now that GATs are stable.