YDB Support #3
YDB Support #3
Conversation
database/ydb/README.md
Outdated
| |:--------------------:|:-----------------------------------------------------------:| | ||
| | `x-auth-token` | Authentication token. | | ||
| | `x-migrations-table` | Name of the migrations table (default `schema_migrations`). | | ||
| | `x-use-grpcs` | Enables gRPCS protocol for YDB connections (default grpc). | |
There was a problem hiding this comment.
not required. Protocol defined in connection string directly
There was a problem hiding this comment.
golang-migrate uses scheme to determine db driver --
https://github.com/golang-migrate/migrate/blob/master/database/driver.go#L86
| } | ||
| }() | ||
|
|
||
| m, err := migrate.NewWithDatabaseInstance("file://./examples/migrations", "ydb", d) |
There was a problem hiding this comment.
it works without a dot here, but the tests for other dbs use the same approach, so I decided to leave it with dot for unification
database/ydb/ydb.go
Outdated
| return err | ||
| } | ||
|
|
||
| res, err := db.driver.Scripting().Execute(context.TODO(), string(rawMigrations), nil) |
There was a problem hiding this comment.
In other places you use db.driver.Query(), here we should also use it
database/ydb/ydb.go
Outdated
|
|
||
| createVersionTableQuery := fmt.Sprintf(` | ||
| CREATE TABLE IF NOT EXISTS %s ( | ||
| version Uint64, |
database/ydb/ydb_test.go
Outdated
| } | ||
| defer func() { _ = d.Close(ctx) }() | ||
|
|
||
| res, err := d.Scripting().Execute(ctx, ` |
| `, db.config.MigrationsTable) | ||
|
|
||
| insertVersionQuery := fmt.Sprintf(` | ||
| INSERT INTO %s (version, dirty, created) VALUES (%d, %t, CurrentUtcTimestamp()) |
There was a problem hiding this comment.
We can use a query with parameters here to eliminate compilation time from migration progress. I am not sure if it is meaningful, though, it is okay to do it with a Sprintf.
There was a problem hiding this comment.
I guess the difference would be in µs and this is a script which will be run manually, so I think Sprintf would be ok:)
database/ydb/ydb.go
Outdated
| ctx := context.TODO() | ||
|
|
||
| getVersionQuery := fmt.Sprintf(` | ||
| SELECT version, dirty FROM %s ORDER BY version DESC LIMIT 1 |
There was a problem hiding this comment.
You clear the table in SetVersion(), then maybe we do not need ORDER BY DESC LIMIT 1 here?
There was a problem hiding this comment.
ORDER BY DESC is not needed, and actually LIMIT 1 also not needed
but all other adapters use LIMIT 1, so I left this for unification
database/ydb/ydb.go
Outdated
| return err | ||
| } | ||
|
|
||
| dropQuery := fmt.Sprintf("DROP TABLE `%s`", table) |
There was a problem hiding this comment.
Other db adapters in the repo use DROP TABLE IF EXISTS, I am not really sure why, but maybe we should keep it consistent and add IF EXISTS here
asmyasnikov
force-pushed
the
support_ydb
branch
from
52281d3 to
1e4515c
Compare
No description provided.