Add YDB support #592

gingersamurai · 2023-08-29T11:32:42Z

Hello! I am part of YDB application team and we added YDB support to the Goose. We would greatly appreciate any feedback on it. Thanks!

asmyasnikov · 2023-09-21T08:39:26Z

@mfridman Can you see this PR?

mfridman · 2023-10-06T13:54:36Z

Yep, just getting back into the swing of things (long few weeks of travelling).

I see there are a few PR's floating around, which one should I take a look at? I presume this one.

Thanks for putting up a PR, and apologize for the delay.

mfridman · 2023-10-23T12:21:26Z

cmd/goose/main.go

@@ -219,6 +220,7 @@ Examples:
    goose mssql "sqlserver://user:password@dbname:1433?database=master" status
    goose clickhouse "tcp://127.0.0.1:9000" status
    goose vertica "vertica://user:password@localhost:5433/dbname?connection_load_balance=1" status
+    goose ydb "grpcs://localhost:2135/local?go_query_mode=scripting&go_fake_tx=scripting&go_query_bind=declare,numeric" status


Can you drop a link to the docs or a comment on what these are?

go_query_mode=scripting go_fake_tx=scripting go_query_bind=declare,numeric

go_query_mode - it is required because YDB have different methods for different types of queries (DDL, DML, etc.). Docs
go_fake_tx - special mode for external tools like goose, which requires a transactional DDL. Release in v3.44.0
go_query_bind=declare,numeric - released in v3.44.0 and published in article database/sql bindings for YDB in Go

mfridman · 2023-10-23T12:24:06Z

internal/testdb/ydb.go

