Closed PhilippMDoerner closed 1 year ago
I wonder if would be better or at all possible to not hide the global pool variable. I.e., what if we have:
Pool
typenew
proc thataddConnections(n: Positive, conn: DbConn = getDb())
nextConn(p: Pool)
withPool
macro that is very similar to withDb
but a) calls nextConn
to get the actual connection and b) doesn't run close
on the connection after it's done; withDb
and withPool
could probably share a common core implementationThis way, you'd define your pool in a shared module and pass it around as an explicit global variable. What do you think? We could have a call and discuss it if you want.
I don't think this should be implemented here. Dropping databases has nothing to do with pooling.
AFAIU burst mode happens when all connections are busy but a request for a new one is made. Is this correct?
If it is, I think it should be replaced with an explicit check and call to addConnections
. Also, I think we should define several policies that regulate how the pool should react to this situation, e.g. addConnection, wait, cancel.
General Concept
I wonder if would be better or at all possible to not hide the global pool variable. I.e., what if we have:
1. the `Pool` type 2. accompanying `new` proc that 3. proc to explicitly add connections to the pool, e.g. `addConnections(n: Positive, conn: DbConn = getDb())` 4. iterator that gets the next free connection from the pool, e.g. `nextConn(p: Pool)` 5. `withPool` macro that is very similar to `withDb` but a) calls `nextConn` to get the actual connection and b) doesn't run `close` on the connection after it's done; `withDb` and `withPool` could probably share a common core implementation
This way, you'd define your pool in a shared module and pass it around as an explicit global variable. What do you think? We could have a call and discuss it if you want.
That would be great! Mostly because I think there's quite a bit of rapid exchange of ideas and concepts around this that can be done and that just goes by faster as a call.
dropDb
I don't think this should be implemented here. Dropping databases has nothing to do with pooling.
Done
Burst Mode
AFAIU burst mode happens when all connections are busy but a request for a new one is made. Is this correct?
If it is, I think it should be replaced with an explicit check and call to
addConnections
. Also, I think we should define several policies that regulate how the pool should react to this situation, e.g. addConnection, wait, cancel.
That is entirely correct. I think we can also discuss this during the call, mostly so I get a better idea of the vision you have behind it.
THIS IS NOT A FINISHED PR
PR for Issue #50
Note
I would like to cooperate on this with you @moigagoo though, to throw ideas back and forth for how exactly we implement this in norm. I've provided a first stab on how this could look like. Please give your general thoughts as well as your specific thoughts about the suggestions I'm making in the "How the pool works" section (marked with "Idea N", N being a number).
How the pool works
The pool is basically just a glorified private global variable of type
seq[DbConn]
with a lock for thread-safe access. This seq can not be interacted with outside of the specifically provided procs that make up the public API of this module.Initially you basically just need to initialize the pool. Upon initialization it will create said seq and populate it with
DbConn
instances created via a proc that creates connections. That proc can be supplied by the user, or they supply nothing, in that case the currentgetDb
proc is used to create connections. (Idea 1: We may want to add checks that if getDb is used, that the env variables must've been set and that we just crash the program if they haven't been).The user then can interact with the pool in an identical manner to tinyPool and more: 1) borrow/recycle connections 2) use
withDb
(which now borrows/recycles connections instead of creating/closing them) 3) usegetDb
to actually create a connection (Idea 2: I do not think it is a good idea to allow the user to circumvent the pool as there is no benefit to them and instead this puts them at risk if they forget to close a connection. I would like to make this proc private, even if that is a breaking API change) 4) use dropDb to drop the database (Idea 3: I am currently uncertain about whether having this makes sense, I feel tempted to suggest that we just remove this proc as well to keep the API simpler)Should the user call
borrowConnection
while the pool is empty it goes into burst mode (functionality is the same as in tinypool, so here the explanation from my readme there):Code organization
genericPool.nim
is the center-piece for logic regarding handling connection pools. It also contains and exports all possible gobal variables needed for interacting with any kind of connection pool. It gets imported by individualpool.nim
files that basically just contain the instance of the pool as well as the public API procs made specific for this particular pool type (aka whether it's forndb/sqlite
orndb/postgres
. It further exports the global variables needed to interact with this specific kind of connection pool.pool.nim
modules then get imported and exported by the respectivesqlite.nim
/postgres.nim
modules, making the API for interacting with the connection pool immediately accessible as part of importingsqlite.nim
/postgres.nim
.I've further refactored out various things from the individual
sqlite.nim
andpostgres.nim
modules intogenericPool.nim
, since these are about handling the connection to the database and it made sense to me to centralize that intogenericPool.nim
.If you have a different idea on how this should be structured please give feedback! As stated, this is just a first stab at it.
Questions
I have a question here though: I have some trouble understanding how the proc
dropDb
for postgres works:What exactly is "template1" and why is it inserted there instead of using the
dbNameEnv
variable?