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

Drop connections before drop database #14

Merged
merged 9 commits into from
Dec 27, 2020
Merged

Drop connections before drop database #14

merged 9 commits into from
Dec 27, 2020

Conversation

p4cket
Copy link
Contributor

@p4cket p4cket commented Dec 6, 2020

Hey,
when you try to drop database in usage psql will error with message: "pq: cannot drop the currently open database
" or similar.

For psql 13+ there is new option for drop force https://www.postgresql.org/docs/13/sql-dropdatabase.html
for older we need kill connections manually.

@cyrilgdn cyrilgdn self-requested a review December 12, 2020 18:00
Copy link
Owner

@cyrilgdn cyrilgdn 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 this PR @p4cket , TIL the new WITH FORCE option 😊 👍

As I said in comment, it should be an option (new force_drop field in postgresql_database for example). Otherwise it can be risky to drop a database if there's still clients using it.

postgresql/resource_postgresql_database.go Show resolved Hide resolved
postgresql/resource_postgresql_database.go Outdated Show resolved Hide resolved
@cyrilgdn
Copy link
Owner

Don't know why Travis didn't run 🤔

Co-authored-by: Cyril Gaudin <cyril.gaudin@gmail.com>
Copy link
Owner

@cyrilgdn cyrilgdn left a comment

Choose a reason for hiding this comment

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

so do you thikn @cyrilgdn we can keep it with enforcing as terraform by default is giving us lifecycle ?

@p4cket Sorry I thought I already answered to you 🤦‍♂️

Yes, I asked for a third party advice on this discussion and you're right, we can force drop by default.

I'll just put a warning in a the CHANGELOG.

I allowed myself to fix the SQL errors, I wanted to create a test to assert the with force is working but it's not as easy as I thought to have a connected user on the database when the tests drops it. The existing tests already showed that there were errors in the code though.

If you want to have a look at my fixes and review it?

@cyrilgdn cyrilgdn merged commit f5da1e2 into cyrilgdn:master Dec 27, 2020
cyrilgdn added a commit that referenced this pull request Jan 2, 2021
FEATURES:

* `postgresql_database`: Drop connections before drop database (Postgresql >=13) - @p4cket
  [#14](#14)

  ⚠️ In previous versions, Terraform failed to drop databases if they are still in used.
            Now databases will be dropped in any case.

* Use lazy connections - @cyrilgdn
  [#5](#5)
@cyrilgdn
Copy link
Owner

cyrilgdn commented Jan 2, 2021

@p4cket FYI, this has just been released in v1.10.0

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.

2 participants