feat: PostgreSQL support #922
Conversation
|
@rdmitr do we need to update our github workflows to also test in Postgres mode? ideally we test in both modes |
Absolutely! I shall figure it out |
| @@ -16,7 +16,7 @@ var _202405302121_store_decrypted_request = &gormigrate.Migration{ | |||
| return err | |||
| } | |||
|
|
|||
| err = tx.Exec("CREATE INDEX `idx_request_events_method` ON `request_events`(`method`);").Error | |||
| err = tx.Exec("CREATE INDEX \"idx_request_events_method\" ON \"request_events\"(\"method\");").Error | |||
There was a problem hiding this comment.
@rdmitr would it be better to be consistent in the format here?
The migration above uses ` and '
This migration escapes using \
| @@ -19,8 +20,11 @@ var _202406061259_delete_content = &gormigrate.Migration{ | |||
| return err | |||
| } | |||
|
|
|||
| if err := tx.Exec("VACUUM").Error; err != nil { | |||
| return err | |||
| // Cannot run when testing with txdb: VACUUM must be run outside of transaction. | |||
There was a problem hiding this comment.
I think we probably don't need this migration any more. Could we just comment out the content?
| if err := tx.Exec("VACUUM").Error; err != nil { | ||
| return err | ||
| // Cannot run when testing with txdb: VACUUM must be run outside of transaction. | ||
| if !testing.Testing() { |
There was a problem hiding this comment.
I think we probably don't need this migration any more. Could we just comment out the content?
| Migrate: func(tx *gorm.DB) error { | ||
| if tx.Dialector.Name() == "postgres" { | ||
| err := tx.Exec(` | ||
| CREATE SEQUENCE user_configs_id_seq; |
There was a problem hiding this comment.
add comment: // due to not explicitly setting the user_configs table to use auto increment in previous migrations
|
|
||
| var txSerializer = make(chan struct{}, 1) | ||
|
|
||
| func RunTransaction(db *gorm.DB, txFunc func(tx *gorm.DB) error) error { |
There was a problem hiding this comment.
I saw only some instances of db.Transaction have been updated to use this. Is that intentional?
There was a problem hiding this comment.
This feels a bit hacky if it is done just for tests, is there a way we can isolate the changes to the test DB instead?
| @@ -0,0 +1,124 @@ | |||
| package main | |||
There was a problem hiding this comment.
just thinking of the filename migrate we have different types of migrations. Could we be more clear e.g. db_migrate or something? (or cmd/db/migrate)
| return fmt.Errorf("failed to start transaction: %w", err) | ||
| } | ||
|
|
||
| // Table migration order matters: referenced tables must be migrated |
There was a problem hiding this comment.
can we fail the migration if the number of tables in the DB are not expected (e.g. we made a new table and forgot to update this migrate command?)
| return nil | ||
| } | ||
|
|
||
| if err := to.Create(data).Error; err != nil { |
There was a problem hiding this comment.
I got an error when I tried to create a new app after migrating from sqlite to postgres. It says duplicate ID not allowed.
My current apps:
id | name | description | app_pubkey |
created_at | updated_at | isolated | metadata | wa
llet_pubkey
----+------+-------------+------------------------------------------------------------------+------------------------------+-------------------------------+----------+----------+------------------------------------------------------------------
1 | test | | a0aa432130518b7bf921e6d55cce725f172693196fa1b8b8b103f2bcd2225c87 | 2025-01-10 15:29:08.26065+07 | 2025-01-10 15:29:08.268216+07 | f | {} | 19c16a92888d3ebbb594fa7fa814886b9bfd014395ca9e09f98165010ec01dd9
(1 row)
TODOs: