search

jackc / tern

The SQL Fan's Migrator
MIT License
925 stars 68 forks source link

#4 add implicit transactions to the Migrator #5

Closed hsyed closed 7 years ago

jackc commented 7 years ago

I think this is a reasonable idea, but there are a few implementation details that need to be worked out.

First, the tests are failing (at least on Go 1.8rc3). [1]

Second, I think the API could be improved. NewMigratorNoTx works when there is only one optional behavior for Migrator, but if we were to add another option then we need to add two more functions NewMigratorOtherOption and NewMigratorNoTxOtherOption. I think a NewMigratorEx(conn *pgx.Conn, versionTable string, options *MigratorOptions) would be a better API. Then any other future options could simple be added to MigratorOptions and the API would remain unchanged. For MigratorOptions we would want the zero value to be default behaviour of internal transactions, so perhaps the option could be called DisableTx.

On a broader note, to answer your question in #4, Conn.TxStatus could be used to detect if a transaction is in-progress. But I think I prefer the explicit direction you've gone here. OTOH, maybe it would be cool to detect an in-progress transaction and use savepoints to to atomically migrate versions while leaving the overall Tx commit up to the external code. Might be too clever and overcomplicate things though.

[1]

jack@edi:~/dev/go/src/github.com/jackc/tern$ go test ./...
--- FAIL: TestMigrate (0.08s)
    tern_test.go:100: tern failed with: exit status 2
        output:
        2017-02-11 11:17:22 executing 001_create_t1.sql up
        create table t1(
          id serial primary key,
          placebo varchar not null
        );

        panic: runtime error: invalid memory address or nil pointer dereference
        [signal SIGSEGV: segmentation violation code=0x1 addr=0x20 pc=0x63c7b6]

        goroutine 1 [running]:
        github.com/jackc/tern/vendor/github.com/jackc/pgx.(*Tx).Commit(0x0, 0xc4200e6120, 0x24)
            /home/jack/dev/go/src/github.com/jackc/tern/vendor/github.com/jackc/pgx/tx.go:78 +0x26
        github.com/jackc/tern/migrate.(*Migrator).MigrateTo(0xc42001ca00, 0xc400000002, 0x0, 0x0)
            /home/jack/dev/go/src/github.com/jackc/tern/migrate/migrate.go:292 +0x6d3
        github.com/jackc/tern/migrate.(*Migrator).Migrate(0xc42001ca00, 0x7fff00000000, 0x0)
            /home/jack/dev/go/src/github.com/jackc/tern/migrate/migrate.go:204 +0x33
        main.Migrate(0xc4200aa800, 0xc4200169c0, 0x0, 0x4)
            /home/jack/dev/go/src/github.com/jackc/tern/main.go:391 +0xcd4
        github.com/jackc/tern/vendor/github.com/spf13/cobra.(*Command).execute(0xc4200aa800, 0xc420016980, 0x4, 0x4, 0xc4200aa800, 0xc420016980)
            /home/jack/dev/go/src/github.com/jackc/tern/vendor/github.com/spf13/cobra/command.go:569 +0x203
        github.com/jackc/tern/vendor/github.com/spf13/cobra.(*Command).ExecuteC(0xc4200ab000, 0xc420055f58, 0x75d601, 0xc4200ab000)
            /home/jack/dev/go/src/github.com/jackc/tern/vendor/github.com/spf13/cobra/command.go:656 +0x3a4
        github.com/jackc/tern/vendor/github.com/spf13/cobra.(*Command).Execute(0xc4200ab000, 0xc420055f50, 0x1)
            /home/jack/dev/go/src/github.com/jackc/tern/vendor/github.com/spf13/cobra/command.go:615 +0x2b
        main.main()
            /home/jack/dev/go/src/github.com/jackc/tern/main.go:228 +0x611
