feat(common): added postgress async pool wrapper #1631
Conversation
|
I started porting treeform's pg library from @arnetheduck @jm-clius @cammellos Can I get a first review from you? |
| ## Database connection pool | ||
|
|
||
| type PgAsyncPoolState {.pure.} = enum | ||
| Live, |
There was a problem hiding this comment.
Maybe use Closed as the default state (first element of the enum) to avoid bugs
|
|
||
| # wait for a free connection without blocking the async runtime | ||
| while pool.isBusy(): | ||
| await sleepAsync(0.milliseconds) |
There was a problem hiding this comment.
Actually, you can probably do a poll-free version by using an Queue that would you check in relaseConn (or maybe AsyncQueue)
There was a problem hiding this comment.
AsyncQueue? 👀 👀 👀 👀 👀 👀 👀 👀
| poolOptions: PgAsyncPoolOptions | ||
|
|
||
| state: PgAsyncPoolState | ||
| conns: seq[DbConn] |
There was a problem hiding this comment.
Personally I would have created a new type to hold the DbConn & bool instead of two seq
There was a problem hiding this comment.
It's another possibility, yes 🙂 Some parts come from treeforms's implementation. This is one of them.
| $pqErrorMessage(db) | ||
|
|
||
|
|
||
| proc open*(options: PgConnOptions): common.PgResult[DbConn] = |
There was a problem hiding this comment.
This proc is not actually async, right?
There was a problem hiding this comment.
No... It is sync. Just a convenience wrapper that takes the Pg options and returns a result.
There was a problem hiding this comment.
So personally I would just have a numConnections instead of min/max, don't want my program to randomly do blocking IO when I reach an arbitrary limit. Since the connections >min are not closed, you would probably end up with always maxConnections anyway
Though would also have to check what happens if for instance, the server restarts (or worst, just shuts down)
There was a problem hiding this comment.
Though would also have to check what happens if, for instance, the server restarts (or worst, just shuts down)
Absolutely! There are a thousand corner cases that I am not covering here. But for time's sake, I am trying to have a little MVP covering the "happy-path" cases, and we can iteratively improve the implementation.
| return err(db.error) | ||
|
|
||
| if pqisBusy(db) == 1: | ||
| await sleepAsync(0.milliseconds) # Do not block the async runtime |
There was a problem hiding this comment.
That's good for a PoC, but ideally we would register the FD in chronos and wait for data to be available
There was a problem hiding this comment.
Fully agree 😁
If we can find a Sans-IO version of the Postgres protocol we can try that. For now... I think that, if we don't want to reimplement the Postgres protocol, we should stick to this approach.
There was a problem hiding this comment.
You don't have to reimplement postgres
https://nim-lang.org/docs/postgres.html#pqsocket%2CPPGconn
This proc will give you the FD of the underlying postgres connection
You would you have to do something like:
while pqIsBusy(db) == 1:
await waitForRead(AsyncSocket(pqsocket(db))
pqconsumeInput(db)Unfortunately, chronos doesn't has a waitForRead, so that would need to be added
Again, fine for a first version, but something to improve on later IMO
More info on the async usage of pg here: https://www.postgresql.org/docs/current/libpq-async.html
And an example in C : https://github.com/Menduist/sample_coroutine_api/blob/master/src/db/re_pgsql.c (callback based)
Context
In this comment: #1590 (comment)
And in this issue: #1604
Description
To overcome the blocking nature of the standard library's
db_postgres, I mimicked treeform's pg package implementation. I wrapped the connections in an asynchronous connection pool.