feat(postgres): complete implementation of driver and apply more tests #1785
Conversation
| query: string, | ||
| args: seq[string] = newSeq[string](0)): | ||
| Future[Result[seq[Row], string]] {.async.} = | ||
| proc runQuery*(pool: PgAsyncPool, |
There was a problem hiding this comment.
Nitpick, but would also stick with query here. "Query" can be both a verb and noun, if it's about sticking to verb names for procs? :)
There was a problem hiding this comment.
hehe, the reason was to avoid the next :)
let rowsRes = await s.connPool.query(query, args)
and instead have:
let rowsRes = await s.connPool.runQuery(query, args)
There was a problem hiding this comment.
Hmm..no strong feelings TBH:)
But I am a sucker for consistency, so I checked the SQLite implementation: https://github.com/waku-org/nwaku/blob/master/waku/common/sqlite/database.nim#L223
There was a problem hiding this comment.
Good point! I'll rename it back to keep it consistent. With less priority though, I'll submit a separate PR to try to enhance that proc name.
Thanks!
| Future[ArchiveDriverResult[void]] {.async.} = | ||
|
|
||
| let execRes = await s.connPool.exec( | ||
| """DELETE FROM messages WHERE id NOT IN |
There was a problem hiding this comment.
Probably not a problem, but this could potentially result in large set of IN, not sure if there could be some performance penalty when a lot of messages (thousands?) would need to be deleted. On the other hand, my SQL-fu is weak, so not sure if there is another way:)
There was a problem hiding this comment.
Ok, disregard, seems like this is actually the recommended way of doing it in postgres:)
There was a problem hiding this comment.
Thanks for the comment! We'll make performance tweaks in further phases.
Summary
postgres_driver retention policy methods plus more tests
Changes
Issue
#1604