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

feat(experimental): add internal migrate package and SessionLocker interface #606

Merged
merged 8 commits into from
Oct 9, 2023

Conversation

mfridman
Copy link
Collaborator

@mfridman mfridman commented Oct 9, 2023

This PR stubs out an internal package migrate that:

  • defines a Migration struct which represents either a Go or SQL migration
  • adds Run, RunNoTx, and RunConn methods to run the up or down migration
  • lazily parses SQL migrations if they haven't been parsed

This PR also introduces a package lock that defines a SessionLocker interface, with a single postgres implementation that uses pg_try_advisory_lock. The default behaviour remains the same, but this should unlock functionality to address #335


The intended usage goes something like:

// Create a new provider with a Postgres session locker

locker, err := lock.NewPostgresSessionLocker(
	lock.WithLockTimeout(10 * time.Minute),
)
if err != nil {
	return fmt.Errorf("failed to create locker: %w", err)
}
provider, err := goose.NewProvider(
	goose.DialectPostgres,
	db,
	os.DirFS("path/to/migrations"),
	goose.WithSessionLocker(locker),
)
if err != nil {
	return fmt.Errorf("failed to create goose provider: %w", err)
}

// Application code

resultes, err := provider.Up(ctx)
...

The one downside is the locker implementation is unaware of the dialect, so someone could accidentally call NewPostgresSessionLocker for a non-postgres database.

// SessionLocker is the interface to lock and unlock the database for the duration of a session. The
// session is defined as the duration of a single connection and both methods must be called on the
// same connection.
type SessionLocker interface {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I took the best bits of #575 and ported them into this PR.

Kudos to @roblaszczak for suggesting a common interface so users can bring their own implementation. I think this should suffice? Feedback/suggestions are welcome.

I called this "SessionLocker" with the hopes of one day also adding a "TransactionLocker".

The one gotcha here is supplying a locker that doesn't match up with the correct dialect. I tried to name the functions appropriately, but there's nothing enforcing this right now.

if err := row.Scan(&unlocked); err != nil {
return err
}
if unlocked {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Could argue this is best effort and goose shouldn't fail if a session cannot be unlocked. Need to think about this scenario a bit more.

@mfridman
Copy link
Collaborator Author

mfridman commented Oct 9, 2023

This PR doesn't implement the locking / unlocking logic in goose itself, but when we get to that portion, let's make sure to address the feedback in #507 (comment)


func (l *postgresSessionLocker) SessionLock(ctx context.Context, conn *sql.Conn) error {
return retry.Do(ctx, l.retryLock, func(ctx context.Context) error {
row := conn.QueryRowContext(ctx, "SELECT pg_try_advisory_lock($1)", l.lockID)
Copy link
Collaborator Author

@mfridman mfridman Oct 9, 2023

Choose a reason for hiding this comment

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

It's possible someone may want to opt out of retries and prefer a blocking operation like pg_advisory_lock

If another session already holds a lock on the same resource identifier, this function (pg_advisory_lock) will wait until the resource becomes available.

I suppose with an option like lock.WithBlocking we could extend and add this logic. While keeping the default as pg_try_advisory_lock with retries.

The alternative is to have a completely different implementation or users just bring their own locker.

@mfridman mfridman changed the title feat(experimental): add internal migrate package and postgres session locker feat(experimental): add internal migrate package and SessionLocker interface Oct 9, 2023
@mfridman mfridman merged commit c590380 into master Oct 9, 2023
8 checks passed
@mfridman mfridman deleted the gh-379-impl branch October 9, 2023 20:45
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.

None yet

1 participant