-
Notifications
You must be signed in to change notification settings - Fork 429
Check for CREATE/DROP INDEX in schema deltas
#18440
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
Conversation
As these should be background updates.
MadLittleMods
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Assuming you've tried this out to ensure the matching actually works 👍
Thanks for making a lint to catch this mistake!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Still some strange pre-existing logic in this file but as good as before with a new good feature 👍
| @@ -0,0 +1 @@ | |||
| Add lint to ensure we don't add a `CREATE/DROP INDEX` in a schema delta. | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why would we ban DROP INDEX? Afaik that is instant?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It isn't instant for large tables, and takes out aggressive locks, which why there's a DROP INDEX CONCURRENTLY version
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Though TBF it is rare for it to actually cause woe
scripts-dev/check_schema_delta.py
Outdated
| clause = match.group() | ||
|
|
||
| click.secho( | ||
| f"Found delta with index creation/deletion: '{clause}' in {delta_file}\nThese should be in background updates.", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe it'd be worth saying that, for new tables, it's fine to create the index right now, but you can write the index definition inline in the CREATE TABLE statement
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point! The CREATE INDEX syntax is useful but we would need to make the logic more robust to also make sure we see an associated CREATE TABLE in the same delta.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
but you can write the index definition inline in the CREATE TABLE statement
lies, it seems you can't do this for indices. :<
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
D'oh. Good catch. Have updated to allow index creation for tables we create in the same PR.
scripts-dev/check_schema_delta.py
Outdated
|
|
||
| repo = git.Repo() | ||
| repo.remote().fetch() | ||
| repo.remote().fetch(refspec="develop") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Label this magic value
| repo.remote().fetch(refspec="develop") | |
| repo.remote().fetch(refspec=DEFAULT_BRANCH) |
Did this spawn from something you saw? I assume GitHub would give you the default branch anyway if you don't specify one?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was purely to speed things up, so that we only pull that one branch that we care about rather than all branches
Co-authored-by: Eric Eastwood <[email protected]>
As these should be background updates.
(foot-gun)
Spawning from #18439