feat: support Spanner PostgreSQL databases#199
Conversation
fc18997 to
d3ba44b
Compare
Add gorm data type names for PostgreSQL-specific types. This prevents gorm from interpreting these structs as models, and automatically creates columns with the correct data type when used as a field in a model. Needed for googleapis/go-gorm-spanner#199
Add gorm data type names for PostgreSQL-specific types. This prevents gorm from interpreting these structs as models, and automatically creates columns with the correct data type when used as a field in a model. Needed for googleapis/go-gorm-spanner#199
1058973 to
f03f0c6
Compare
1746416 to
2c3095e
Compare
Adds support for using the gorm PostgreSQL dialect with Spanner. Connections to Spanner use the Spanner database/sql driver, and not one of the PostgreSQL database drivers (e.g. it does not use pgx). Using this dialect with Spanner does not require the use of PGAdapter.
2c3095e to
85456de
Compare
|
|
||
| func (m spannerPostgresMigrator) disableAutoMigrateBatching() bool { | ||
| if cfg, ok := m.Dialector.(Dialector); ok { | ||
| return cfg.SpannerConfig.AutoOrderByPk |
There was a problem hiding this comment.
Should it not return cfg.SpannerConfig.DisableAutoMigrateBatching?
There was a problem hiding this comment.
Good catch, done (and added test for this)
| } | ||
|
|
||
| // TODO: Remove | ||
| _ = os.Setenv("SPANNER_EMULATOR_HOST", "localhost:9010") |
| conn = c | ||
| } else { | ||
| sqlDB, _ := db.DB() | ||
| conn, _ = sqlDB.Conn(context.Background()) |
There was a problem hiding this comment.
If db.DB() returns an error (e.g., pool exhausted, connection closed), sqlDB will be nil and sqlDB.Conn() will panic with a nil pointer dereference.
NOTE: The GoogleSQL version has the same pattern, so this might be copied intentionally, but it's still a bug in both. At the very least, return a migrator that reports errors rather than crashing.
There was a problem hiding this comment.
Yeah, that is a good point. The probability of this actually returning an error is minimal, as you can only get to this point if you already have a valid database connection. But nonetheless you're right that we should check this. I've changed this to return a migrator that uses the default ConnPool. That again will then cause the same error to happen when the application tries to use the migrator.
|
|
||
| func BeforeUpdate(db *gorm.DB) { | ||
| if db.Statement != nil && db.Statement.Schema != nil && db.Statement.Schema.PrimaryFieldDBNames != nil { | ||
| omit := append(db.Statement.Omits, db.Statement.Schema.PrimaryFieldDBNames...) |
There was a problem hiding this comment.
append(db.Statement.Omits, ...) may mutate the underlying array of db.Statement.Omits if it has capacity, causing a data race in concurrent usage.
Per the https://go.dev/ref/spec#Appending_and_copying_slices, append may reuse the underlying
array. This is different from the GoogleSQL version which calls db.Statement.Omit(db.Statement.Schema.PrimaryFieldDBNames...) directly. Use the same pattern or explicitly copy the slice first.
| tx.Exec(fmt.Sprintf(`alter database "%s" set spanner.default_sequence_kind = 'bit_reversed_positive'`, m.CurrentDatabase())) | ||
| } | ||
| err = m.Migrator.AutoMigrate(values...) | ||
| if err == nil { |
There was a problem hiding this comment.
in case of error StartBatchDDL() will be open, should we call AbortBatch()
There was a problem hiding this comment.
Yes, I've added a defer func for this.
| stmt.Schema.Fields = append(stmt.Schema.Fields, pk) | ||
| stmt.Schema.FieldsByDBName[pk.DBName] = pk | ||
| stmt.Schema.FieldsByName[pk.Name] = pk | ||
| // stmt.Schema.PrimaryFields = append(stmt.Schema.PrimaryFields, pk) |
There was a problem hiding this comment.
nit: here and everywhere we remove the commented out lines OR make comment on why they were disabled?
| @@ -0,0 +1,260 @@ | |||
| package spannerpg | |||
There was a problem hiding this comment.
nit: add copyright/license header
| @@ -0,0 +1,768 @@ | |||
| package spannerpg | |||
There was a problem hiding this comment.
nit: add copyright/license header
|
|
||
| func (m spannerPostgresMigrator) dropTableIndexes(tx *gorm.DB, stmt *gorm.Statement) error { | ||
| currentSchema, curTable := m.CurrentSchema(stmt, stmt.Table) | ||
| fk := m.queryRawWithTx(tx, "select index_name from information_schema.indexes WHERE table_schema = ? AND table_name = ? AND index_type in ('INDEX') AND spanner_is_managed='NO'", currentSchema, curTable) |
There was a problem hiding this comment.
nit: The variable is named fk (suggesting foreign key) but holds index query results. This was copy-pasted from dropForeignKeysReferencingTable where fk makes sense. Minor but confusing for readers.
3a0ae2b to
9369b99
Compare
2 participants
Adds support for using the gorm PostgreSQL dialect with Spanner. Connections to Spanner use the Spanner database/sql driver, and not one of the PostgreSQL database drivers (e.g. it does not use pgx). Using this dialect with Spanner does not require the use of PGAdapter.
Fixes #198