refactor: Wrap database pool in class for reusability #5494
Conversation
| module.exports = { | ||
| ...defaultPool, | ||
| // Class methods are non-enumerable, so we can't spread them. We'll manually | ||
| // attach them to `module.exports`. |
There was a problem hiding this comment.
Ah, I didn't know this. Explicit pass-throughs still aren't too bad here.
| const sqldb = require('./sql-db'); | ||
|
|
||
| describe('sqldb', () => { | ||
| it('exports the full PostgresPool interface', () => { |
| * the fact that we tried to acquire new clients inside of transactions, which | ||
| * ultimately lead to a deadlock. | ||
| */ | ||
| this.alsClient = new AsyncLocalStorage(); |
There was a problem hiding this comment.
Do we in fact still want the AsyncLocalStorage code here? I'm worried that we might inadvertently rely on it, only to have it break in some complex and impossible-to-debug way. An alternative might be to use AsyncLocalStorage to throw an error if anyone tries to nest a DB call inside a transaction?
There was a problem hiding this comment.
Until we merge #5489, I want to keep this here. Even then, I think it's still useful, and it mirrors how transactions work in things like Ruby on Rails (no need to manually pipe around a client, which is a footgun). That said, after the other PR, we really shouldn't have any more transactions, so this code should then be unused.
This PR introduces a
PostgressPoolclass that encapsulates our opinionated layer on top ofnode-postgres. Thesqldbmodule retains the same interface as before - that is, we still expose a default pool. However, this change allows us to potentially have multiple database pools, which we'll take advantage of in the the future to improve how our named locks work.