sbdchd / squawk

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

Assume in transaction still warns about concurrent index creation #331

Open peroxy opened 11 months ago

peroxy commented 11 months ago

My SQL migration that I run in transaction with flyway:

CREATE INDEX IF NOT EXISTS idx_foo_bar ON foo.bar (field);

Lint it:

squawk --assume-in-transaction --verbose "migration.sql"

Output:

./sql/migration.sql:1:1: warning: require-concurrent-index-creation

   1 | CREATE INDEX IF NOT EXISTS idx_foo_bar ON foo.bar (field);

  note: Creating an index blocks writes.
  help: Create the index CONCURRENTLY.

I would expect that this warning gets suppressed if I create indexes non-concurrently in a transaction.

chdsbd commented 11 months ago

I'm not sure that disabling require-concurrent-index-creation when --assume-in-transaction is the right decision here.

--assume-in-transaction simulates linting a migration with a BEGIN; COMMIT;. I think if a user creates an index, they should be warned about concurrency indices, even if they wrap in a transaction.

Maybe we need a new warning to warn that you cannot run a concurrent index in a transaction.

I think require-concurrent-index-creation is still useful here, because on large tables you really only want to create transactions concurrently.

For your use case, can you disable require-concurrent-index-creation?

squawk --exclude=require-concurrent-index-creation migration.sql
peroxy commented 11 months ago

That's what we ended up doing:

# .squawk.toml
excluded_rules = [
    "require-concurrent-index-creation",
    "require-concurrent-index-deletion"
]
assume_in_transaction = true

Maybe we need a new warning to warn that you cannot run a concurrent index in a transaction.

I think that would be a good compromise, I agree.