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

Bug: concurrent_index complaining when creating/dropping an index on a partitioned table in PostgreSQL #38

Closed
francktrouillez opened this issue Mar 19, 2024 · 2 comments

Comments

@francktrouillez
Copy link
Contributor

I got the cop RuboCop(Sequel/ConcurrentIndex) complaining when I'm trying to run a migration to create/drop an index on a partitioned table in PostgreSQL when concurrently: true is not set, while it is currently not possible in PostgreSQL.

Steps to reproduce

  1. Create a migration that adds an index to a partitioned table
  2. Don't add concurrently: true
  3. Check that the cop RuboCop(Sequel/ConcurrentIndex) is complaining
  4. Add concurrently: true
  5. Run the migration
  6. Get an exception
Sequel::DatabaseError: PG::FeatureNotSupported: ERROR:  cannot create index on partitioned table "my_table" concurrently
ROM::SQL.migration do
  no_transaction

  change do
    alter_table :my_table do
      add_index :my_index, concurrently: true
    end
  end
end

Expected behavior

The cop should not complain, as the proposed change is not working for partitioned table in PostgreSQL

Actual behavior

The cop is complaining, forcing me to change to a non-working solution where a Sequel::DatabaseError: PG::FeatureNotSupported is raised.

Proposed change

If this situation is still happening with up-to-date gems and ruby versions, I believe we could go in 2 directions:

  • Remove the cop: fast and simple, but might decrease experience for people that don't fact this issue.
  • Make the cop less restrictive or more clear: we could enforce to specify concurrently, even if it is set to false, and change the message from 'Prefer creating or dropping new index concurrently' to 'Specify concurrently option when creating or dropping an index.
  • I'm of course open for other solutions!

I'm more in favor of the second option of keeping the cop but forcing it to say we should specify the concurrently option, instead of setting it to true, but I'm open for suggestions!

I've opened a PR for this if it makes sense.

System configuration

rubocop-sequel version:
0.3.4

sequel version:
5.76.0

pg version:
1.5.4

Ruby version:
ruby 3.2.2 (2023-03-30 revision e51014f9c0) [x86_64-linux]

@cyberdelia
Copy link
Collaborator

You could also disable the cop locally using comments, see: https://docs.rubocop.org/rubocop/1.62/configuration.html#disabling-cops-within-source-code

@francktrouillez
Copy link
Contributor Author

You could also disable the cop locally using comments, see: https://docs.rubocop.org/rubocop/1.62/configuration.html#disabling-cops-within-source-code

Yes, that was also a possibility! Thanks for pointing that out!
I think updating the message is still a good idea, as it is more explicit about what's expected and doesn't enforce "wrong" implementation in some situation (like the one in this PR).

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

No branches or pull requests

2 participants