-
Notifications
You must be signed in to change notification settings - Fork 263
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
Add support for statement timeout #311
Conversation
…is not implemented for this particular database
… call the underlying driver to set the statement timeout
…tation in each driver
This looks very straightforward. Nice tests. |
@@ -361,6 +363,11 @@ func (drv *Driver) Ping() error { | |||
return err | |||
} | |||
|
|||
func (drv *Driver) IncreaseStatementTimeout(db *sql.DB, timeout time.Duration) error { | |||
_, err := db.Exec(fmt.Sprintf("SET statement_timeout = %s", strconv.Itoa(int(timeout.Milliseconds())))) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If this can be changed by a SQL statement, then does it really make sense to implement this specially in dbmate? Why not include the statement in your migration file around the statement that requires the different statement timeout amount?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
because we usually run these in CI/CD and passing a flag to dbmate to take care of timeout is a little bit easier
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are you saying that when you run these outside of CI/CD, changing the statement_timeout
is unnecessary?
Is there any harm in baking the SET statement_timeout
statements into your migrations that need it? I would assume that only specific migrations that are long-running will need it and not every single migration that you're running, so having an app-wide setting seems like the wrong approach, anyway.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The migration is originally written by a developer on their laptop. The statement timeout is an operational characteristic that might be different in a different environment. If the migration is under version control than these operational characteristics would have to be committed ahead of time for what the CI and production needs.
I have an issue where I would like to set timeout for long running migrations. Setting
The |
Context
When running migrations, some statements might take a little bit longer which results in a statement timeout which is usually either a default value or a global value set in some configuration file.
What we want is to be able to modify this default statement timeout according to our needs.
Solution approach
A CLI flag of
--statement-timeout
has been added to provide the user the ability of passing a custom statement timeout.Caveats
Even though modifications are on the interface level, currently an implementation for statement timeout is only provided for PostgresSQL since this is the database I am familiar with and easily have access to.
For other supported databases (clickhouse, mysql, sqlite), the
--statement-timeout
has no effect.In case there's enough interest in this work, I can put some time for the other databases (clickhouse, mysql, sqlite)