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

Assume in transaction still warns about concurrent index creation #331

Open
peroxy opened this issue Dec 21, 2023 · 2 comments
Open

Assume in transaction still warns about concurrent index creation #331

peroxy opened this issue Dec 21, 2023 · 2 comments

Comments

@peroxy
Copy link

peroxy commented Dec 21, 2023

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
Copy link
Collaborator

chdsbd commented Dec 21, 2023

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
Copy link
Author

peroxy commented Dec 21, 2023

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.

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