search

sbdchd / squawk

🐘 linter for PostgreSQL, focused on migrations
https://squawkhq.com
GNU General Public License v3.0
541 stars 37 forks source link

New ban-concurrent-index-creation-in-transaction in v0.27 breaks workflow #338

Closed janrueth closed 5 months ago

janrueth commented 5 months ago

Hey,

I like the ban-concurrent-index-creation-in-transaction (Added by @alixlahuec in https://github.com/sbdchd/squawk/pull/335). However, I think the rule needs some adjustment.

I use go-migrate which usually wraps migrations in transactions. As such I'm setting

assume_in_transaction = true

go-migrate is either smart enough to see that single statements don't need to be wrapped in a transaction or that CREATE INDEX CONCURRENTLY must not be wrapped. Either way, having it in a single migration with go-migrate (which usually wraps transactions) works without any issues, it starts failing as soon as I have additional statements in the migration (which makes sense as it need a transaction then).

With the new rule, the linter complains on all concurrent index creations. I don't want to disable assume_in_transaction and I actually want ban-concurrent-index-creation-in-transaction to work here too.

I think the rule could treat assume_in_transaction differently:

For me, a problem only comes when I have a concurrent index creation together with further statements in a migration.

So, I'd propose to change the rule such that if assume_in_transaction is on, that the rule requires more than 1 statement to fail (which would be a useful rule for me to lint on such cases that will break with go-migrate).

Happy to provide a PR if you agree that the behavior should change.

chdsbd commented 5 months ago

Sounds good to me. I'd accept a PR with this change

chdsbd commented 5 months ago

Your PR #339 has been released in v0.28.0. Thanks!