refactor(db): make SQL layer portable and concurrency-safe#754
Merged
Conversation
Replace SQLite-only COLLATE NOCASE with functional LOWER() predicates and
indexes for case-insensitive lookups, and add a migration converting the
legacy NOCASE indexes on existing SQLite databases. Drop MySQL-specific
longblob column type tags.
Add SELECT ... FOR UPDATE row locking (clause.Locking{Strength: "UPDATE"})
to read-modify-write transactions so concurrent updates of the same row are
serialized. On SQLite the single-connection pool already serializes writes,
so the lock is a no-op; the change is groundwork that fixes lost-update races
on any concurrent RDBMS backend.
sqlmock query expectations updated to match the new SQL.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
Two backend-agnostic improvements to the GORM SQL layer:
Case-insensitive lookups via
LOWER()instead ofCOLLATE NOCASE.COLLATE NOCASEis SQLite-specific. Replace it withLOWER(col) = LOWER(?)predicates and matching functional
LOWER(col)indexes in the models, plus amigration (
0002_lower_indexes) that converts the legacy NOCASE indexes onexisting SQLite databases. Also drop the MySQL-specific
longblobcolumn-typetags.
Row locking in read-modify-write transactions.
Add
SELECT ... FOR UPDATE(clause.Locking{Strength: "UPDATE"}) to thetransactions that read a row, mutate it in Go, and write it back, so concurrent
updates of the same row are serialized and can't lose writes.
Why
This improves portability of the existing SQL layer and is a prerequisite for adding a
concurrent backend such as PostgreSQL [#753].
On SQLite both changes are behavior-preserving:
LOWER()andNOCASEboth foldASCII, and the single-writer connection pool already serializes writes, so the
FOR UPDATElock is effectively a no-op there. The value shows up on any backendthat permits concurrent writers, where the lock prevents lost-update races.
The
0002_lower_indexesmigration drops and recreates unique indexes on existing SQLite databases; this is safe because NOCASE and LOWER() both fold ASCII, so any row unique under the old index stays unique under the new one.Testing
go test -tags testing ./database/sql/... ./config/...passes.LOWER()SQL.planner uses them (e.g.
SEARCH organizations USING INDEX idx_org_name_nocase).Out of scope
The file-object/blob storage layer (
file_store.go, theFileBlob/FileObjectTagmodels, and their migration) is deliberately not modified inthis PR because the layer is tied to SQLite specifics; its conversion is
handled in a follow-up PostgreSQL PR.