golang-migrate / migrate

Database migrations. CLI and Golang library.
Other
15.36k stars 1.4k forks source link

SQL Server Driver Attempts to Release Nonexistent Locks #253

Open AlexaDeWit opened 5 years ago

AlexaDeWit commented 5 years ago

Describe the Bug When calling WithInstance (https://github.com/golang-migrate/migrate/blob/master/database/sqlserver/sqlserver.go#L59) to construct the SQL Server driver, sometimes calls to ensureVersionTable (https://github.com/golang-migrate/migrate/blob/master/database/sqlserver/sqlserver.go#L311) result in the lock Release being attempted, but there is no underlying lock.

I suspect there might be a more complex interaction somewhere I haven't yet been able to identify, such as how SQL Server handles locks. But at present it seems that there should be a graceful behaviour where the library accepts that the lock was released on the RDB side, and gracefully return and continue?

I'm not sure exactly what the best approach here is though, and am mostly trying to share what I've been able to determine thusfar.

For context, the MS SQL Server version we are using is the 2017 one provided via the mcr.microsoft.com/mssql/server:2017-latest docker container.

Expected Behavior A clear and concise description of what you expected to happen.

Migrate Version v4.4.0

Loaded Database Drivers github.com/denisenkom/go-mssqldb

Go Version go1.12 darwin/amd64

maxekman commented 5 years ago

@dhui Maybe you have some information or experience with this behavior?

dhui commented 5 years ago

Could you provide more info on the error? e.g.

AlexaDeWit commented 5 years ago

This is the error returned from the call to WithInstance.

 mssql: Cannot release the application lock (Database Principal: 'public', Resource: '1111088596') 
because it is not currently held. in line 0: EXEC sp_releaseapplock @Resource = @p1, @LockOwner = 'Session'

There is no stack trace as no error is thrown.

From what I read, this is the call to Unlock() inside of migrate that generates and executes that query.

This is happening in a test environment, where we are not starting any other goroutines, so as best I understand, it should be single threaded.

AlexaDeWit commented 5 years ago

Actually, golang's testing library might parallelize and that could be the issue. I might have created a useless issue here that's related to false assumptions about tests being atomic.

I'll take a look and see. I'll close the issue if that's all it was.

AlexaDeWit commented 5 years ago

Yeah, sorry, this is on me. I'm going to leave one comment of minor puzzlement, though...

It's perplexing that both locks could be acquired though, in this multi-threaded context.

I was tripped up particularly because the problem was at lock release.... But I would expect that the second thread attempting to grab the lock would actually fail if there was a parallelisation issue.

I'll leave the issue open for the moment because of that oddity, but I recognize that the core initial issue was based on incorrect understanding, and you can close it if you feel it's appropriate.

dhui commented 5 years ago

I'm not familiar with SQLServer. My WAG is that the SQL statement for sp_getapplock is not correct. It's unclear to me what @LockMode (update vs exclusive) and @LockOwner (transaction vs session) we should be using. Also, I'm not sure if we're getting the status of the lock acquisition properly. I'd expect a SELECT statement after the EXEC sp_getapplock statement to be used with sql.Conn.QueryRowContext instead of with sql.Conn.ExecContext().

References:

d10i commented 2 months ago

Sorry for reviving this old issue but I do think this is an actual issue. I created a PR for a fix: https://github.com/golang-migrate/migrate/pull/1123