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

Switch pg backend to session-level advisory locking #20561

Merged
merged 3 commits into from
Mar 5, 2019

Conversation

mars
Copy link
Contributor

@mars mars commented Mar 5, 2019

…to avoid rollback of partial state updates.

Based on discussion following merge of Postgres backend with @jbardin

Acceptance tests are once again passing locally ✅ (updated for commit 34fa67c)

$ TF_ACC=1 GO111MODULE=on go test -v -mod=vendor -timeout=2m -parallel=4 github.com/hashicorp/terraform/backend/remote-state/pg
=== RUN   TestBackend_impl
--- PASS: TestBackend_impl (0.00s)
=== RUN   TestBackendConfig
--- PASS: TestBackendConfig (0.06s)
    backend_test.go:50: TestBackendConfig on *pg.Backend with configs.synthBody{Filename:"<TestWrapConfig>", Values:map[string]cty.Value{"conn_str":cty.StringVal("postgres://localhost/terraform_backend_pg_test?sslmode=disable"), "schema_name":cty.StringVal("terraform_TestBackendConfig")}}
=== RUN   TestBackendStates
--- PASS: TestBackendStates (0.06s)
    backend_test.go:90: TestBackendConfig on *pg.Backend with configs.synthBody{Filename:"<TestWrapConfig>", Values:map[string]cty.Value{"conn_str":cty.StringVal("postgres://localhost/terraform_backend_pg_test?sslmode=disable"), "schema_name":cty.StringVal("terraform_TestBackendStates")}}
=== RUN   TestBackendStateLocks
--- PASS: TestBackendStateLocks (0.07s)
    backend_test.go:113: TestBackendConfig on *pg.Backend with configs.synthBody{Filename:"<TestWrapConfig>", Values:map[string]cty.Value{"schema_name":cty.StringVal("terraform_TestBackendStateLocks"), "conn_str":cty.StringVal("postgres://localhost/terraform_backend_pg_test?sslmode=disable")}}
    backend_test.go:119: TestBackendConfig on *pg.Backend with configs.synthBody{Filename:"<TestWrapConfig>", Values:map[string]cty.Value{"conn_str":cty.StringVal("postgres://localhost/terraform_backend_pg_test?sslmode=disable"), "schema_name":cty.StringVal("terraform_TestBackendStateLocks")}}
    backend_test.go:125: TestBackend: testing state locking for *pg.Backend
=== RUN   TestRemoteClient_impl
--- PASS: TestRemoteClient_impl (0.00s)
=== RUN   TestRemoteClient
--- PASS: TestRemoteClient (0.05s)
    client_test.go:34: TestBackendConfig on *pg.Backend with configs.synthBody{Filename:"<TestWrapConfig>", Values:map[string]cty.Value{"conn_str":cty.StringVal("postgres://localhost/terraform_backend_pg_test?sslmode=disable"), "schema_name":cty.StringVal("terraform_TestRemoteClient")}}
=== RUN   TestRemoteLocks
--- PASS: TestRemoteLocks (0.07s)
    client_test.go:63: TestBackendConfig on *pg.Backend with configs.synthBody{Filename:"<TestWrapConfig>", Values:map[string]cty.Value{"conn_str":cty.StringVal("postgres://localhost/terraform_backend_pg_test?sslmode=disable"), "schema_name":cty.StringVal("terraform_TestRemoteLocks")}}
    client_test.go:69: TestBackendConfig on *pg.Backend with configs.synthBody{Filename:"<TestWrapConfig>", Values:map[string]cty.Value{"conn_str":cty.StringVal("postgres://localhost/terraform_backend_pg_test?sslmode=disable"), "schema_name":cty.StringVal("terraform_TestRemoteLocks")}}
PASS
ok  	github.com/hashicorp/terraform/backend/remote-state/pg	0.453s

Also, have manually exercised terraform apply & destroy with this revised backend ✅

switch {
case err == sql.ErrNoRows:
// When the row does not yet exist in state, take
// the `-1` lock to create the new row.
innerRow := c.txn.QueryRow(`SELECT pg_try_advisory_xact_lock(-1)`)
innerRow := c.Client.QueryRow(`SELECT pg_try_advisory_lock(-1)`)
Copy link
Member

Choose a reason for hiding this comment

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

Just to make sure I understand correctly the reason for the 2 lock attempts; this looks for an individual "workspace" entry with a lock, and if it didn't exist it takes a coarser lock on everything to make sure it's safe to create the workspace?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, that's right.

Although, what you've pointed out made me realize that there was a gap in this implementation. Once locked for creation [-1], after a new workspace's DB row is inserted, another DB session would have been able to lock that now existant row [the row's id], before the creation lock was unlocked.

I just pushed another rev that prevents taking a workspace-specific lock if already locked for creation.

Copy link
Member

Choose a reason for hiding this comment

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

Thanks for the clarification, this makes sense now, especially with the latest update.

@jbardin jbardin merged commit 81c94f7 into hashicorp:master Mar 5, 2019
mars added a commit to mars/terraform that referenced this pull request Apr 8, 2019
pselle pushed a commit to mars/terraform that referenced this pull request Apr 18, 2019
pselle added a commit that referenced this pull request Apr 18, 2019
@ghost
Copy link

ghost commented Mar 29, 2020

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.

@ghost ghost locked and limited conversation to collaborators Mar 29, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants