some concerns
caarlos0 opened this issue · 6 comments
Hi, first of all, great lib, thanks for the hard work!
I just have a few points:
- the lib creates a connection pool with the database, assuming the app is already using a postgres database, that will leave us with 2 database pools running. That is a resource waste, since each connection to the DB uses some amount of memory and the session store and the "app store" are not sharing the pool, so, we might have more connections opened than needed
- I see that you are using gorp to create the table and sequences... I think maybe it would be best to document that - or remove it entirely and provide a sql script instead.
I already did a PR fixing some minor stuff, I can try to work on this if you find it interesting...
Cheers!
Hi @caarlos0 and thanks!
The first point, I think it would be nice to support both having a stand alone pool and an existing pool that can be provided.
The second point, I think that it should be fine documenting that gorp is being used. Is using gorp an issue or why should it be explicitly documented?
It would be awesome if you'd like to work on this. 🙇
Great points, thanks!
@antonlindstrom Nice, I don't think that using gorp is an issue, what I meant is that docs should say that it will create a new table etc..
About the other thing, will try to do something right now :)
Although verbose, something like NewPGStoreFromPool would probably be clear.
Some questions:
- Does this method still use gorp to handle table creation/querying?
- If NOT, how does it manage that? Up to the package user? A
varcontaining raw SQL wrapped withCREATE IF NOT EXISTS?
Otherwise I'm okay with this. A lib I'm writing accepts a Redis connection pool for a similar reason - to prevent the lib from being the 'sole owner' of the connection.
Can you mock up the SQL? I think it'd make more sense to have it in
embedded in a var rather than a separate file.
I'm not typically an "ORM" user but for small self-contained libraries like
these an ORM has a lot of value. You'll need to test the SQL syntax and
make sure table creation and columns exist to get the same level of
"guarantees".
On Wed, Sep 9, 2015 at 8:56 AM Carlos Alexandro Becker <
notifications@github.com> wrote:
@elithrar https://github.com/elithrar about the naming, agreed, fixed
it already in #13 #13 [image:
💃]About your questions:
- I think we can still use gorp...
- I really like more the idea of having a SQL file and something in
docs saying that the user should execute it...—
Reply to this email directly or view it on GitHub
#10 (comment)
.