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

[WIP] - Add exclusive session level advisory lock (postgres only) #483

Closed
wants to merge 10 commits into from

Conversation

mfridman
Copy link
Collaborator

@mfridman mfridman commented Mar 21, 2023

First pass to fix #335

  • postgres only with session level advisory lock
  • transaction level advisory lock not implemented, likely outside the scope of this PR

The default is no lock mode, but lock=session (cli) and goose.WithLock(...) (pkg) are now functional.

EDIT: not overly happy with this implementation because it's awkward to thread the *sql.Conn through the rest of the functions and the *Migration methods. The interface seems fine to start and can be expanded to other databases.

This implementation is functional, but it does mean the max number of connections must be greater than 1. This probably solves the most common use case, but it's not ideal.

The long-term solution is uplifting goose and implementing this properly in a major version alongside #379.


goose CLI:

goose ... -lock=session up

goose package:

type LockMode int

const (
	LockModeNone LockMode = iota
	LockModeAdvisorySession
	LockModeAdvisoryTransaction
)

goose.WithLock(mode LockMode)
  • Need to re-use the connection, otherwise if the *sql.DB was set with db.SetMaxOpenConns(1) it'll hang
  • Add tests
  • Remove the retry logic
  • Remove pg_advisory_unlock value check
  • If adding a provider abstraction, need to add a mutex to guard the lock

@@ -45,6 +68,24 @@ func UpTo(db *sql.DB, dir string, version int64, opts ...OptionsFunc) error {
return err
}

switch option.lockMode {
case LockModeAdvisorySession:
conn, err := db.Conn(ctx)
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This connection should be re-used in the event *sql.DB was configured with a maximum of 1 open connection. Otherwise, this will block indefinitely.

@@ -7,12 +7,15 @@ import (
"fmt"
"sort"
"strings"

"go.uber.org/multierr"
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Will probably remove this package dep, just so happens I'm quite familiar with it.

return retry.RetryableError(err)
}
if !unlocked {
// TODO(mf): provide the user with some documentation on how they can unlock
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Consider removing this check, see code comment.

func (s *store) LockSession(ctx context.Context, conn *sql.Conn) error {
switch t := s.querier.(type) {
case *dialectquery.Postgres:
// TODO(mf): need to be VERY careful about the retry logic here to avoid
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Revisit this retry logic, see code comment.

@cassiodias
Copy link

Hello! Do you have any estimation for this feature?

@mfridman
Copy link
Collaborator Author

mfridman commented Apr 2, 2023

Hello! Do you have any estimation for this feature?

This is implemented in #493 for the /v4 refactor. I'm not sure this PR will be merged into /v3. It was more of an experiment on how this could be achieved.

I'm hoping to tag a v4.0.0-beta.1 in the next few weeks, and then a more table v4.0.0-rc.1 within the next few months.

@mfridman
Copy link
Collaborator Author

I'm not sure adding locking in the module github.com/pressly/goose/v3 is worth it. It leads to some surprising behaviour.

My comments from the description still hold, and I have not spent much time thinking about it further as I've been more occupied with getting things to work in /v4

not overly happy with this implementation because it's awkward to thread the *sql.Conn through the rest of the functions and the *Migration methods.

The long-term solution is uplifting goose and implementing this properly in a major version alongside #379.

Closing this PR in favor of adding this feature to the /v4 module.

@mfridman mfridman closed this Apr 16, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add locking mechanism to prevent parallel execution as a safety measure
2 participants