[yugabyte] SQL Migrations #1138
Conversation
barroco
force-pushed
the
yb-sql-migration
branch
from
dea4bff
to
0c4950d
Compare
barroco
force-pushed
the
yb-sql-migration
branch
from
0c4950d
to
dcf7215
Compare
There was a problem hiding this comment.
Partial review of everything except SQL migrations: LGTM, only minor comments for clarity / control flow.
Question about the SQL migrations: Since with yugabyte DBs we will be starting from scratch anyway, we may have an opportunity here to simplify the definitions by consolidating them into a single migration. Have you considered that?
| isCockroach = ds.Version.Type == datastore.CockroachDB | ||
| isYugabyte = ds.Version.Type == datastore.Yugabyte |
There was a problem hiding this comment.
Return error if !isCockroach && !isYugabyte as sanity check?
| // Reconnect to proper database (Yugabyte does not support cross-database references) | ||
| connectParameters = crdbflags.ConnectParameters() | ||
| connectParameters.ApplicationName = "db-manager" | ||
| connectParameters.DBName = dbName | ||
| ds, err = datastore.Dial(ctx, connectParameters) | ||
| if err != nil { | ||
| return fmt.Errorf("failed to reconnect to database %s: %w", dbName, err) | ||
| } |
There was a problem hiding this comment.
Factor this away in a function? (duplicated from above)
| @@ -157,25 +173,37 @@ func migrate(cmd *cobra.Command, _ []string) error { | |||
|
|
|||
| // Ensure SQL session has implicit transactions disabled for CRDB versions 22.2+ | |||
| sessionConfigurationSQL := "" | |||
| if ds.Version.SemVer.Compare(*semver.New("22.2.0")) >= 0 { | |||
| if isCockroach && ds.Version.SemVer.Compare(*semver.New("22.2.0")) >= 0 { | |||
| sessionConfigurationSQL = "SET enable_implicit_transaction_for_batch_statements = false;\n" | |||
There was a problem hiding this comment.
Put this inside the below block if isCockroach out of clarity?
| } else { | ||
| ds, err = datastore.Dial(ctx, connectParameters) | ||
| if err != nil { | ||
| return fmt.Errorf("failed to reconnect to database %s: %w", dbName, err) | ||
| } | ||
| migrationSQL = sessionConfigurationSQL + string(rawMigrationSQL) | ||
| } |
There was a problem hiding this comment.
While functionally equivalent, shouldn't this be inside a if isYugabyte block to be conceptually correct? IIUC this is required for yugabyte to reconnect to a different DB, this is not required because 'the DB is not cockroach'.
This PR adds migration scripts for yugabyte and adapts the migration CLI accordingly.
Migration
Yugabyte does not support the exact same data types and declaration syntax as CockroachDB, therefore the following adaptations have been made:
Data Types (CockroachDB -> Yugabyte):
Indexes:
Syntax required changes:
Additional notes
CI and Tests