Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

postgres support #360

Open
wants to merge 5 commits into
base: main
Choose a base branch
from
Open

postgres support #360

wants to merge 5 commits into from

Conversation

erma07
Copy link

@erma07 erma07 commented Jul 18, 2024

No description provided.

Comment on lines +35 to +40
func SQLReplacer(src string) string {
for nParam := 1; strings.Contains(src, "?"); nParam++ {
src = strings.Replace(src, "?", fmt.Sprintf("$%d", nParam), 1)
}
return src
}
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is that the only change in the code compared to the mysql backend or was there more you had to change?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If I can remember it correct, the migration sql files and the SQLReplacer was the only thing i changed in order to get postgres to work. Wanted the query's to remain as same as possible so there would be only copy-paste without much thinking to do.

Copy link
Author

@erma07 erma07 Aug 2, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

there are minor changes in backend/postgres/postgres.go file:

"github.com/golang-migrate/migrate/v4/database/postgres"

dsn := fmt.Sprintf("host=%s port=%d user=%s password=%s dbname=%s sslmode=require", host, port, user, password, database)

db, err := sql.Open("postgres", dsn)

db, err := sql.Open("postgres", schemaDsn)

dbi, err := postgres.WithInstance(db, &postgres.Config{})

m, err := migrate.NewWithInstance("iofs", migrations, "postgres", dbi)

opt(options)
}

dsn := fmt.Sprintf("host=%s port=%d user=%s password=%s dbname=%s sslmode=require", host, port, user, password, database)
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

postgres specific change

@plunkettscott
Copy link

Any idea when this may be reviewed? I just stumbled across this and would really like postgres as a backend.

@@ -0,0 +1,70 @@
CREATE TABLE IF NOT EXISTS instances (
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Any reason NOT EXISTS is used here? In cases like migration (especially creation) being critical path, this may skip creating a table when one already exists with different schema. Reentrancy here has too much of an edge case.

"github.com/golang-migrate/migrate/v4/database/postgres"
"github.com/golang-migrate/migrate/v4/source/iofs"
"github.com/google/uuid"
_ "github.com/lib/pq"
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lib/pq is largely stale at this point, jackc/pgx is actively worked on. it is compatible with database/sql` as well, see https://pkg.go.dev/github.com/jackc/pgx/v5/stdlib

}
defer tx.Rollback()

row := tx.QueryRowContext(ctx, SQLReplacer("SELECT state FROM instances WHERE instance_id = ? AND execution_id = ? LIMIT 1"), instance.InstanceID, instance.ExecutionID)
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@erma07 I think you are fine to use positional parameters when using pgx, e.g.

Suggested change
row := tx.QueryRowContext(ctx, SQLReplacer("SELECT state FROM instances WHERE instance_id = ? AND execution_id = ? LIMIT 1"), instance.InstanceID, instance.ExecutionID)
row := tx.QueryRowContext(
ctx,
"SELECT state FROM instances WHERE instance_id = $1 AND execution_id = $2 LIMIT 1",
instance.InstanceID,
instance.ExecutionID,
)

@emmanuelay
Copy link

Is this being worked on?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants