Removing libpq (to enable async)

[mehcode]

I did a foray into implementing async support with libpq because I naively thought that because libpq looks like it has non-blocking functions that you can effectively use them in a non-blocking fashion.

Sadly it's essentially impossible to efficiently use libpq in a non-blocking mode of operation. Establishing a connection does a good chunk of blocking IO (reading various files, a DNS query, etc.) and is 100% blocking if connecting with TLS.

---

To unblock async we have a few options:

• Use both postgres and tokio-postgres under the hood as our postgres driver. I'm not a fan of how the async and sync APIs are separated.

• Write our own native Rust driver using postgres-protocol.

• Write our own native Rust drive from scratch.

  ---

Looking at a few async bindings to postgres in other languages it seems they either roll their own protocol layer or use a worker pool internally for libpq.

What is the interest here? Does the Diesel team want to collaborate with rust-postgres or is their enough benefit to writing our own protocol layer? Note that I'm unsure rust-postgres is low-level enough to play nicely with Diesel.

[weiznich]

Before thinking about the actual implementation we need to figure out a usable interface. Our experiments indicate that this is unfortunately currently not possible.

[mehcode]

@weiznich I'm not seeing the issue there. Have you seen the examples here: https://github.com/mehcode/actix-diesel/blob/master/example/src/routes.rs

---

You seem to be trying to pass around &mut references to Connection which doesn't make sense to me (looking at the gist in the comment you linked). The current infra in Diesel uses & references to Connection.

---

Assuming I'm not seeing some blocking issue for .transaction; can we move forward with discussing implementation details and bring everything else up? I think not having a transaction closure for async is a worthwhile trade-off for now.

[weiznich]

You seem to be trying to pass around &mut references to Connection which doesn't make sense to me (looking at the gist in the comment you linked). The current infra in Diesel uses & references to Connection. Assuming I'm not seeing some blocking issue for .transaction; can we move forward with discussing implementation details and bring everything else up? I think not having a transaction closure for async is a worthwhile trade-off for now.

Unfortunately it is not that simple. Have you tried to replace all &mut with & in that example? That show the underlying problem that we tried to solve by using &mut :wink: To elaborate a bit more on this problem: The main issue here is that connections are Send but not Sync. Futures passed to some realistic executor like tokio or whatever are required to be Send. That's a fundamental thing that we cannot change. Now lets outline what should be minimally possible with an async diesel api:

• We want to be able to execute more than one query per async function
• We want to have some usable transaction api. The best one that comes in mind is something like the current closure based api. The first condition breaks the usage of & for connections because shared references are copy and we extend their lifetime across yield points. This means one reference could already be send to another thread, while another one is left behind in the original one. Fortunately rust prevents that. It is also indicated by the docs of Send where the impl for & requires T to be Sync. If we use &mut this seems to work fine. The second condition breaks then on the usage of &mut, because we are not able to say that everything returned by the closure inside of the connection needs to outlive the lifetime of the connection itself in a meaning full way.

I'm not seeing the issue there. Have you seen the examples here: https://github.com/mehcode/actix-diesel/blob/master/example/src/routes.rs

Just try do to more than one query. Than this will break down.

Back to the implementation details of a possible native postgres driver: I think both postgres and tokio-postgres are way to high level to provide the necessary api's for diesel. Namely that's accessing the raw representation of results/binds. I haven't looked at postgres-protocol yet, so I don't know what's provided there. If it only provides building blocks to implement the driver, we can try to use it. Otherwise we need to start from scratch. But again: That does not make much sense as long as we've found no way to articulate an usable api.

[alexander-irbis]

Before thinking about the actual implementation we need to figure out a usable interface. Our experiments indicate that this is unfortunately currently not possible.

I am somewhat puzzled by this approach.

Sharing RefCell between threads without synchronization is a really bad idea. Maybe a kind of Mutex is needed here?

&mut here is supposed to protect against simultaneously running of more than one future with a reference to Connection at once. This is not how it works in Rust with threads. But anyway, in this form, it looks like it doesn't make sense to use Connection with a multi-threaded runtime, and therefore to demand Send.

In futures-prevew-0.3.0-alpha.10 was added futures::lock::Mutex for async-aware synchronization.

On the other hand, tokio provides a single-threaded runtime, which doesn't demand Send.

[weiznich]

@alexander-irbis I will try to clear some things up: First please remember that playground is just an toy example, so not necessary everything there is 100% real. Also please assume that we have done quite a bit experimentation in that area and have quite a lot experience in what is possible in rust and what not.

Sharing RefCell between threads without synchronization is a really bad idea. Maybe a kind of Mutex is needed here?

It's not about sharing a RefCell, the RefCell is one of those things that are only there for demonstration. A real connection implementation will not contain a RefCell, but will have the same Send but not Sync constrains. See for example this documentation for libpq. Especially the following

One thread restriction is that no two threads attempt to manipulate the same PGconn object at the same time. In particular, you cannot issue concurrent commands from different threads through the same connection object.

This means PQConn and therefore the current PgConnection in diesel are Send but not `Sync. This constrain will likely remain even if we drop the libpq dependency.

&mut here is supposed to protect against simultaneously running of more than one future with a reference to Connection at once. This is not how it works in Rust with threads. But anyway, in this form, it looks like it doesn't make sense to use Connection with a multi-threaded runtime, and therefore to demand Send.

No &mut there is not supposed to protect against simultaneously use, that is already specified by not implementing Sync. The underlying problem is the following:

• Returned futures must be Send. And no that tokio single threaded runtime does not help, because we need to integrate with the remaining async ecosystem like actix, …, which assume Send (And Send makes sense for them because for a performant webserver you need to execute futures on a thread pool rather than a single thread.). So dropping this constrain would make the implementing basically unusable in the targeted area.
• async fn and returning impl Future are leaking Send and Sync.
• Shared references(&T) of not Sync types are not Send.

So if we have the following code:

async fn do_db_thing(conn: &DbConnection) -> Result<Res> {
    some_db_call_1(conn).async?;
    some_db_call_2(conn).async
}

the returned future will not be Send because the connection lives over some yield point.

In futures-prevew-0.3.0-alpha.10 was added futures::lock::Mutex for async-aware synchronization.

As far as I understand locking would remove much of the gains that a potential async implementation would provide, because you would be back to only execute one query per connection, which you already could to with the existing sync implementation and a thread pool.

[nsossonko]

Being somewhat of a noob to rust and definitely diesel, I just wanted to pipe in on the last comment and add: going async would provide tremendous gains regardless of being able to do multiple queries simultaneously per connection since the application would be free to keep working while the network activity goes on, that is really the entire purpose of async and the event loop. So if I have a web app that blocks on each query, I cannot accept new connections until the query completes whereas with async I can keep accepting connections and maintain very high throughput...

[sgrif]

As much as I've talked about how async requires removing libpq, I think we can actually make things work on top of libpq's built in async support, which would be much less work. Right now the main blocker here is the Send issue mentioned above. I believe this is incorrect behavior on Rust's part, so I'm hoping to convince folks that we can change this in Rust before committing to adding a ton of synchronization to make the connection Sync (when it is inherently not thread safe, and never will be)

[lnicola]

@nsossonko Sharing a connection across requests from different clients is a bad idea. Depending on your database server it either can't work, or can work, but is very finicky.

You'll want (at least) one connection per request, possibly with a connection pool to speed things up and ease the load on the server. You can already have that today by running your queries on a thread pool -- look into Tokio's blocking API or tokio_sync::oneshot::channel. Limiting the number of concurrent database queries is actually a good practice, as some servers really dislike running many queries at once. In any case, there's absolutely no reason to block the async event loop, even without an async API for Diesel.

[mehcode]

Sharing a connection across multiple requests is a bad idea. Depending on your database server it either can't work, or can work, but is very finicky.

@lnicola As I've been doing a lot of research and experimental development is this are, I'm very interested in sources on this. I've mainly played with postgres and I can see massive performance increases through overlapped query processing ( described briefly in http://2ndquadrant.github.io/postgres/libpq-async.html ) which is only possible if you can asynchronously share the connection.

---

@sgrif

As much as I've talked about how async requires removing libpq, I think we can actually make things work on top of libpq's built in async support, which would be much less work. [...]

I strongly recommend you try and drop libpq regardless of it being somewhat possible to bind with libpq. Not only is pure Rust an admirable goal (and would reduce the issue spam here, heh); but, libpq is abysmally slow. There are numerous examples out there but https://edgedb.com/blog/m-rows-s-from-postgres-to-python/ is a recent one I read.

[lnicola]

@mehcode Do you mean this?

If a command string contains multiple SQL commands, the results of those commands can be obtained individually. (This allows a simple form of overlapped processing, by the way: the client can be handling the results of one command while the server is still working on later queries in the same command string.)

That sounds more like pipelining: you submit multiple queries at once and read the results as they arrive. That's different from having multiple running queries on the same connection.

https://www.postgresql.org/docs/current/libpq-threading.html says:

One thread restriction is that no two threads attempt to manipulate the same PGconn object at the same time. In particular, you cannot issue concurrent commands from different threads through the same connection object. (If you need to run concurrent commands, use multiple connections.)

SQL Server has MARS, but only on a single thread:

MARS operations are not thread-safe.

---

But maybe I misread nsossonko's comment and they were only interested in the pipelining case, and not actually sharing the connection across threads or futures.

[nsossonko]

@lnicola I am very familiar with the postgres async functionality. However, I think the whole point of Futures and async functionality is being missed here. The point of non-blocking IO is so that you get to squeeze the most out of your CPU without wasting any time in idle operations. Even if you push the blocking IO to a separate thread, that is NOT async, it is now occupying a thread and wasting a CPU core on idle operation (waiting for network activity).

So if I can setup queries (whether I use a pool of connections or even just one connection depends on my particular use-case) to run asynchronously I can continue running other operations on the same thread. If I use just one connection I'd just queue up any other queries to run after the current running query is done, since it's all async anyhow. Performance would be through the roof.

[weiznich]

@nsossonko Please stop adding comments about how important an async database driver would be. Without substantial new information (like how to solve the Send problem mentioned in #399 and here) those comments are not helpful. They wont make anything happen faster (in contrast it will likely slow things down, because somebody needs to read those comments and so on…)

Just assume that we are aware of the advantages of an async adapter :wink:

[nicoburns]

We want to have some usable transaction api. The best one that comes in mind is something like the current closure based api.

Could we have an API similar to the API for Mutex? (instead of a mutex.lock() returning a MutexGuard, connection.beginTransaction() would return a Transaction, which can then be used in place of Connection)

In order to interact well with the ? operator and non-database code failing, perhaps committing could require an explicit transaction.commit(), and dropping a non-explicitly-committed Transaction implies rollback?

Something like:

fn perform_database_queries(conn: &Connection) -> Result<(), Box<dyn Error>> {
    let transaction = conn.beginTransaction(); // Probably doesn't need to be async, as the BEGIN could be sent with the first actual statement.

    sql_query("INSERT INTO ...").load(&transaction).await?;
    let results = sql_query("SELECT...").load(&transaction).await?;
    transaction.commit().await?;

     Ok(())
}

[weiznich]

@nicoburns Yes we've thought about that. Short answer it doesn't help. Longer Answer

[MOZGIII]

I think this is solved now since diesel-async exists and there are separate postgres (for libpq + postgres_backend) and just postgres_backend (for the actual bits of code for everything except the connection). Should this be closed?

[weiznich]

@MOZGIII I do not consider this closed in the sense of: That's implemented in diesel itself. That written: I also do not consider that as something that will be implemented in diesel in the near future. Therefore I will close this issue as not planned.

For anyone that looks for a pure rust diesel postgres implementation: It is possible and supported to implement the Connection trait outside of diesel. We encourage people to provide an alternative connection implementation as third party crate.

[MOZGIII]

Removing libpq is implemented though, so technically this particular issue is complete (although no async in the actual diesel).

[weiznich]

@MOZGIII The main point why I do not consider that as "implemented" is that diesel-async is a different project. It's not part of diesel itself, nor is it owned by the diesel github org.