+)
+
+const (
+	YDB_IMAGE    = "cr.yandex/yc/yandex-docker-local-ydb"


Any chance we can pull this from DockerHub?

We have an different registry ghcr.io/ydb-platform/local-ydb (official github registry).
Is it ok for you?
If ok I will change a registry in next commit

Yes, that'd be great. It makes it a bit easier to maintain if the images are pulled from the same 1-2 registries. If those are down so be it.

Nothing against other registries, but if we have many different ones it increases the chances of flakes.

Switched docker image to ghcr.io/ydb-platform/local-ydb

mfridman · 2023-10-23T12:33:29Z

internal/dialect/dialectquery/ydb.go

+
+func (c *Ydb) CreateTable(tableName string) string {
+	return fmt.Sprintf(`
+		CREATE TABLE %s (


Where possible, we try to add NOT NULL and a default. Would something like this make sense for ydb?

CREATE TABLE goose_db_version ( version_id Uint64 NOT NULL, is_applied Bool NOT NULL, tstamp Timestamp DEFAULT CURRENT_TIMESTAMP, PRIMARY KEY (version_id) );

Nits,

Remove the trailing semicolons from all queries

Use q=<query> and return fmt.Sprintf(q, tableName) pattern like all the other dialect queries (this makes it easier to do maintain, do large search/replace, etc.)

YDB not supports DEFAULT and NOT NULL constraints for non-pk columns in latest release (23.3). We will do that in one of the next release. With back-compatible of course. But in current release we can do autocomplete tstamp field explicitly on insert stage with built-in function CurrentUtcTimestamp()

Fixed in 3dba2c8 issues about trailing semicolons and patern fmt.Sprintf(q, tableName)

mfridman · 2023-10-23T12:37:27Z

internal/dialect/dialectquery/ydb.go

+}
+
+func (c *Ydb) ListMigrations(tableName string) string {
+	return fmt.Sprintf(`--!syntax_pg


Can you explain the --!syntax_pg ? I presume this is some hint if the database supports it, otherwise it's a noop?

Native YQL in YDB not supported next statement

SELECT version_id, is_applied FROM %s ORDER BY tstamp DESC

where ORDER BY column (tstamp) not present in results. It provokes undefined behaviour. And in YDB is too difficult to fix because some intermediate results are materialized in processing stage and missed tstamp column meta for ordering final results.

For garanted behaviour need to add tstamp to results like

SELECT version_id, is_applied, tstamp FROM %s ORDER BY tstamp DESC

But this ad-hoc brokes in database/sql scanner - because core of goose put only two destination values in this line.
I think it is not good implicit contract for get last migration by timestamp field/column. But I understand - this is legacy.
Thats why I maked another ad-hoc for this trouble with --!syntax_pg flag.

We have a milestone for support postgresql syntax. In this mode (--!syntax_pg) a query

SELECT version_id, is_applied FROM %s ORDER BY tstamp DESC

works fine. Because in postgresql-compatible mode we are processing queries with postgresql parser. And in this level we allowed some ttrade-off's for proccessing pg-queries under ydb core

added short comment into code about --!syntax_pg

asmyasnikov · 2023-10-25T08:18:00Z

@mfridman I fixed all your issues. Can you see latest version of code in PR?

mfridman · 2023-10-25T12:27:29Z

Makefile

@@ -50,6 +51,9 @@ test-e2e-clickhouse:
 test-e2e-vertica:
 	go test $(GO_TEST_FLAGS) ./tests/vertica

+test-e2e-ydb:
+	go test $(GO_TEST_FLAGS) -parallel=1 ./tests/e2e -dialect=ydb


Curious, why need to set -parallel=1?

Unfortunately, ydb container requires a lot of resources, and on a small machine, high parallelism with running many containers can provoke out-of-memory errors or unexpected behaviour. Because db docker image created for tests only, not for production. In production we have a distributed cluster of ydb nodes and perfect ACID guarantees

mfridman · 2023-10-25T12:28:58Z

internal/testdb/ydb.go

+		}
+	}
+	// Fetch port assigned to container
+	dsn := fmt.Sprintf("grpc://%s:%s/%s",


Ah interesting, this uses grpc. Shuld try https://connect.build ;)

mfridman · 2023-10-25T12:30:12Z

internal/dialect/dialectquery/ydb.go

+}
+
+func (c *Ydb) ListMigrations(tableName string) string {
+	// "--!syntax_pg" enables query processing with PostgreSQL-compatible syntax.


Perfect, thank you!

mfridman

Lgtm. Thanks for contributing a database 🎉

gingersamurai marked this pull request as draft August 31, 2023 09:17

asmyasnikov force-pushed the master branch 5 times, most recently from 49afa82 to 3892e0a Compare September 20, 2023 20:07

gingersamurai marked this pull request as ready for review September 20, 2023 20:09

asmyasnikov force-pushed the master branch from 3892e0a to bce95ca Compare September 21, 2023 05:34

asmyasnikov mentioned this pull request Sep 21, 2023

Added ydb support #437

Closed

asmyasnikov force-pushed the master branch from bce95ca to 81a886c Compare September 21, 2023 08:38

mfridman mentioned this pull request Oct 7, 2023

Development Roadmap for v3 #605

Closed

asmyasnikov force-pushed the master branch 2 times, most recently from 8378152 to 898d81d Compare October 22, 2023 12:32

mfridman reviewed Oct 23, 2023

View reviewed changes

asmyasnikov and others added 5 commits October 25, 2023 08:08

added container healthcheck methods

4987244

Added YDB support

54cc0cd

fix sql issues from mfridman

b076cc5

switched YDB docker image to ghcr.io/ydb-platform/local-ydb

d80fece

added comment for --!syntax_pg instruction

a9d9d4b

asmyasnikov force-pushed the master branch from 3dba2c8 to a9d9d4b Compare October 25, 2023 05:11

mfridman reviewed Oct 25, 2023

View reviewed changes

mfridman approved these changes Oct 25, 2023

View reviewed changes

mfridman merged commit 0c243fb into pressly:master Oct 25, 2023
9 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add YDB support #592

Add YDB support #592

gingersamurai commented Aug 29, 2023

asmyasnikov commented Sep 21, 2023

mfridman commented Oct 6, 2023

mfridman Oct 23, 2023

asmyasnikov Oct 23, 2023 •

edited

Loading

mfridman Oct 23, 2023

asmyasnikov Oct 23, 2023

mfridman Oct 23, 2023

asmyasnikov Oct 25, 2023

mfridman Oct 23, 2023 •

edited

Loading

asmyasnikov Oct 23, 2023

asmyasnikov Oct 23, 2023

mfridman Oct 23, 2023

asmyasnikov Oct 23, 2023 •

edited

Loading

asmyasnikov Oct 25, 2023

asmyasnikov commented Oct 25, 2023

mfridman Oct 25, 2023

asmyasnikov Oct 25, 2023

mfridman Oct 25, 2023

mfridman Oct 25, 2023

mfridman left a comment

Add YDB support #592

Add YDB support #592

Conversation

gingersamurai commented Aug 29, 2023

asmyasnikov commented Sep 21, 2023

mfridman commented Oct 6, 2023

Choose a reason for hiding this comment

asmyasnikov Oct 23, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mfridman Oct 23, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

asmyasnikov Oct 23, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

asmyasnikov commented Oct 25, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mfridman left a comment

Choose a reason for hiding this comment

asmyasnikov Oct 23, 2023 •

edited

Loading

mfridman Oct 23, 2023 •

edited

Loading

asmyasnikov Oct 23, 2023 •

edited

Loading