-
Notifications
You must be signed in to change notification settings - Fork 450
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
For PostgreSQL 14+, provide DROP INDEX migration lines that use CONCURRENTLY #437
base: master
Are you sure you want to change the base?
For PostgreSQL 14+, provide DROP INDEX migration lines that use CONCURRENTLY #437
Conversation
Hi @ankane. I noticed PgHero doesn't show an example that removes an index using the For tables that are actively queried, I've typically dropped indexes in this way. This PR is to ask you if you think this would be helpful to add to PgHero. If so, I can improve this PR further with unit tests, and would be interested in feedback on naming, whether to use configuration parameters, documentation, or other feedback. To see what this looks like, I added a screenshot in the PR description above where the setting is enabled, and in a local Rails test app using this branched version of PgHero. Thanks! |
Also FWIW, @NikolayS covers it here as "Case 14. Careless DROP INDEX" https://postgres.ai/blog/20220525-common-db-schema-change-mistakes#case-14-careless-drop-index |
Looks like this was only added in version 14 so this code should be updated to work only for that version and newer |
I simplified this from the original proposal. I think for PostgreSQL 14, PgHero should provide the drop_index statements with the concurrently option by default. Part of the simplification was to remove the optionality there. Here's a screenshot of what the explanation looks like: The explanation box, and the appearance of the "algorithm: :concurrently" option on each remove_index line are both based on the some predicate method that checks for PostgreSQL 14 or greater. Since the current major version is 16, I think it's more and more likely developers are using version 14 or newer, which means there's a good educational opportunity to make sure developers know that indexes can be dropped concurrently for extra safety. Let me know what you think @ankane. I can squash this down to a single commit if you're interested, since the earlier commits were abandoned. Update: updated screenshot from Space screen |
Whoops, this had commits which weren't meant to be part of this branch. Update: removed all the extra commits, squashed to a single one, rebased against master. |
67fd867
to
0a8fe59
Compare
Present a explanation box recommending to use CONCURRENTLY with DROP INDEX. In Active Record, this requires two things: disable_ddl_transaction! once at the top of the migration file, and "algorithm: :concurrently" option added to every remove_index line Since this is a sensible default when PostgreSQL 14 or greater is in use, since it can avoid downtime from an index drop, make it the default. Remove the ability to disable it since it seems unnecessary in retrospect. Only show the explanation box and only add the option to remove_index when version 14 or greater is in use
0a8fe59
to
a36f400
Compare
Using the concurrently option when dropping indexes still seems like a good default practice for version 14 and newer. |
Hey @ankane - thoughts on this addition? Since Postgres 17 is out now, CONCURRENTLY has been available with DROP INDEX for 4 major versions (14, 15, 16, 17). Happy to update the code against the main branch if needed. Thanks! |
PostgreSQL DROP INDEX documentation (linked) says this:
Background:
When duplicate indexes are detected, PgHero recommends removing one of them. To help make this possible, it provides some commands to generate a Rails Migration. The implementation performs a
remove_index
PostgreSQL Background:
In PostgreSQL it's possible to remove an index using the CONCURRENTLY keyword. e.g.
DROP INDEX index_name CONCURRENTLY
Concurrent modifications are supported by Active Record in the same way as
add_index
withremove_index
, using the combo ofdisable_ddl_transaction!
andalgorithm: :concurrently
.(no longer optional)
This PR adds a new optional configuration parameter to control this behavior. When this parameter is disabled, there is no change to PgHero. It may make sense to disable this by default and allow users to opt in.When this parameter is enabled (set to true), behavior changes slightly.Additional text is added to the HTML view showing how to generate aremove_column
migration using the Active Record equivalent which isalgorithm: :concurrently
.Links to Rails docs on disabling DDL transactions (
disable_ddl_transaction!
in Active Record), and links to PostgreSQLDROP INDEX
documentation are added.Ideally, the new migration could be generated with
disable_ddl_transaction!
set already. Unfortunately it doesn't seem possible currently in Active Record to do this, so it was added to the explanation text area.