Repository navigation
used uber atomic bool instead standard in lock/unlock db - #580
Conversation
Pull Request Test Coverage Report for Build 466
💛 - Coveralls |
dhui
left a comment
dhui
left a comment
There was a problem hiding this comment.
Thanks for the PR! 🙏
I've been meaning to make this change for a while but haven't gotten around to it.
| func (ch *ClickHouse) Unlock() error { | ||
| if !ch.isLocked.CAS(true, false) { | ||
| return database.ErrLocked | ||
| return database.ErrNotLocked |
There was a problem hiding this comment.
yeah, this is my bug))
| // https://www.postgresql.org/docs/9.6/static/explicit-locking.html#ADVISORY-LOCKS | ||
| func (p *Postgres) Lock() error { | ||
| if p.isLocked { | ||
| if p.isLocked.Load() { |
There was a problem hiding this comment.
We should replace this with CAS, release the local if the postgres lock fails, and remove the CAS call below for a proper local lock implementation
There was a problem hiding this comment.
A lot of thanks for review. fixed it
| // https://www.postgresql.org/docs/9.6/static/explicit-locking.html#ADVISORY-LOCKS | ||
| func (p *Postgres) Lock() error { | ||
| if p.isLocked { | ||
| if p.isLocked.Load() { |
There was a problem hiding this comment.
Same here about fixing the local lock optimization implementation
| // Lock creates an advisory local on the database to prevent multiple migrations from running at the same time. | ||
| func (ss *SQLServer) Lock() error { | ||
| if ss.isLocked { | ||
| if ss.isLocked.Load() { |
There was a problem hiding this comment.
Same here about fixing the local lock optimization implementation
| // Unlock froms the migration lock from the database | ||
| func (ss *SQLServer) Unlock() error { | ||
| if !ss.isLocked { | ||
| if !ss.isLocked.Load() { |
There was a problem hiding this comment.
Same here about fixing the local lock optimization implementation
| } | ||
| return nil | ||
| if !success { | ||
| m.isLocked.Store(false) |
There was a problem hiding this comment.
Add a comment here about how removing the local lock in this situation is desired since the local lock is an optimization and it's not trying to reflect the state of the db lock.
Actually, let's add our own CAS wrapper to automatically restore the lock state on error (in database/util.go) so we don't need to remember to do it:
func casRestoreOnErr(lock *atomic.Bool, o, n bool, f func() error) error {
if !lock.CAS(o, n) {
return ErrLocked
}
if err := f(); err != nil {
// Automatically unlock
lock.Store(o)
return err
}
return nil
}Also, can you add tests for this wrapper?
There was a problem hiding this comment.
Thank you. Of course I will do it
dhui
left a comment
dhui
left a comment
There was a problem hiding this comment.
Thanks again for addressing my feedback! Just a few minor fixes left!
dhui
left a comment
dhui
left a comment
There was a problem hiding this comment.
Thanks for addressing my feedback!
Sorry for the delay, I must have missed a notification that there were more commits to review...
| } | ||
|
|
||
| return database.CasRestoreOnErr(&m.isLocked, false, true, database.ErrLocked, func() error { | ||
| if !m.config.Locking.Enabled { |
There was a problem hiding this comment.
I think we can always try to acquire the local lock before acquiring the db lock even if locking is disabled. IIRC, the locking enabled config was added for backwards compatibility and to avoid an extra network hop for existing clients
dhui
left a comment
dhui
left a comment
There was a problem hiding this comment.
Thanks again for the PR and addressing all of the feedback!
3 participants
No description provided.