index_not_concurrently should verify that DDL transactions/migrations lock are disabled

[milmazz]

Hello 👋

First, thanks for working on this library. It seems helpful to enforce good practices while you do data migrations. In particular, I'm interested in the index_not_concurrently check. On my quick review, it seems to detect the lack of the concurrently option in your index., but apparently, the check doesn't verify for:

@disable_ddl_transaction true
@disable_migration_lock true

Both module attributes are recommended when you add or drop an index concurrently, from the Ecto docs we have:

PostgreSQL supports adding/dropping indexes concurrently (see the docs). However, this feature does not work well with the transactions used by Ecto to guarantee integrity during migrations.

Therefore, to migrate indexes concurrently, you need to set both @disable_ddl_transaction and @disable_migration_lock to true:

Disabling DDL transactions removes the guarantee that all of the changes in the migration will happen at once. Disabling the migration lock removes the guarantee only a single node will run a given migration if multiple nodes are attempting to migrate at the same time.

Since running migrations outside a transaction and without locks can be dangerous, consider performing very few operations in migrations that add concurrent indexes. We recommend to run migrations with concurrent indexes in isolation and disable those features only temporarily.

Also, based on the last paragraph, the calls that should be allowed while you have disabled the DDL transactions and the migration lock are create/drop index(...., concurrently: true), but I don't know if that's feasible with this check.

[hiagomeels]

@Artur-Sulej, I will work on a proposal PR to add this checker since I'm running into the same issue.

I think we will need to do this to go through the entire code_ast for the module and see if those module attributes exist and their values are true, and one of the database operations should be an index concurrently.

Should we show an error per attribute? what the name of these errors looks like (index_concurrently_without_MODULE_ATTR_NAME)?

What do you think?

[hiagomeels]

@Artur-Sulej After closing MR #14. I think we can close this issue as well.