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.

Sign up for GitHub

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

Jump to bottom

postgres: Make ensureVersionTable atomic #150

Merged
merged 13 commits into from
Jan 15, 2019
Merged

postgres: Make ensureVersionTable atomic #150

merged 13 commits into from
Jan 15, 2019

Conversation

tsenart
Copy link
Contributor

@tsenart tsenart commented Jan 11, 2019

Fixes #55. Superseeds #149.

@tsenart tsenart mentioned this pull request Jan 11, 2019
Merged
@coveralls
Copy link

Pull Request Test Coverage Report for Build 268

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 59.188%
Totals Coverage Status
Change from base Build 264: 0.0%
Covered Lines: 773
Relevant Lines: 1306
💛 - Coveralls

@coveralls
Copy link

coveralls commented Jan 11, 2019 •
edited
Loading

Pull Request Test Coverage Report for Build 283

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 59.188%
Totals Coverage Status
Change from base Build 279: 0.0%
Covered Lines: 773
Relevant Lines: 1306
💛 - Coveralls

@tsenart tsenart closed this Jan 11, 2019
@tsenart tsenart reopened this Jan 11, 2019
@tsenart
Copy link
Contributor Author

tsenart commented Jan 11, 2019

Closed and reopened to trigger CI again which had a flake in the previous run.

dhui
dhui requested changes Jan 12, 2019
View reviewed changes
Copy link
Member

@dhui dhui left a comment

Choose a reason for hiding this comment

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

Thanks for investigating and fixing!

I think this approach is fine for now. Specifically, for postgres, we don't need to worry about the lock stomping over itself since we're using session level advisory locks which are reentrant.

There's the larger issue where all driver operations should be atomic for all drivers. Locks may not be reentrant for other db drivers. I'll create a separate issue for this.

database/postgres/postgres_test.go Outdated Show resolved Hide resolved
database/postgres/postgres.go Outdated Show resolved Hide resolved
@dhui dhui mentioned this pull request Jan 12, 2019
Open
1 task
@tsenart
Copy link
Contributor Author

tsenart commented Jan 12, 2019

@dhui: Please take another look!

dhui
dhui requested changes Jan 12, 2019
View reviewed changes
Copy link
Member

@dhui dhui left a comment

Choose a reason for hiding this comment

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

Thanks for addressing the feedback!

database/postgres/postgres.go Outdated Show resolved Hide resolved
database/postgres/postgres.go Outdated Show resolved Hide resolved
@tsenart tsenart force-pushed the atomic-migrations-table-creation branch from 4793961 to 8323c1c Compare January 13, 2019 11:47
@tsenart
Copy link
Contributor Author

tsenart commented Jan 13, 2019

@dhui: Please take another look :-)

dhui
dhui requested changes Jan 14, 2019
View reviewed changes
database/postgres/postgres.go Outdated Show resolved Hide resolved
Tomás Senart added 12 commits January 15, 2019 11:28
postgres: TestWithInstance_Concurrent
a06a92d
fixup! Pass config to WithInstance
ce7a223
postgres: Make ensureVersionTable atomic
519dae2 
Fixes golang-migrate#55
postgres: Use sync.WaitGroup in TestWithInstance_Concurrent
15713e6
postgres: Preserve Unlock error when outer err isn't nil
09a1959
postgres: Add comment about connection pool in TestWithInstance_Concu…
5bf80d7 
…rrent
postgres: Query table existance in ensureVersionTable
94f5aa0 
This commit reverts back to querying the existance of the table instead
of using CREATE IF NOT EXISTS because we want to support versions of
Postgres older than 9.1 which is when this feature was introduced.
postgres: Be explicit about max idle and open conns
0beddcd
postgres: Use multierror.Error in ensureVersionTable
06c80ae
postgres: Revert back to use CREATE IF NOT EXISTS
b57dbce
fixup! More explicit conditionals
f58da1d
fixup! Add github.com/hashicorp/go-multierror@v1.0.0 to Gopkg.yaml
41a595f
@tsenart tsenart force-pushed the atomic-migrations-table-creation branch from a112283 to 41a595f Compare January 15, 2019 10:29
@tsenart
Copy link
Contributor Author

tsenart commented Jan 15, 2019

@dhui: I added the dependency manually to Gopkg.toml but that didn't seem to work. Running dep ensure -add github.com/hashicorp/go-multierror@v1.0.0 leads to errors: https://gist.github.com/tsenart/bad59caf9a41e55ea8347c43d2c11719

Not sure how to proceed. Please help :-)

@dhui
Run dep ensure
5ad19c5 
    - Updates Gopkg.lock after github.com/hashicorp/go-multierror package was manually added to Gopkg.toml
@dhui
Copy link
Member

dhui commented Jan 15, 2019

Thanks for adding the dependency via dep.

I added the dependency manually to Gopkg.toml

I pushed a new commit that updates Gopkg.lock to your repo branch. Once tests pass, I'll merge.

@dhui dhui merged commit f6d624c into golang-migrate:master Jan 15, 2019
@dhui
Copy link
Member

dhui commented Jan 15, 2019

Will cut a new release in ~1 week

@dhui dhui mentioned this pull request Jan 22, 2019
Closed
@vlymar vlymar mentioned this pull request Jan 23, 2019
Closed
@dhui dhui mentioned this pull request Feb 17, 2019
Merged
@renovate renovate bot mentioned this pull request May 29, 2021
Closed
1 task
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dhui dhui dhui approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@tsenart @coveralls @dhui