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

Add support for multi-schema migrations in Postgres #127

Merged
merged 2 commits into from
Nov 6, 2018
Merged

Add support for multi-schema migrations in Postgres #127

merged 2 commits into from
Nov 6, 2018

Conversation

vporoshok
Copy link
Contributor

I've save interface of function GenerateAdvisoryLockId with variadic params. So I've just add test to ensure that lock id depend on this additional parameters.

There is lock conflict on parallel migrations in different postgres
schemas. To avoid this conflicts function GenerateAdvisoryLockId added
variadic params to change lock id with schema name. Schema name taked
with postgres CURRENT_SCHEMA function. Null byte used as separator
between database and schema name, because any other symbol may be used
in both of it.

Closes #118
@coveralls
Copy link

coveralls commented Nov 5, 2018

Pull Request Test Coverage Report for Build 215

  • 4 of 4 (100.0%) changed or added relevant lines in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.07%) to 46.883%

Totals Coverage Status
Change from base Build 210: 0.07%
Covered Lines: 707
Relevant Lines: 1508

💛 - Coveralls

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 the PR!

database/util.go Outdated
func GenerateAdvisoryLockId(databaseName string) (string, error) {
sum := crc32.ChecksumIEEE([]byte(databaseName))
func GenerateAdvisoryLockId(databaseName string, additionalNames ...string) (string, error) {
buf := bytes.NewBufferString(databaseName)
Copy link
Member

Choose a reason for hiding this comment

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

Use strings.Join() instead of bytes.NewBufferString() since it's more readable (less code) and faster in all cases (except when there's one string)

[dhui:benchmark_buf_join]$ cat bench_test.go 
package test

import (
	"bytes"
	"strings"
	"testing"
)

const base = "base"

var strs = [][]string{
	{},
	{"foo"},
	{"foo", "bar"},
	{"foo", "bar", "hello", "world"},
}

func Buf(s string, others ...string) []byte {
	buf := bytes.NewBufferString(s)
	for _, s := range others {
		buf.WriteByte(0)
		buf.WriteString(s)
	}
	return buf.Bytes()
}

func Join(s string, others ...string) []byte {
	others = append(others, s)
	return []byte(strings.Join(others, "."))
}

func BenchmarkBuf(b *testing.B) {
	for _, s := range strs {
		b.Run("", func(b *testing.B) {
			for i := 0; i < b.N; i++ {
				Buf(base, s...)
			}
		})
	}
}

func BenchmarkJoin4(b *testing.B) {
	for _, s := range strs {
		b.Run("", func(b *testing.B) {
			for i := 0; i < b.N; i++ {
				Join(base, s...)
			}
		})
	}
}
[dhui:benchmark_buf_join]$ go test -bench . -benchmem
goos: darwin
goarch: amd64
BenchmarkBuf/#00-8 	20000000	        79.2 ns/op	     120 B/op	       2 allocs/op
BenchmarkBuf/#01-8 	20000000	        92.8 ns/op	     120 B/op	       2 allocs/op
BenchmarkBuf/#02-8 	10000000	       210 ns/op	     152 B/op	       3 allocs/op
BenchmarkBuf/#03-8 	 5000000	       335 ns/op	     200 B/op	       4 allocs/op
BenchmarkJoin4/#00-8         	20000000	        70.4 ns/op	      24 B/op	       2 allocs/op
BenchmarkJoin4/#01-8         	10000000	       141 ns/op	      48 B/op	       3 allocs/op
BenchmarkJoin4/#02-8         	10000000	       177 ns/op	      96 B/op	       3 allocs/op
BenchmarkJoin4/#03-8         	10000000	       228 ns/op	     224 B/op	       4 allocs/op
PASS
ok  	benchmark_buf_join	15.506s
[dhui:benchmark_buf_join]$ 

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Wow! It's a good tip! Thank you!

@@ -7,14 +7,21 @@ import (
func TestGenerateAdvisoryLockId(t *testing.T) {
testcases := []struct {
dbname string
schema string
Copy link
Member

Choose a reason for hiding this comment

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

Use []string and add a test case for multiple strings

"fmt"
"hash/crc32"
)

const advisoryLockIdSalt uint = 1486364155

// GenerateAdvisoryLockId inspired by rails migrations, see https://goo.gl/8o9bCT
func GenerateAdvisoryLockId(databaseName string) (string, error) {
sum := crc32.ChecksumIEEE([]byte(databaseName))
func GenerateAdvisoryLockId(databaseName string, additionalNames ...string) (string, error) {
Copy link
Member

Choose a reason for hiding this comment

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

I like the usage of variadic args!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you!

@@ -35,6 +35,7 @@ var (
type Config struct {
MigrationsTable string
DatabaseName string
SchemaName string
Copy link
Member

Choose a reason for hiding this comment

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

I'm not a fan of the schema name being exported, but you're just following the existing pattern where the database name is exported. Let's leave this as is for now and we'll tackle unnecessarily exported field names in a later backwards incompatible version of migrate.

@dhui dhui merged commit 1df8057 into golang-migrate:master Nov 6, 2018
@vporoshok
Copy link
Contributor Author

Thank you!

@dhui
Copy link
Member

dhui commented Nov 6, 2018

Thanks for reporting and fixing the issue!

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.

3 participants