Open jaimem88 opened 1 year ago
@rubenv 👋
We would like to hear what you think about the proposal?
cc @joaodrp
So perhaps I'm missing something, but why not just run the whole set of migrations against such an exclusive connection?
That wouldn't require any changes to sql-migrate at all.
@rubenv as far as I understand the current code uses a connection pool to execute each statement separately and not a common connection across statements (always referring to a single migration file):
https://github.com/rubenv/sql-migrate/blob/bc5e259a9623795fe565877f8b711b78a2bc2933/migrate.go#L541
https://github.com/go-gorp/gorp/blob/2db0f5e22596df067c3d4edf9b2f3e0727cc31ca/gorp.go#L195
https://github.com/go-gorp/gorp/blob/2db0f5e22596df067c3d4edf9b2f3e0727cc31ca/db.go#L34
The conditions that the issue describes can be quite frequent, the statement_timeout
for example must be extended in some cases, however each SET
command must be guaranteed to execute in the same connection, otherwise they won't take effect.
The proposed solution sounds to me that would work. If you think there might backwards compatibility issues, perhaps it's best to provide an additional command like notransaction
, e.g. singleconnection
or singlesession
. Let me know if I can provide assistance on the implementation, if you decide that this is a feature that can be added to the project. Thanks for your efforts in maintaining this library!
Context
GitLab's container registry uses
sql-migrate
to manage database migrations. Our database usage has grown to a point where we need to run post-deployment migrations such as creating indexes on partitioned tables. This requires the use ofCONCURRENTLY
in Postgres.We started by creating indexes on just two partitions which worked just fine, with a total runtime of ~30s. We now wanted to execute the post-deployment migrations that will create the remaining partition indexes but it failed due to exceeding the statement timeout limit of 15s that we configure for our Postgres instance.
Problem
We initially thought that we could simply turn off the statement timeout off before executing the statement. However, this won't work for production because with the connection pool on the registry side and PgBouncer's pooling after that, index creation statements and the preceding
SET statement_timeout TO 0
would most likely be executed in different sessions, so the latter wouldn't have an effect on the former.We cannot rely on transactions for these migrations either (as a way to ensure that all statements within would be executed in the same session) because creating an index
CONCURRENTLY
cannot be done within transactions.Solution
The ideal solution in our case, is to simply use an exclusive connection for certain statements rather than relying on the pool abstraction.
At the API level, this could be implemented by adding a new e.g.
ExclusiveConn
parameter to theMigration
struct, similar to DisableTransaction*, which we're already using. If this was set to true on the migration's definition, the lib would grab and use an exclusive connection to execute all its statements.We are happy to make the contribution ourselves, but would like to get a maintainer's opinion on the solution to see if it's something that would be accepted into the project.
Related to https://gitlab.com/gitlab-org/container-registry/-/issues/889.