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

For PostgreSQL 14+, provide DROP INDEX migration lines that use CONCURRENTLY #437

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

andyatkinson
Copy link
Contributor

@andyatkinson andyatkinson commented Dec 28, 2022

image

PostgreSQL DROP INDEX documentation (linked) says this:

Drop the index without locking out concurrent selects, inserts,
updates, and deletes on the index's table

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 with remove_index, using the combo of disable_ddl_transaction! and algorithm: :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 a remove_column migration using the Active Record equivalent which is algorithm: :concurrently.

Links to Rails docs on disabling DDL transactions (disable_ddl_transaction! in Active Record), and links to PostgreSQL DROP 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.

@andyatkinson andyatkinson changed the title Adds CONCURRENTLY to remove_index [feedback requested] [wip] Adds CONCURRENTLY to remove_index Dec 28, 2022
@andyatkinson
Copy link
Contributor Author

andyatkinson commented Dec 28, 2022

Hi @ankane. I noticed PgHero doesn't show an example that removes an index using the CONCURRENTLY keyword. This may not always be necessary, but it is a bit safer and the cost is nominal. My understanding is that remove an index using CONCURRENLTY will run a bit slower compared with removing the index and not using it.

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!

@andyatkinson
Copy link
Contributor Author

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

@andyatkinson
Copy link
Contributor Author

Looks like this was only added in version 14 so this code should be updated to work only for that version and newer

@andyatkinson andyatkinson changed the title [feedback requested] [wip] Adds CONCURRENTLY to remove_index Adds CONCURRENTLY to remove_index Jan 3, 2024
@andyatkinson
Copy link
Contributor Author

andyatkinson commented Jan 3, 2024

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:

explanation

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

@andyatkinson
Copy link
Contributor Author

andyatkinson commented Jan 3, 2024

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.

@andyatkinson andyatkinson force-pushed the feature/remove-index-concurrently branch 2 times, most recently from 67fd867 to 0a8fe59 Compare January 3, 2024 22:13
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
@andyatkinson andyatkinson force-pushed the feature/remove-index-concurrently branch from 0a8fe59 to a36f400 Compare January 3, 2024 22:17
@andyatkinson andyatkinson changed the title Adds CONCURRENTLY to remove_index For PostgreSQL 14+, provide DROP INDEX migration lines that use CONCURRENTLY Jan 3, 2024
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.

None yet

1 participant