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

Support multi-statement execution for PostgreSQL #495

Merged

Conversation

AnatolyRugalev
Copy link
Contributor

@AnatolyRugalev AnatolyRugalev commented Jan 17, 2021

This PR adds support for multi-statement SQL execution for PostgreSQL driver.

In PostgreSQL running multiple SQL statements in one Exec executes them inside a transaction. Sometimes this behavior is not desirable because some statements can be only run outside of transaction (e.g. CREATE INDEX CONCURRENTLY). If you want to use CREATE INDEX CONCURRENTLY without activating multi-statement mode you have to put such statements in a separate migration files.

  • Provide some tests

Resolves #284, Resolves #492

@AnatolyRugalev AnatolyRugalev marked this pull request as draft January 17, 2021 14:21
@AnatolyRugalev AnatolyRugalev force-pushed the postgresql-support-multistatement branch from 0bd3bd9 to 3a5169a Compare January 18, 2021 09:16
@AnatolyRugalev AnatolyRugalev marked this pull request as ready for review January 18, 2021 09:17
@AnatolyRugalev
Copy link
Contributor Author

Hey @dhui!

Can you please take a look? This is mostly a carbon copy from clickhouse multi-statement implementation. The init code, and constants duplicate each other across the drivers, so probably not a bad idea to extract this into a common place (or even introduce new MultiStatementDriver interface).

Also I'm not sure how the multi-statement parser will split something like this:

INSERT INTO foo (bar)  VALUES (';');

Shall we improve it in a separate issue and PR?

Copy link
Member

@dhui dhui left a comment

Choose a reason for hiding this comment

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

Thanks for the PR!

Also I'm not sure how the multi-statement parser will split something like this:
INSERT INTO foo (bar) VALUES (';');
Shall we improve it in a separate issue and PR?

Agreed that this is an issue and is out of scope for this PR.
How would you work around this issue w/o writing a mini SQL parser? There are other cases with invalid SQL and various escape sequences that will break the simple and naive ; splitting that multistmt does.
migrate intentionally avoids parsing migrations with multistmt being the exception due to the need by various DB drivers and the simplicity of the approach.

FYI, unfortunately, merges are blocked until CI is fixed. See: #252 (comment)
However, if this PR looks good. I'll approve and will merge it once CI is fixed.

@@ -132,10 +140,23 @@ func (p *Postgres) Open(url string) (database.Driver, error) {
}
}

multiStatementMaxSize := DefaultMultiStatementMaxSize
Copy link
Member

Choose a reason for hiding this comment

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

The init code, and constants duplicate each other across the drivers, so probably not a bad idea to extract this into a common place (or even introduce new MultiStatementDriver interface).

Is this what you meant by init code?
What were you thinking in terms of a MultiStatementDriver interface?

database/postgres/postgres.go Outdated Show resolved Hide resolved
database/postgres/postgres_test.go Outdated Show resolved Hide resolved
Copy link
Member

@dhui dhui left a comment

Choose a reason for hiding this comment

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

Thanks for addressing my feedback!

@dhui dhui merged commit a185b9e into golang-migrate:master Feb 3, 2021
@andream16
Copy link

I was hoping that this PR would also address this error I'm getting:

CREATE INDEX CONCURRENTLY cannot be executed from a function or multi-command string in line 0: SET statement_timeout = "24h";

This is the migrations file looks like:

SET statement_timeout = "24h";
CREATE INDEX CONCURRENTLY IF NOT EXISTS my_index ON my_table(my_column);
SET statement_timeout = default;

Any suggestions?

@AnatolyRugalev
Copy link
Contributor Author

@andream16 did you put x-multi-statement=true to the connection string?

@andream16
Copy link

@AnatolyRugalev thanks for the reply.

I'm using migrate.NewWithDatabaseInstance() to get a new pointer to Migrate.

I'm using pq to create the instance and it apparently it doesn't support such parameter in the dsn. The creation fails with: pq: unrecognized configuration parameter "x-multi-statement"

I guess I'll have to try using migrate.New() directly.

@AnatolyRugalev AnatolyRugalev deleted the postgresql-support-multistatement branch February 3, 2021 15:15
@AnatolyRugalev
Copy link
Contributor Author

AnatolyRugalev commented Feb 3, 2021

@andream16 if you have sql.DB ready to use with migrate you can probably do this:

var db *sql.DB
... // I assume db gets initialized here

pgDriver := postgres.WithInstance(db, postgres.Config{
    MultiStatementEnabled: true
})
migrate.NewWithDatabaseInstance(sourceURL, dbName, pgDriver)

@andream16
Copy link

@AnatolyRugalev that works! Thank you 🎉

For other folks having this problem - you can also achieve the same result using migrate.New():

...

import (
    ...
    "github.com/golang-migrate/migrate/v4"
    _ "github.com/golang-migrate/migrate/v4/database/postgres"
    _ "github.com/golang-migrate/migrate/v4/source/file"
)

func NewMigrate(migrationsPath, dsn) (*migrate.Migrate) {
    m, err := migrate.New("file://"+migrationsPath, dsn)
    if err != nil {
        return nil, fmt.Errorf("could not create new migrate: %w", err)
    }
    
    return m, nil
}

...
_, _ = NewMigrate("/migrations", "postgres://user:password@hostname:port/db_name?sslmode=disable&x-multi-statement=true")

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