Skip to content

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.

Sign up for GitHub

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

Open
wants to merge 4 commits into
base: develop
Choose a base branch
from
+36 −5
Open

Fix schema lint script to understand CREATE TABLE IF NOT EXISTS #19020

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
1660826
Fix CREATE TABLE regex to handle IF NOT EXISTS
erikjohnston Oct 6, 2025
f9003c8
Test schema file
erikjohnston Oct 6, 2025
2688f8b
Better diagnostics
erikjohnston Oct 6, 2025
b767d62
Newsfile
erikjohnston Oct 6, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions changelog.d/19020.misc
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Fix CI linter for schema delta files to correctly handle all types of `CREATE TABLE` syntax.
Copy link
Contributor

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.

20 changes: 15 additions & 5 deletions scripts-dev/check_schema_delta.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,9 +11,13 @@
import git

SCHEMA_FILE_REGEX = re.compile(r"^synapse/storage/schema/(.*)/delta/(.*)/(.*)$")
INDEX_CREATION_REGEX = re.compile(r"CREATE .*INDEX .*ON ([a-z_]+)", flags=re.IGNORECASE)
INDEX_DELETION_REGEX = re.compile(r"DROP .*INDEX ([a-z_]+)", flags=re.IGNORECASE)
TABLE_CREATION_REGEX = re.compile(r"CREATE .*TABLE ([a-z_]+)", flags=re.IGNORECASE)
INDEX_CREATION_REGEX = re.compile(
r"CREATE .*INDEX .*ON ([a-z_0-9]+)", flags=re.IGNORECASE
)
INDEX_DELETION_REGEX = re.compile(r"DROP .*INDEX ([a-z_0-9]+)", flags=re.IGNORECASE)
TABLE_CREATION_REGEX = re.compile(
r"CREATE .*TABLE.* ([a-z_0-9]+)\s*\(", flags=re.IGNORECASE
)
Comment on lines +18 to +20
Copy link
Contributor

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)
);


# The base branch we want to check against. We use the main development branch
# on the assumption that is what we are developing against.
Expand Down Expand Up @@ -173,11 +177,14 @@ def main(force_colors: bool) -> None:
clause = match.group()

click.secho(
f"Found delta with index deletion: '{clause}' in {delta_file}\nThese should be in background updates.",
f"Found delta with index deletion: '{clause}' in {delta_file}",
fg="red",
bold=True,
color=force_colors,
)
click.secho(
" ↪ These should be in background updates.",
)
return_code = 1

# Check for index creation, which is only allowed for tables we've
Expand All @@ -188,11 +195,14 @@ def main(force_colors: bool) -> None:
table_name = match.group(1)
if table_name not in created_tables:
click.secho(
f"Found delta with index creation: '{clause}' in {delta_file}\nThese should be in background updates.",
f"Found delta with index creation for existing table: '{clause}' in {delta_file}",
fg="red",
bold=True,
color=force_colors,
)
click.secho(
" ↪ These should be in background updates (or the table should be created in the same delta).",
)
return_code = 1

click.get_current_context().exit(return_code)
Expand Down
20 changes: 20 additions & 0 deletions synapse/storage/schema/main/delta/92/00_test1.sql
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
CREATE TABLE test1 (
id INT
);

CREATE INDEX test1_id_idx ON test1 (id);

CREATE TABLE IF NOT EXISTS test2 (
Copy link
Contributor

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 ⏩

Copy link
Member

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

id INT
);

CREATE INDEX test2_id_idx ON test2 (id);
CREATE INDEX IF NOT EXISTS test2_id_idx ON test2 (id);

CREATE INDEX CONCURRENTLY IF NOT EXISTS test3_id_idx ON test3 (id);

CREATE TEMPORARY TABLE IF NOT EXISTS test4 (
id INT
);

CREATE INDEX IF NOT EXISTS test4_id_idx ON test4 (id);
Loading