[WIP] - v4-beta.1 #507

mfridman · 2023-04-25T12:18:08Z

This is a continuation of #502

The goal of these branches is to continue polishing up the /v4 release, making sure the packeg/CLI is correct, idiomatic, has good test coverage and supports the main features.

There will be breaking changes along the alpha branches, less in the beta, and little in the release candidates.

Note, to actually ship this chunk of work and the documentation, blogs, etc. I'm going to start cutting scope for items that can be added post-launch, such as the grouped migrations feature, JSON support and a few other nice-to-have (but not critical) items.

beta.N

Revisit method-specific options vs provider options, see comment
Add mutex to *goose.Provider (maybe?)
Add back mysql-specific -certfile, -ssl-cert, and -ssl-key
Fix CLI output

rc.N

Add goose validate command

Post stable v4

Add -json support
Add support for Grouped Migrations feature

mfridman · 2023-04-29T14:27:15Z

To actually ship something, I'm going to cut scope and ship a /v4 with items that can be added in subsequent point releases.

mfridman · 2023-05-05T13:42:21Z

A thought, instead of forcing users to exclude files, what if goose was opinionated in the files it parsed. Something I've been thinking about from this #331 (comment)

For example, goose will scan for .sql and .go (not including _test.go) files in the given directory and exclude all other files.

This would allow folks to have a ./migrations folder with ./scripts, a README.md file and all sorts of other directories and files.

Not sure, just a thought ..

ori-shalom · 2023-05-08T13:52:39Z

A thought, instead of forcing users to exclude files, what if goose was opinionated in the files it parsed. Something I've been thinking about from this #331 (comment)

For example, goose will scan for .sql and .go (not including _test.go) files in the given directory and exclude all other files.

A use case for having other go files that are not tests or migrations in the same package is if we have multiple go migrations and we want them to use some shared code.

This shared code ideally will be private functions/constants in the same package because they are relevant only for migrations and there no point in putting them in another package and makes them public.

ori-shalom · 2023-05-08T13:57:03Z

If we are OK with introducing a breaking change in v4 then maybe #513 is a good candidate to make the package more idiomatic.

I currently implemented it here #517 in a backward-compatible way but maybe some code can be simplified if we drop the old methods that don't receive context.

mfridman · 2023-05-08T14:01:31Z

If we are OK with introducing a breaking change in v4 then maybe #513 is a good candidate to make the package more idiomatic.

I currently implemented it here #517 in a backward-compatible way but maybe some code can be simplified if we drop the old methods that don't receive context.

The /v4 module already has context threaded through the entire package and all (or at least most) methods take a ctx context.Context. There are a few breaking changes, but afaics it's the only way to evolve this package.

Fwiw there was already a PR to accomplish this. #195. I think it makes sense to add one of these PR's into the /v3 module.

mfridman · 2023-05-19T22:49:57Z

Alright, here's the *goose.Provider methods so far. The checkmark indicates which methods are safe to call concurrently and a test has been added in 6e2416e

✔ func (p *Provider) ApplyVersion
✔ func (p *Provider) Down
✔ func (p *Provider) DownTo
✔ func (p *Provider) GetDBVersion
  func (p *Provider) ListSources
✔ func (p *Provider) Ping
  func (p *Provider) Redo
✔ func (p *Provider) Status
✔ func (p *Provider) Up
✔ func (p *Provider) UpByOne
✔ func (p *Provider) UpTo

roblaszczak

Hey @mfridman!

Great job with this PR! I have a use case when I need to run two types of migrations within one application. They should be versioned separately and executed independently. It was not possible in v3 (due to the global state), but with v4, it seems to work after initial investigation.

As it's beta, I wanted to ensure at which stage the update is. During that, I found two things that may be worth considering before release. If something is not clear, please let me know!

roblaszczak · 2023-07-18T09:25:44Z

run.go

+	case LockModeAdvisorySession:
+		if err := p.store.LockSession(ctx, conn); err != nil {
+			p.mu.Unlock()
+			return nil, nil, err
+		}
+		cleanup = func() error {
+			defer p.mu.Unlock()
+			return errors.Join(p.store.UnlockSession(ctx, conn), conn.Close())
+		}


@mfridman, please correct me if I'm wrong, but I am right with such a scenario:

I assume that I'm running application from the level of the application. During application start, I'm running Provider.Up()

We have our application in version 1 with all migrations applied

We are deploying an application with version 2 and long-running migration

I assume that the application in version 2 is not healthy yet (so deploy is not considered as done)

During the execution of the long-running migration, old instances of service in version 1 are killed and are re-created (in version 1)

Service in version 1 is not able to start because lock is acquired here