--- FAIL: TestStatus (0.05s)
    tern_test.go:100: tern failed with: exit status 2
        output:
        2017-02-11 11:17:23 executing 001_create_t1.sql up
        create table t1(
          id serial primary key,
          placebo varchar not null
        );

        panic: runtime error: invalid memory address or nil pointer dereference
        [signal SIGSEGV: segmentation violation code=0x1 addr=0x20 pc=0x63c7b6]

        goroutine 1 [running]:
        github.com/jackc/tern/vendor/github.com/jackc/pgx.(*Tx).Commit(0x0, 0xc420106120, 0x24)
            /home/jack/dev/go/src/github.com/jackc/tern/vendor/github.com/jackc/pgx/tx.go:78 +0x26
        github.com/jackc/tern/migrate.(*Migrator).MigrateTo(0xc4200ae8c0, 0xc400000002, 0x0, 0x0)
            /home/jack/dev/go/src/github.com/jackc/tern/migrate/migrate.go:292 +0x6d3
        github.com/jackc/tern/migrate.(*Migrator).Migrate(0xc4200ae8c0, 0x7ffd00000000, 0x0)
            /home/jack/dev/go/src/github.com/jackc/tern/migrate/migrate.go:204 +0x33
        main.Migrate(0xc4200c4800, 0xc4200807c0, 0x0, 0x4)
            /home/jack/dev/go/src/github.com/jackc/tern/main.go:391 +0xcd4
        github.com/jackc/tern/vendor/github.com/spf13/cobra.(*Command).execute(0xc4200c4800, 0xc420080780, 0x4, 0x4, 0xc4200c4800, 0xc420080780)
            /home/jack/dev/go/src/github.com/jackc/tern/vendor/github.com/spf13/cobra/command.go:569 +0x203
        github.com/jackc/tern/vendor/github.com/spf13/cobra.(*Command).ExecuteC(0xc4200c5000, 0xc420055f58, 0x75d601, 0xc4200c5000)
            /home/jack/dev/go/src/github.com/jackc/tern/vendor/github.com/spf13/cobra/command.go:656 +0x3a4
        github.com/jackc/tern/vendor/github.com/spf13/cobra.(*Command).Execute(0xc4200c5000, 0xc420055f50, 0x1)
            /home/jack/dev/go/src/github.com/jackc/tern/vendor/github.com/spf13/cobra/command.go:615 +0x2b
        main.main()
            /home/jack/dev/go/src/github.com/jackc/tern/main.go:228 +0x611
FAIL
FAIL    github.com/jackc/tern   1.030s

----------------------------------------------------------------------
PANIC: migrate_test.go:160: MigrateSuite.TestMigrate

... Panic: runtime error: invalid memory address or nil pointer dereference (PC=0x458038)

/usr/local/go/src/runtime/asm_amd64.s:514
  in call32
/usr/local/go/src/runtime/panic.go:489
  in gopanic
/usr/local/go/src/runtime/panic.go:63
  in panicmem
/usr/local/go/src/runtime/signal_unix.go:290
  in sigpanic
/home/jack/dev/go/src/github.com/jackc/tern/vendor/github.com/jackc/pgx/tx.go:78
  in Tx.Commit
migrate.go:292
  in Migrator.MigrateTo
migrate.go:204
  in Migrator.Migrate
migrate_test.go:163
  in MigrateSuite.TestMigrate
/usr/local/go/src/runtime/asm_amd64.s:514
  in call32

----------------------------------------------------------------------
PANIC: migrate_test.go:273: MigrateSuite.TestMigrateToIrreversible

... Panic: runtime error: invalid memory address or nil pointer dereference (PC=0x458038)

/usr/local/go/src/runtime/asm_amd64.s:514
  in call32
/usr/local/go/src/runtime/panic.go:489
  in gopanic
/usr/local/go/src/runtime/panic.go:63
  in panicmem
/usr/local/go/src/runtime/signal_unix.go:290
  in sigpanic
/home/jack/dev/go/src/github.com/jackc/tern/vendor/github.com/jackc/pgx/tx.go:78
  in Tx.Commit
migrate.go:292
  in Migrator.MigrateTo
migrate_test.go:277
  in MigrateSuite.TestMigrateToIrreversible
/usr/local/go/src/runtime/asm_amd64.s:514
  in call32

----------------------------------------------------------------------
PANIC: migrate_test.go:169: MigrateSuite.TestMigrateToLifeCycle

... Panic: runtime error: invalid memory address or nil pointer dereference (PC=0x458038)

/usr/local/go/src/runtime/asm_amd64.s:514
  in call32
/usr/local/go/src/runtime/panic.go:489
  in gopanic
/usr/local/go/src/runtime/panic.go:63
  in panicmem
