Connection pool

[moigagoo]

Alternative approach to #175

This is an attempt to come up with a simpler, more explicit pool implementation. The general concept: #175 (comment)

[moigagoo]

@PhilippMDoerner please take a look at this PR. I've only implemented Pool for SQLite so far, just to test-drive the concept.

So far, the concept looks viable. I've tested it against concurrent access, seems to work.

Two policies for pool exchaustion were implemented: raise and extend. You can reset the pool after extension.

[PhilippMDoerner]

Do you have any plans yet on if you want to have a shared module with all key procs / if you want to make this module generic?
The only thing I can see so far is the differing implementations on getDb between the sqlite and the postgres module. Feels tempting to have a single generic pool module, import and export that in both sqlite/postgres and use mixin getDb wherever getDb is used.

[moigagoo]

@PhilippMDoerner

Do you have any plans yet on if you want to have a shared module with all key procs / if you want to make this module generic?
The only thing I can see so far is the differing implementations on getDb between the sqlite and the postgres module. Feels tempting to have a single generic pool module, import and export that in both sqlite/postgres and use mixin getDb wherever getDb is used.

I would prefer a single module but so far I can't see how I can do that. If I have a wrapper type DbConn = sqlite.DbConn | postgres.DbConn, I get these “ambigous call” errors because it's now unclear what newSeq[DbConn]() or close conn means. The compiler can't decide which DbConn I mean here.

So I'm OK to either have sqlite/pool and postgres/pool or just put the pool code into sqlite and postgres modules.

[PhilippMDoerner]

I'll need to play around with that later when I can. Worst case scenario I'd suggest a core pool modules + one for sqlite/postgres who do nothing but call the core modules but now with explicit types. The reason why I'm suggesting that is because it centralises business logic in one place. It does mean annoying glue code, but I'd prefer that over duplicated business logic 15.10.2022 12:16:09 Constantine Molchanov ***@***.***>:

…

@PhilippMDoerner[https://github.com/PhilippMDoerner] Do you have any plans yet on if you want to have a shared module with all key procs / if you want to make this module generic? The only thing I can see so far is the differing implementations on getDb between the sqlite and the postgres module. Feels tempting to have a single generic pool module, import and export that in both sqlite/postgres and use mixin getDb wherever getDb is used. I would prefer a single module but so far I can't see how I can do that. If I have a wrapper type *DbConn = sqlite.DbConn | postgres.DbConn*, I get these “ambigous call” errors because it's now unclear what *seq[DbConn]* or *close conn* means. The compiler can't decide which DbConn I mean here. — Reply to this email directly, view it on GitHub[#177 (comment)], or unsubscribe[https://github.com/notifications/unsubscribe-auth/APWD6LKWIBDG6T5FLYK5KX3WDJ76RANCNFSM6AAAAAARF2WCBQ]. You are receiving this because you were mentioned.[Verfolgungsbild][https://github.com/notifications/beacon/APWD6LJ3VJXXIKCIQVFKJGTWDJ76RA5CNFSM6AAAAAARF2WCBSWGG33NNVSW45C7OR4XAZNMJFZXG5LFINXW23LFNZ2KUY3PNVWWK3TUL5UWJTSMI3PPS.gif]

[moigagoo]

@PhilippMDoerner sure, I hate copy-pasted code too. I've tried using generics, to no avail so far. I'll give it some more thought.

Do I have your approval on the concept in general?

[PhilippMDoerner]

Overall looks good to me. I'm personally a fan of the "borrow/recycle" naming since "borrow" indicates that you're supposed to return the connection, but "pop/add" suits the general idea that the user can do whatever they want with the pool, you're just providing some utility procs to deal with it.

Though I do disagree with the doc comment changes from ref object to Model as stated in the comment for it.

[PhilippMDoerner]

Sidenote!
It could make sense to add a "warn" log or the like to the add proc if the amount of connections in there exceeds a specified multiple of the pools default size.
If, for example, the pool contains 1000x the amount of connections that the pool was originally sized for, I'd assume something is fishy.

[moigagoo]

It could make sense to add a "warn" log or the like to the add proc if the amount of connections in there exceeds a specified multiple of the pools default size.
If, for example, the pool contains 1000x the amount of connections that the pool was originally sized for, I'd assume something is fishy.

Sounds like an interesting idea! Maybe make this number configurable with -d:normdPoolWarning=1000 with a default of 100?

[PhilippMDoerner]

It could make sense to add a "warn" log or the like to the add proc if the amount of connections in there exceeds a specified multiple of the pools default size.
If, for example, the pool contains 1000x the amount of connections that the pool was originally sized for, I'd assume something is fishy.

Sounds like an interesting idea! Maybe make this number configurable with -d:normdPoolWarning=1000 with a default of 100?

I'd actually not provide a default if we do this via compiler-flag, that way only folks that want this kind of warning can opt into it, everybody else can just ignore it.
Further, this is actually the kind of log that would be most useful in production. You almost certainly won't encounter the kind of bug that would make you want to have this log in a staging environment, as these types of bugs tend to come around most likely when your system is under heavy load.

Thus I'd want to make this logging independent of how norm generally is dependent on the -d:normDebug flag.

[moigagoo]

'm personally a fan of the "borrow/recycle" naming since "borrow" indicates that you're supposed to return the connection, but "pop/add" suits the general idea that the user can do whatever they want with the pool, you're just providing some utility procs to deal with it.

I started with borrowDb and returned and ended up with pop and add because I suddenly realised that:

1. Theoretically, you can add a connection that has never been in the pool. So it's not necessarily returning, it may be just adding.
2. Pop and add are primitives just like pop and add for a seq. So it may be a good idea to name them similarly to their seq counterparts.

[PhilippMDoerner]

Does that actually get past the tests?
I'm a bit surprised, I'd have assumed that the compiler starts nagging about how it can't figure out whether to use getDb for sqlite or getDb for postgres

[PhilippMDoerner]

Given that the tests ran through, it actually does work... I'm so confused. Oh well, nice to see it function!

[moigagoo]

@PhilippMDoerner OK I'm ready to merge. I've decided not to add logging for now as this is would slow me down and this can be safely added later. This has little to do with pooling per se, it's more about adding a compilation flag and modifying the logging proc and then putting it all together.

If you don't see any blockers, I'll merge the PR.

[PhilippMDoerner]

Looks good! Really happy to see norm have connection pooling!