💥 our application is down because new instances are not able to start and old are dead

What I would expect:

We have our application in version 1 with all migrations applied

We are deploying an application with version 2 and long-running migration

I assume that the application in version 2 is not healthy yet (so deploy is not considered as done)

During the execution of the long-running migration, old instances of service in version 1 are killed and are re-created (in version 1)

Service in version 1 checks that all migrations were applied and doesn't acquire the lock and starts properly

New instances of application in version 1 start properly

Long-running migration of version 2 finish

Application instances in version 1 are removed

🎉

I wasn't analyzing it very deeply, but maybe the logic here can stay as is, but maybe we could check if all migrations were applied outside of trasnsaction/lock safely. If migrations are up to date, there is no point of acquiring lock.

If versions are not up to date, we can query versions again (as we do now), but now within transaction and lock.

As a workaround, I could create two providers with almost identical config, but with different lock settings.
In that case, I can at the startup check migration status without lock, and run Provider.Up only if they were not applied migrations. But it's a question if it shouldn't work like that out of the box.

You're spot on, that's exactly how it'd work. app 2 would acquire the lock, and if app 1 is restarted (for any reason) it would fail to come up because the lock is held by the (not-yet-ready) app 2.

By status, do you mean comparing against the last applied migration? I think this would work for sequential migrations, but we'd also have to account for out-of-order migrations.

A crude way would be to list all db versions and compared them against the migration files. If there's 100% parity then we can exit early. I believe this would work for both sequential and out-of-order migrations.

There's an edge case here, if app 2 has two migrations to apply, a fast one and a long-running one, then app 1 could see a view of the world that is different.

E.g., we have 1,2,3 applied and then app 2 starts running with migrations 4, 5.

migration 4 gets applied immediately.

migration 5 is long-running

app 1 is restarted, it sees 1,2,3,4 in the goose db table.. but it only knows about 1,2,3 (migrations from filesystem)

should app 1 exit with no error here?

Thanks again for the detailed report, I went ahead and implemented this in #751 and it's natively baked into the *goose.Provider now, which exposes both the .HasPending() method and also a pre-check in the .Up set of methods.

roblaszczak · 2023-07-18T09:33:21Z

internal/dialectadapter/locker.go

+	// string "goose". This is used to ensure that the lock is unique to goose.
+	//
+	// crc64.Checksum([]byte("goose"), crc64.MakeTable(crc64.ECMA))
+	defaultLockID int64 = 5887940537704921958


It would be awesome to have the ability to configure that. It would allow to use goose in scenarios where:

