-
Notifications
You must be signed in to change notification settings - Fork 395
Fix schema lint script to understand CREATE TABLE IF NOT EXISTS
#19020
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
base: develop
Are you sure you want to change the base?
Conversation
da202a3
to
bff4465
Compare
6845314
to
b767d62
Compare
|
||
CREATE INDEX test1_id_idx ON test1 (id); | ||
|
||
CREATE TABLE IF NOT EXISTS test2 ( |
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 is separate from the lint being updated but this spawned some discussion in an internal room about whether IF NOT EXISTS
was desirable.
On one hand, @sandhose mentions how IF NOT EXISTS
could be a foot-gun as the table may exist but not have the same shape that we have in the schema.
@richvdh also noted some historical context that we may have used IF NOT EXISTS
in some places because migration files weren't applied atomically, so in some edge-cases you could end up running them twice. Although that may be fixed now.
Basically, it seems that if we have fixed the atomicity problem of deltas, we should also have a new lint that prevents people from using IF NOT EXISTS
. Something for the future ⏩
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.
@richvdh also noted some historical context that we may have used IF NOT EXISTS in some places because migration files weren't applied atomically, so in some edge-cases you could end up running them twice. Although that may be fixed now.
I think we fixed it: matrix-org/synapse#6467
TABLE_CREATION_REGEX = re.compile( | ||
r"CREATE .*TABLE.* ([a-z_0-9]+)\s*\(", flags=re.IGNORECASE | ||
) |
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.
I think there are more edge cases. Perhaps some of these would be better prevented with some SQL lints 🤷
Create table using another table
It's also possible to create a table using another table:
CREATE TABLE new_table_name AS
SELECT column1, column2,...
FROM existing_table_name
WHERE ....;
Or:
CREATE TABLE new_table (LIKE old_table);
Quoted identifiers
It looks like it's also possible to put quotes around the identifiers:
CREATE TABLE "My_Table_ABC" ("My_Very_Upper_and_Lowercasy_Column" INT, ...)
Create table over multiple lines
CREATE TABLE users
(
id INT,
name VARCHAR(255)
);
@@ -0,0 +1 @@ | |||
Fix CI linter for schema delta files to correctly handle all types of `CREATE TABLE` syntax. |
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.
I think this is better than before so we can merge ⏩
But I think there are a few more edge cases this doesn't handle.
The schema lint tries to make sure we don't add or remove indices in schema files (rather than as background updates), unless the table was created in the same schema file.
The regex to pull out the
CREATE TABLE
SQL incorrectly didn't recogniseIF NOT EXISTS
.There is a test delta file that shows that we accept different types of
CREATE TABLE
andCREATE INDEX
statements, as well as an index creation that doesn't have a matching create table (to show that we do still catch it). The test delta should be removed before merge.