Driver tests for full lib/pq connection + necessary fixes
#883
Conversation
bc7a0a2 to
33a2de3
Compare
|
@bgentry Mind taking a look at this one? |
326972d to
be8d1eb
Compare
There was a problem hiding this comment.
It's always a bit unfortunate to have to move more in the direction of the lowest common denominator between drivers, but it's tolerable here. In the event we ever need to add another job state it might we'll need more than 8 bits and will need to adjust the approach.
| @@ -9,16 +9,17 @@ CREATE UNLOGGED TABLE river_leader( | |||
|
|
|||
| -- name: LeaderAttemptElect :execrows | |||
| INSERT INTO /* TEMPLATE: schema */river_leader (leader_id, elected_at, expires_at) | |||
| VALUES (@leader_id, coalesce(sqlc.narg('now')::timestamptz, now()), coalesce(sqlc.narg('now')::timestamptz, now()) + @ttl::interval) | |||
| -- @ttl is inserted as | |||
There was a problem hiding this comment.
not sure what this comment is meant to convey
There was a problem hiding this comment.
Ah doh, I started a comment, but didn't finish it. I just put this more expounded version in instead:
-- @ttl is inserted as as seconds rather than a duration because `lib/pq` doesn't support the latter| // lib/pq reads in `bits` (like `bits(8)`) as a bit string that's a normal | ||
| // decimal integer like 1111_1111 (_not_ 0b1111_1111). This function converts it | ||
| // back to binary representation. See the test suite for examples. |
There was a problem hiding this comment.
Yeah haha. There might be some other way around this, but at least this isn't that bad.
|
Does this also fix #566? |
|
@philippgille it should fix it! 🤞 |
So it turns out that while bearing some relation to `lib/pq` through various pieces that sqlc pulls in, the `riverdatabasesql` driver didn't actually work when used with `lib/pq`. When testing it, we'd open a `sql.DB` pool _via_ Pgx, so Pgx was doing all the heavy lifting. Here, add an alternate driver test strategy to test `riverdatabasesql` with a `lib/pq` pool, thereby ensuring that the full range of operations works even with no Pgx in the mix at all. Things were broken, so we get those fixed up. There we two main problems: * As described in #545, `lib/pq` doesn't handle intervals correctly, and reads them in as ints, which is a big problem because they're in nanoseconds. We work around this by injecting intervals (like leader TTLs) an second floats instead, which really doesn't feel that much worse. * The custom bits class for `unique_states` didn't work. To fix this I move to an alternate strategy of inserts `unique_states` as an integer, a technique that I've been using in SQLite because there's no `bits` type. I could make a strong argument that the use of an integer over `bits` would've been a better design from the get go anyway, because it makes all the bit-wise arithmetic much simpler as you use C-style conventions like `states & (1 << 3)` that are very widespread and well understood. Either way though, things work reasonably well if we insert as an integer, then transition to storage in bits. Fixes #545 and #566. Supersedes #882.
be8d1eb to
bff8718
Compare
Yeah for sure. I was reasonably happy that that none of the hacks to get this working were that gross. Code's different, but not obvious that much worse.
Yeah, although most of the integers we're working with are at least four bytes, so they should still mostly work in case we need more than those 8 bits. |
|
Thanks! |
This is just a dumb one where I was addressing code review feedback from the `lib/pq` change #883, specifically a half finished comment on `@ttl`, then forgot to push the change up to my branch before merging it. Here, address the problem by finishing the comment.
This is just a dumb one where I was addressing code review feedback from the `lib/pq` change #883, specifically a half finished comment on `@ttl`, then forgot to push the change up to my branch before merging it. Here, address the problem by finishing the comment.
So it turns out that while bearing some relation to
lib/pqthroughvarious pieces that sqlc pulls in, the
riverdatabasesqldriver didn'tactually work when used with
lib/pq. When testing it, we'd open asql.DBpool via Pgx, so Pgx was doing all the heavy lifting.Here, add an alternate driver test strategy to test
riverdatabasesqlwith a
lib/pqpool, thereby ensuring that the full range of operationsworks even with no Pgx in the mix at all.
Things were broken, so we get those fixed up. There we two main
problems:
As described in Elector interval field value out of range #545,
lib/pqdoesn't handle intervals correctly, andreads them in as ints, which is a big problem because they're in
nanoseconds. We work around this by injecting intervals (like leader
TTLs) an second floats instead, which really doesn't feel that much
worse.
The custom bits class for
unique_statesdidn't work. To fix this Imove to an alternate strategy of inserts
unique_statesas aninteger, a technique that I've been using in SQLite because there's no
bitstype. I could make a strong argument that the use of an integerover
bitswould've been a better design from the get go anyway,because it makes all the bit-wise arithmetic much simpler as you use
C-style conventions like
states & (1 << 3)that are very widespreadand well understood. Either way though, things work reasonably well if
we insert as an integer, then transition to storage in bits.
Fixes #545 and #566. Supersedes #882.