schemamigration: skip executing ALTER COLUMN SET NOT NULL queries when possible #1650

jo3-l · 2024-05-22T01:08:44Z

Context

YAGPDB currently uses a bespoke database migration system that works as follows. For each database migration (called a "schema" internally), we try to classify (by testing a set of regexes against the SQL) the migration as doing one of the following:

creating a new table,
adding a new column to a table, or
adding a new index on a column.

If the migration is not recognized as any of the above types, it is run. Otherwise, to avoid gratuitously executing migrations, we check some database metadata to see if the migration is necessary. For instance, if the migration is creating a table, then we query information_schema.tables to see if the table already exists. If so, the query is skipped.

This process is somewhat effective: it successfully detects (and thus skips) most migrations that have already been run, with some exceptions.

The change

As discussed in #dev-discussion, in an upcoming set of PRs, we plan to migrate existing code away from gorm to sqlboiler. Unfortunately, almost all tables created with gorm are currently missing NOT NULL constraints on all columns (see #1652.) Therefore we will need to run a large number of migrations of the type

ALTER TABLE ... ALTER COLUMN ... SET NOT NULL;

Currently this type of query is not recognized by the migration system and so will always be executed. In preparation for the aforementioned series of changes switching away from gorm (which will require many more migrations of this type), this PR therefore lets the migration system recognize ALTER TABLE ... ALTER COLUMN ... SET NOT NULL queries and skip running them if possible.

…n possible

jo3-l · 2024-05-22T01:12:43Z

common/schemamigration.go

+	createTableRegex          = regexp.MustCompile(`(?i)create table if not exists ([0-9a-z_]*) *\(`)
+	alterTableAddColumnRegex  = regexp.MustCompile(`(?i)alter table ([0-9a-z_]*) add column if not exists "?([0-9a-z_]*)"?`)
+	addIndexRegex             = regexp.MustCompile(`(?i)create (unique )?index if not exists ([0-9a-z_]*) on ([0-9a-z_]*)`)
+	addNotNullConstraintRegex = regexp.MustCompile(`(?i)alter table ([0-9a-z_]*) alter column "?([0-9a-z_]+)"? set not null`)


Current regexes assume that column names will always be unquoted; this assumption holds for the current set of migrations but is not generally true. For instance, the reminders table contains a field called when which is a reserved keyword in Postgres and so must be quoted.

Therefore in both the new regex for ALTER COLUMN ... SET NOT NULL queries and the existing ALTER TABLE ... ADD COLUMN regex I have added support for (optionally) quoted column names.

jo3-l · 2024-05-22T01:13:48Z

common/schemamigration.go

@@ -35,7 +36,7 @@ func initSchema(schema string, name string) {
 		return
 	}

-	skip, err := checkSkipSchemaInit(schema, name)
+	skip, err := checkSkipSchemaInit(schema)


While at it, I took the liberty of removing some commented-out code, unused parameters, and extraneous returns as suggested by the linter. These changes are purely stylistic in nature.

It really should be +, but the current code works and hey -- when in Rome, do as the Romans.

