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

Support force dropping database introduced in PostgreSQL 13 #280

Merged
merged 3 commits into from
Oct 27, 2020

Conversation

ruudk
Copy link
Contributor

@ruudk ruudk commented Oct 26, 2020

I was getting annoyed by:

$ mix ecto.reset
** (Mix) The database for App.Repo couldn't be dropped: ERROR 55006 (object_in_use) database "app_dev" is being accessed by other users

There is 1 other session using the database.

So I searched online and found out that in PostgreSQL 13 there is a new DROP DATABASE "db" WITH (FORCE) syntax.

https://www.postgresql.org/docs/current/sql-dropdatabase.html

Is this something that can be added? The PR is not fully done as I need some advice on how to do this properly 🙏


@impl true
def storage_down(opts) do
database = Keyword.fetch!(opts, :database) || raise ":database is nil in repository configuration"
command = "DROP DATABASE \"#{database}\""
force = case Keyword.fetch(opts, :force_drop) do
Copy link
Member

Choose a reason for hiding this comment

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

Should we simply call this option --force? We also need to change mix ecto.drop to make sure it accepts the --force option. Or are you actually setting this in your repo configuration? 🤔

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was setting it in the Repo config but it's up for debate. I think it makes more sense as param on mix ecto.drop. Will fix.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There is already a --force option on mix ecto.drop:

    * `-f`, `--force` - do not ask for confirmation when dropping the database.
      Configuration is asked only when `:start_permanent` is set to true
      (typically in production)

What do to?

Copy link
Member

Choose a reason for hiding this comment

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

@ruudk I think we can reuse the option but I am not sure. What do you think @wojtekmach?

Copy link
Member

Choose a reason for hiding this comment

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

I assume WITH FORCE is only available on pg 13+ which means it's gonna fail on earlier versions? If so, we need to add another option to not break existing users.

Let's add a --force-drop option to mix ecto.drop and perhaps say that it's only used by adapters supporting it.

Copy link
Member

Choose a reason for hiding this comment

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

Maybe we can write this logic conditionally in Postgres itself, using a single command? Otherwise I would have a separate flag as I would like to avoid doing multiple operations.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I tried this:

               DO $$
               DECLARE pg_version int := current_setting('server_version_num');
               BEGIN
                 IF pg_version >= 130000 THEN
                   DROP DATABASE "#{database}" WITH (FORCE);
                 ELSE
                   DROP DATABASE "#{database}";
                 END IF;
               END $$ LANGUAGE plpgsql;

But it doesn't work because you cannot drop a database inside a transaction.

ERROR 25001 (active_sql_transaction) DROP DATABASE cannot be executed from a function

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, so a separate flag, like --force-drop as you originally proposed is def. the best way to go IMO.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I updated the PR to use force_drop from opts but it turns out that those opts are the repo.config. See https://github.com/elixir-ecto/ecto/blob/e58209ce7af4008d9015c3ff7084a20239103fbd/lib/mix/tasks/ecto.drop.ex#L74

How to proceed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Would this be Ok?

case repo.__adapter__.storage_down(Keyword.merge([force_drop: opts[:force_drop]], repo.config)) do

That would even allow you to set this up in repo.config :)

@ruudk ruudk force-pushed the force_drop branch 2 times, most recently from c72d2ed to 37c33ff Compare October 26, 2020 11:35
This is introduced in PostgreSQL 13 (https://www.postgresql.org/docs/current/sql-dropdatabase.html) and will be enabled when `:force_drop` option is enabled.
@josevalim josevalim merged commit dd0f661 into elixir-ecto:master Oct 27, 2020
@josevalim
Copy link
Member

💚 💙 💜 💛 ❤️

@ruudk
Copy link
Contributor Author

ruudk commented Oct 27, 2020

Yeey, thanks for merging. How are we going to implement this on the other side? Is the repo config the way to go? Should I make a PR?

@ruudk ruudk deleted the force_drop branch October 27, 2020 16:00
@josevalim
Copy link
Member

Yup, what you proposed for the other side is good. :)

josevalim pushed a commit that referenced this pull request Oct 27, 2020
This is introduced in PostgreSQL 13 (https://www.postgresql.org/docs/current/sql-dropdatabase.html)
and will be enabled when `:force_drop` option is enabled.
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