SQL Driver for KV interface #36
Conversation
virtual/registry/sql_kv.go
Outdated
| return nil, err | ||
| } | ||
|
|
||
| // TODO Improve indexing |
There was a problem hiding this comment.
What can be improved here? Just curious if you have any ideas
There was a problem hiding this comment.
I added a comment in here suggesting that we could add an index on the first few bytes of the byte array to improve the performance of iterPrefix.
virtual/registry/sql_kv.go
Outdated
| return nil, fmt.Errorf("failed to prepare delete statement: %w", err) | ||
| } | ||
|
|
||
| rangeStmt, err := db.Prepare("SELECT k, v FROM nola_kv WHERE k LIKE ?;") |
There was a problem hiding this comment.
Can you use any of these functions (sub string maybe?) to make this faster: https://www.postgresql.org/docs/9.0/functions-binarystring.html
virtual/registry/sql_kv.go
Outdated
| } | ||
|
|
||
| func (sqltransactionkv *sqlTransactionKV) getVersionStamp() (int64, error) { | ||
| return time.Since(sqltransactionkv.vst).Microseconds(), nil |
There was a problem hiding this comment.
I did a bit of googling and I think we can do something that is actually correct using Postgres using this function: https://pgpedia.info/p/pg_current_xact_id.html
https://www.postgresql.org/docs/current/functions-info.html#FUNCTIONS-PG-SNAPSHOT
double check me but it seems like calling that function in a new transaction is guaranteed to give us a monotonically increasing number like FDBs version stamp.
of course it won’t increase at the right “rate” which is a bit annoying…
At the very least instead of using local timestamp, can you run a transaction that calls a function in Postgres to get the time? This makes it so we only depend on a single nodes clock (Postgres) being consistent and increasing monotonically instead of every node in the clusters clock being accurate
There was a problem hiding this comment.
There is probably some smart way to combiners these two ideas but I’m not sure what it is. I’ll take calling the get time function in a Postgres transaction for now though
There was a problem hiding this comment.
There is probably some smart way to combiners these two ideas but I’m not sure what it is. I’ll take calling the get time function in a Postgres transaction for now though
Yeah I'll do this for now.
From what I saw in a comment, this should be increasing at a rate of 1mil/sec, from what I understand FDB does dummy transactions under the hood which keeps this value increasing. In this case I'd need to see when PG updates this value if no changes are made.
griffin
force-pushed
the
griffin/kv-sql
branch
from
1374a1f to
43b232c
Compare
|
Resolved most of the comments, need to finish testing, no need to re-review yet. |
Enables using a SQL database as a registry through the KV interface.
Todo:
Fixes #15