…n possible (#1650) * schemamigration: skip executing ALTER COLUMN SET NOT NULL queries when possible * Use quantifier * not + in regex for consistency It really should be +, but the current code works and hey -- when in Rome, do as the Romans.

* all: migrate executed command logging away from gorm to sqlboiler (#1654) * common: add schemas for executed command logs * common: configure and run sqlboiler for executed command log model * all: migrate executed command logging to sqlboiler * reminders: migrate away from gorm to sqlboiler (#1652) * reminders: add database schemas * reminders: set up and run sqlboiler * reminders: migrate to sqlboiler While at it, lightly refactor and simplify the code as applicable. The most substantial (non-database) refactor is renaming and moving strReminders to DisplayReminders. To aid in the refactor, also add discordgo.ParseID as a shortcut for strconv.ParseInt(...). This operation is often needed to convert string IDs in database to int64's and having a shorter name helps. * reminders: auto-delete old entries with null guild_id as necessary * soundboard: remove stray references to gorm (#1651) The soundboard module used to use gorm, but was migrated to sqlboiler some years ago in commit 628ea9a. There are still two (erroneous) references to gorm remaining which were not caught since this section of code ignores errors. This commit changes these to use sqlboiler as well. * schemamigration: skip executing ALTER COLUMN SET NOT NULL queries when possible (#1650) * schemamigration: skip executing ALTER COLUMN SET NOT NULL queries when possible * Use quantifier * not + in regex for consistency It really should be +, but the current code works and hey -- when in Rome, do as the Romans. * youtube: migrate away from gorm to sqlboiler (#1660) * youtube: add database schemas * youtube: set up and run sqlboiler * youtube: migrate to sqlboiler * notifications: migrate away from gorm to sqlboiler (#1659) * notifications: decouple from configstore * notifications: add database schemas * notifications: set up and run sqlboiler * notifications: migrate to sqlboiler * moderation: migrate away from gorm to sqlboiler (#1658) * moderation: decouple from configstore * moderation: add database schemas * moderation: set up and run sqlboiler * moderation: migrate to sqlboiler * web: support null.Int64 in forms and validator We currently use gorilla/schema to parse HTML form values into structs -- this allows us to specify, e,g., `<input ... name=SomeField>` and have `payload.SomeField` set correctly in Go handlers. This approach mostly works nicely, with the major exception of null.Int64 (formerly sql.NullInt64 with gorm) fields, which show up in the moderation plugin, e.g., Config.DefaultMuteDuration. Particularly, since gorilla/schema has no native support for null.Int64, we have instead instructed gorilla/schema to directly set the Int64 value using <input ... name=DefaultMuteDuration.Int64> and then manually set DefaultMuteDuration.Valid=true in the web handler, without which the changes are not saved to database. This approach is, for obvious reasons, rather brittle. In this commit we therefore register a custom gorilla/schema decoder for null.Int64, obviating the need for both pointing to the Int64 field and manually setting Valid=true. While at it, we also fix the validator so that it checks null.Int64 fields properly. * web: support validating types.Int64Array gorm uses pq.Int64Array but sqlboiler uses types.Int64Array, so we need also support the latter now. * common/configstore: remove package (#1668) * all: remove last traces of gorm (#1670) * common: connect to postgres via database/sql ...instead of through gorm. * common: remove gorm log hook * common: remove gorm SmallModel * deps: run go mod tidy --------- Co-authored-by: Joe L <56809242+jo3-l@users.noreply.github.com>

…g#1655) * all: migrate executed command logging away from gorm to sqlboiler (botlabs-gg#1654) * common: add schemas for executed command logs * common: configure and run sqlboiler for executed command log model * all: migrate executed command logging to sqlboiler * reminders: migrate away from gorm to sqlboiler (botlabs-gg#1652) * reminders: add database schemas * reminders: set up and run sqlboiler * reminders: migrate to sqlboiler While at it, lightly refactor and simplify the code as applicable. The most substantial (non-database) refactor is renaming and moving strReminders to DisplayReminders. To aid in the refactor, also add discordgo.ParseID as a shortcut for strconv.ParseInt(...). This operation is often needed to convert string IDs in database to int64's and having a shorter name helps. * reminders: auto-delete old entries with null guild_id as necessary * soundboard: remove stray references to gorm (botlabs-gg#1651) The soundboard module used to use gorm, but was migrated to sqlboiler some years ago in commit 628ea9a. There are still two (erroneous) references to gorm remaining which were not caught since this section of code ignores errors. This commit changes these to use sqlboiler as well. * schemamigration: skip executing ALTER COLUMN SET NOT NULL queries when possible (botlabs-gg#1650) * schemamigration: skip executing ALTER COLUMN SET NOT NULL queries when possible * Use quantifier * not + in regex for consistency It really should be +, but the current code works and hey -- when in Rome, do as the Romans. * youtube: migrate away from gorm to sqlboiler (botlabs-gg#1660) * youtube: add database schemas * youtube: set up and run sqlboiler * youtube: migrate to sqlboiler * notifications: migrate away from gorm to sqlboiler (botlabs-gg#1659) * notifications: decouple from configstore * notifications: add database schemas * notifications: set up and run sqlboiler * notifications: migrate to sqlboiler * moderation: migrate away from gorm to sqlboiler (botlabs-gg#1658) * moderation: decouple from configstore * moderation: add database schemas * moderation: set up and run sqlboiler * moderation: migrate to sqlboiler * web: support null.Int64 in forms and validator We currently use gorilla/schema to parse HTML form values into structs -- this allows us to specify, e,g., `<input ... name=SomeField>` and have `payload.SomeField` set correctly in Go handlers. This approach mostly works nicely, with the major exception of null.Int64 (formerly sql.NullInt64 with gorm) fields, which show up in the moderation plugin, e.g., Config.DefaultMuteDuration. Particularly, since gorilla/schema has no native support for null.Int64, we have instead instructed gorilla/schema to directly set the Int64 value using <input ... name=DefaultMuteDuration.Int64> and then manually set DefaultMuteDuration.Valid=true in the web handler, without which the changes are not saved to database. This approach is, for obvious reasons, rather brittle. In this commit we therefore register a custom gorilla/schema decoder for null.Int64, obviating the need for both pointing to the Int64 field and manually setting Valid=true. While at it, we also fix the validator so that it checks null.Int64 fields properly. * web: support validating types.Int64Array gorm uses pq.Int64Array but sqlboiler uses types.Int64Array, so we need also support the latter now. * common/configstore: remove package (botlabs-gg#1668) * all: remove last traces of gorm (botlabs-gg#1670) * common: connect to postgres via database/sql ...instead of through gorm. * common: remove gorm log hook * common: remove gorm SmallModel * deps: run go mod tidy --------- Co-authored-by: Joe L <56809242+jo3-l@users.noreply.github.com>

schemamigration: skip executing ALTER COLUMN SET NOT NULL queries whe…

a13375a

…n possible

jo3-l commented May 22, 2024

View reviewed changes

Use quantifier * not + in regex for consistency

8dafe19

It really should be +, but the current code works and hey -- when in Rome, do as the Romans.

jo3-l changed the base branch from dev to gorm-to-sqlboiler May 22, 2024 05:16

ashishjh-bst merged commit 76c003b into botlabs-gg:gorm-to-sqlboiler May 28, 2024
1 check passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

schemamigration: skip executing ALTER COLUMN SET NOT NULL queries when possible #1650

schemamigration: skip executing ALTER COLUMN SET NOT NULL queries when possible #1650

jo3-l commented May 22, 2024 •

edited

Loading

jo3-l May 22, 2024

jo3-l May 22, 2024

schemamigration: skip executing ALTER COLUMN SET NOT NULL queries when possible #1650

schemamigration: skip executing ALTER COLUMN SET NOT NULL queries when possible #1650

Conversation

jo3-l commented May 22, 2024 • edited Loading

Context

The change

jo3-l May 22, 2024

Choose a reason for hiding this comment

jo3-l May 22, 2024

Choose a reason for hiding this comment

jo3-l commented May 22, 2024 •

edited

Loading