Two applications are using the same database (doesn't sound unlikely)

You want to run two different migrations in parallel (it's the use case that I may have).

roblaszczak · 2023-07-18T10:23:24Z

internal/dialectadapter/locker.go

+	default:
+		return ErrLockNotImplemented
+	}
+	return retry.Do(ctx, s.retryLock, fn)


It would be nice to have the ability to decide if we want to retry (in other words - if migration can't just return an error when migrations are locked) or how many times we want to retry.

For now, we can just cancel context, but we don't know if we are stuck on mock or in the mgiration.

roblaszczak · 2023-07-18T10:25:28Z

internal/dialectadapter/locker.go

+//
+// The only database that currently supports locking is Postgres. Other databases will return
+// ErrLockNotImplemented.
+type Locker interface {


Any plans to allow to use user-provided Locker?

I can imagine that someone may want to implement a custom one (for example Redis based) locker.

mfridman · 2023-07-18T13:42:23Z

Hey @mfridman!

Great job with this PR! I have a use case when I need to run two types of migrations within one application. They should be versioned separately and executed independently. It was not possible in v3 (due to the global state), but with v4, it seems to work after initial investigation.

As it's beta, I wanted to ensure at which stage the update is. During that, I found two things that may be worth considering before release. If something is not clear, please let me know!

Hey @roblaszczak, thank you for the feedback. Indeed one of the motivations is to enable multiple providers within the same runtime and eliminate ~~as much~~ all global state.

Realistically, the work on the /v4 branch is ~85% (rough ballpark). There are still a few things left, and I plan to publish a series of posts on why we're creating a new major release and what are the changes and future steps. On that last point, I think getting this project into a stable state will allow us to add more interesting user features while hopefully keeping the tool simple for the default case.

https://pressly.github.io/goose/blog/

Over the next few days/weeks, I'll address the comments and if you have additional feedback regarding the implementation or even the user-facing API surface do share your thoughts.

ps. My goal was to have a stable release candidate by the end of the summer, so we're looking at a good few months.

roblaszczak · 2023-07-25T14:04:22Z

Hey @mfridman! I spent some time looking at how it could be possible to improve the Locker configuration (so it's possible to change the lock ID and provide a custom locker).

If you feel that it may be useful, feel free to accept the PR (or modify it in any way): #575

mfridman · 2023-11-12T17:50:38Z

Closing this PR in favor of the feature that landed in a backwards-compatible way in the /v3 module in v3.16.0.

Check out the blog post which has a bunch more details:

https://pressly.github.io/goose/blog/2023/goose-provider/

mfridman mentioned this pull request Apr 25, 2023

[WIP] - v4-alpha.1 #502

Closed

12 tasks

mfridman force-pushed the v4-beta.1 branch 2 times, most recently from 3bb36e5 to 6ebf925 Compare May 5, 2023 13:18

mfridman force-pushed the v4-beta.1 branch from 4c8c889 to 11f9848 Compare May 10, 2023 13:26

mfridman force-pushed the v4-beta.1 branch from a368c43 to a36956d Compare May 25, 2023 00:33

mfridman added 2 commits May 28, 2023 15:59

continue - v4-beta.1

1d5cc23

Improve comment

eb00ac2

mfridman force-pushed the v4-beta.1 branch from a36956d to eb00ac2 Compare May 28, 2023 20:00

mfridman mentioned this pull request Jun 29, 2023

Refactored code to have a Provider. #360

Closed

mfridman added the /v4 label Jul 5, 2023

mfridman added 3 commits July 5, 2023 09:44

remove logger from lib

4da8fb6

start README.md

7452d93

fix

3d10b39

roblaszczak reviewed Jul 18, 2023

View reviewed changes

mfridman mentioned this pull request Aug 23, 2023

Goose isn't thread safe? 🤔 #587

Closed

mfridman mentioned this pull request Sep 14, 2023

Add goose provider with unimplemented methods #596

Merged

oliverpool mentioned this pull request Sep 19, 2023

Adding a goose provider #379

Closed

This was referenced Oct 9, 2023

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

Merged

Allow custom locker implementations #575

Closed

mfridman mentioned this pull request Nov 11, 2023

Avoid acquiring lock if no pending migrations #640

Closed

mfridman closed this Nov 12, 2023

mfridman mentioned this pull request Apr 20, 2024

Add provider HasPending method #751

Merged

BrewTestBot mentioned this pull request Apr 22, 2024

goose 3.20.0 Homebrew/homebrew-core#169758

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[WIP] - v4-beta.1 #507

[WIP] - v4-beta.1 #507

mfridman commented Apr 25, 2023 •

edited

mfridman commented Apr 29, 2023

mfridman commented May 5, 2023

ori-shalom commented May 8, 2023

ori-shalom commented May 8, 2023 •

edited

mfridman commented May 8, 2023 •

edited

mfridman commented May 19, 2023 •

edited

roblaszczak left a comment

roblaszczak Jul 18, 2023

mfridman Jul 19, 2023 •

edited

mfridman Apr 21, 2024 •

edited

roblaszczak Jul 18, 2023

roblaszczak Jul 18, 2023

roblaszczak Jul 18, 2023

mfridman commented Jul 18, 2023 •

edited

roblaszczak commented Jul 25, 2023

mfridman commented Nov 12, 2023

[WIP] - v4-beta.1 #507

[WIP] - v4-beta.1 #507

Conversation

mfridman commented Apr 25, 2023 • edited

mfridman commented Apr 29, 2023

mfridman commented May 5, 2023

ori-shalom commented May 8, 2023

ori-shalom commented May 8, 2023 • edited

mfridman commented May 8, 2023 • edited

mfridman commented May 19, 2023 • edited

roblaszczak left a comment

Choose a reason for hiding this comment

roblaszczak Jul 18, 2023

Choose a reason for hiding this comment

mfridman Jul 19, 2023 • edited

Choose a reason for hiding this comment

mfridman Apr 21, 2024 • edited

Choose a reason for hiding this comment

roblaszczak Jul 18, 2023

Choose a reason for hiding this comment

roblaszczak Jul 18, 2023

Choose a reason for hiding this comment

roblaszczak Jul 18, 2023

Choose a reason for hiding this comment

mfridman commented Jul 18, 2023 • edited

roblaszczak commented Jul 25, 2023

mfridman commented Nov 12, 2023

mfridman commented Apr 25, 2023 •

edited

ori-shalom commented May 8, 2023 •

edited

mfridman commented May 8, 2023 •

edited

mfridman commented May 19, 2023 •

edited

mfridman Jul 19, 2023 •

edited

mfridman Apr 21, 2024 •

edited

mfridman commented Jul 18, 2023 •

edited