Postgres backend

[mars]

Implements the Postgres relational database as a Terraform backend with support for locking and workspaces.

Motivation is to enable running Terraform in a Heroku app with the Heroku Postgres database add-on.

I gleaned most of the design from a combination of the inmem, s3, and consul backends, with a hefty dose of my own reasoning.

[mars]

After fighting with master, I found the v0.11 branch to base this PR on until master (0.12.0-dev) stabilizes.

[mars]

I've now rebased onto master (0.12)

Running tests, it looks like there are a bunch of API/interface changes. Would someone with knowledge of the 0.12 changes, please give me some hints or docs to help figure this out?

$ TF_PG_TEST=true make test TEST=./backend/remote-state/pg TESTARGS='-v'
==> Checking that code complies with gofmt requirements...
go generate ./...
2018/11/07 15:32:56 Generated command/internal_plugin_list.go
go list ./backend/remote-state/pg | xargs -t -n4 go test -v -timeout=2m -parallel=4
go test -v -timeout=2m -parallel=4 github.com/hashicorp/terraform/backend/remote-state/pg
# github.com/hashicorp/terraform/backend/remote-state/pg [github.com/hashicorp/terraform/backend/remote-state/pg.test]
backend/remote-state/pg/backend.go:48:2: cannot use result (type *Backend) as type backend.Backend in return argument:
	*Backend does not implement backend.Backend (missing DeleteWorkspace method)
backend/remote-state/pg/backend_state.go:110:52: cannot use terraform.NewState() (type *terraform.State) as type *states.State in argument to stateMgr.WriteState
backend/remote-state/pg/backend_test.go:30:6: cannot use new(Backend) (type *Backend) as type backend.Backend in assignment:
	*Backend does not implement backend.Backend (missing DeleteWorkspace method)
backend/remote-state/pg/backend_test.go:47:39: cannot use config (type map[string]interface {}) as type hcl.Body in argument to backend.TestBackendConfig:
	map[string]interface {} does not implement hcl.Body (missing Content method)
backend/remote-state/pg/backend_test.go:47:50: impossible type assertion:
	*Backend does not implement backend.Backend (missing DeleteWorkspace method)
backend/remote-state/pg/backend_test.go:92:39: cannot use config (type map[string]interface {}) as type hcl.Body in argument to backend.TestBackendConfig:
	map[string]interface {} does not implement hcl.Body (missing Content method)
backend/remote-state/pg/backend_test.go:92:50: impossible type assertion:
	*Backend does not implement backend.Backend (missing DeleteWorkspace method)
backend/remote-state/pg/backend_test.go:115:39: cannot use config (type map[string]interface {}) as type hcl.Body in argument to backend.TestBackendConfig:
	map[string]interface {} does not implement hcl.Body (missing Content method)
backend/remote-state/pg/backend_test.go:115:50: impossible type assertion:
	*Backend does not implement backend.Backend (missing DeleteWorkspace method)
backend/remote-state/pg/backend_test.go:121:40: cannot use config (type map[string]interface {}) as type hcl.Body in argument to backend.TestBackendConfig:
	map[string]interface {} does not implement hcl.Body (missing Content method)
backend/remote-state/pg/backend_test.go:121:40: too many errors
FAIL	github.com/hashicorp/terraform/backend/remote-state/pg [build failed]
make: *** [test] Error 1

[mars]

With Postgres now setup in TravisCI and testing limited to only this new pg backend, this feature's tests are passing ✅

Of course, I will revert or undo that "Test only the pg backend in CI" commit before eventually merging.

Please let me know what y'all think 😄

[jbardin]

Hi @mars,

Thank you for your patience. We're nearing the completion of the 0.12 work, and I'd like to evaluate merging this into the dev build for inclusion in the 0.12 release.

Could you rebase this and verify that everything is working correctly? I'll can still start in on reviewing this in the meantime.

For these tests, you can use the TF_ACC=1 convention to flag these off as acceptance tests, rather than the custom TF_PG_TEST.

Thanks!

[mars]

Howdy @jbardin 😄

I switched this over to native Postgres locking and removed my special CI test commit that ran just these tests with TEST_ACC=1. I don't see that ACC flag being set in TravisCI, so I'm not sure these tests are really running.

I'll proceed with exercising this new version of the pg backend this week and follow up with any news.

[mars]

I've now exercised this backend locally with Terraform built from master/0.12-dev.

⚠️ If you've used this PR/branch before with a Terraform config, the database should be recreated to allow the new schema to take effect.

dropdb terraform_backend && createdb terraform_backend

Otherwise, this appears to be working great ✅🙌

[jbardin]

Thanks @mars.
Yes, travis doesn't handle the TF_ACC tests at all. Acceptance tests have to be setup individually in another system. While this set did run OK, the travis test suite as a whole is already quite heavy, and we'd rather not continue to add to to it.

[jbardin]

Sorry, one last thing, let's remove the travis changes altogether.
Thanks!

[mars]

🏁

[jbardin]

Oops, looks like there's a test that's not flagged off with TF_ACC

[mars]

👌 thanks for the deep review @jbardin ☺️

[jbardin]

Thanks @mars! lets see if we can test this out in master! 👍

[mars]

Hi again @jbardin 😄

I've been using this backend and reviewing its current implementation. Wondering if the implementation of locking using database transactions is working as Terraform expects.

Currently, after calling Lock() any changes to state that are Put() will be lost if the transaction rolls-back due to a database connection failure, Go panic, or other unexpected error. Essentially, calling Unlock() is required to commit changes.

Do you think this is a problem? If so, I can switch from transactional advisory locking to session advisory locking, so that interrupting errors leave the state's partial updates intact.

[jbardin]

Hi @mars, that's a good observation. Terraform doesn't assume the state storage is transactional in any way. While it currently only writes once at the end of each phase, it could in the future make more incremental writes to the storage, and we would want the last state written preserved.

If we could do that with the session based locks, I think that would be preferable. Thanks!

[mars]

Okay, I'll open a new PR with that change very soon.

[ghost]

I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues.

If you have found a problem that seems similar to this, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.