/usr/local/go/src/runtime/signal_unix.go:290
  in sigpanic
/home/jack/dev/go/src/github.com/jackc/tern/vendor/github.com/jackc/pgx/tx.go:78
  in Tx.Commit
migrate.go:292
  in Migrator.MigrateTo
migrate_test.go:186
  in MigrateSuite.TestMigrateToLifeCycle
/usr/local/go/src/runtime/asm_amd64.s:514
  in call32
OOPS: 8 passed, 3 PANICKED
--- FAIL: Test (0.19s)
--- FAIL: Example_OnStartMigrationProgressLogging (0.02s)
panic: runtime error: invalid memory address or nil pointer dereference [recovered]
    panic: runtime error: invalid memory address or nil pointer dereference
[signal SIGSEGV: segmentation violation code=0x1 addr=0x20 pc=0x64d8d6]

goroutine 1 [running]:
testing.runExample.func2(0xed0313e21, 0xc4379129d8, 0x9095e0, 0xc4200e81e0, 0xc42000e018, 0xc4200e7620, 0x791c1e, 0x27, 0x797ef0, 0x78ddc1, ...)
    /usr/local/go/src/testing/example.go:117 +0x359
panic(0x7321c0, 0x8ffe10)
    /usr/local/go/src/runtime/panic.go:489 +0x2cf
github.com/jackc/tern/vendor/github.com/jackc/pgx.(*Tx).Commit(0x0, 0xc4201139b0, 0x24)
    /home/jack/dev/go/src/github.com/jackc/tern/vendor/github.com/jackc/pgx/tx.go:78 +0x26
github.com/jackc/tern/migrate.(*Migrator).MigrateTo(0xc42010eb90, 0x1, 0x0, 0x0)
    /home/jack/dev/go/src/github.com/jackc/tern/migrate/migrate.go:292 +0x6d3
github.com/jackc/tern/migrate.(*Migrator).Migrate(0xc42010eb90, 0x924898, 0x0)
    /home/jack/dev/go/src/github.com/jackc/tern/migrate/migrate.go:204 +0x33
github.com/jackc/tern/migrate_test.Example_OnStartMigrationProgressLogging()
    /home/jack/dev/go/src/github.com/jackc/tern/migrate/migrate_test.go:310 +0x390
testing.runExample(0x791c1e, 0x27, 0x797ef0, 0x78ddc1, 0x1d, 0x0, 0x0)
    /usr/local/go/src/testing/example.go:122 +0x204
testing.runExamples(0xc420053ed8, 0x9014c0, 0x1, 0x1, 0x1)
    /usr/local/go/src/testing/example.go:46 +0x167
testing.(*M).Run(0xc420053f20, 0xc420053f20)
    /usr/local/go/src/testing/testing.go:823 +0x182
main.main()
    github.com/jackc/tern/migrate/_test/_testmain.go:46 +0xf7
FAIL    github.com/jackc/tern/migrate   0.206s
?       github.com/jackc/tern/vendor/github.com/inconshreveable/mousetrap   [no test files]
?       github.com/jackc/tern/vendor/github.com/jackc/pgx   [no test files]
?       github.com/jackc/tern/vendor/github.com/spf13/cobra [no test files]
?       github.com/jackc/tern/vendor/github.com/spf13/pflag [no test files]
?       github.com/jackc/tern/vendor/github.com/vaughan0/go-ini [no test files]
?       github.com/jackc/tern/vendor/golang.org/x/crypto/curve25519 [no test files]
?       github.com/jackc/tern/vendor/golang.org/x/crypto/ssh    [no test files]
?       github.com/jackc/tern/vendor/golang.org/x/crypto/ssh/agent  [no test files]
hsyed commented 7 years ago

Just starting out with go. Tests should pass now.

I couldn't find proof on how to use the Conn.TxStatus to detect status. I bet I will need to detect transaction status eventually in this way.

I'll squash the commits now.

jackc commented 7 years ago

I've merged in your PR. I added a test and made a few adjustments to make it more idiomatic Go in b1e821d9364b58004ac9a12893d2c64263ca02a2. In particular, I simplified the MigratorOptions into a single struct.

hsyed commented 7 years ago

@jackc Fair enough looks good ! I copied the pattern from type DialOption func(*dialOptions) from the grpc code -- but I think the single struct is clearer.