-
-
Notifications
You must be signed in to change notification settings - Fork 87
fix(sync-engine): replace pg-node-migrations with custom runner #234
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: main
Are you sure you want to change the base?
fix(sync-engine): replace pg-node-migrations with custom runner #234
Conversation
|
Thanks for the contribution already! For the new migrations, what would be very useful is to also support some type of placeholders - i.e. the |
|
Good point on the schema placeholder. Two approaches come to mind: Option 1: Multi-pattern regex sql
.replace(/"stripe"\./g, `"${schema}".`)
.replace(/\bstripe\./g, `${schema}.`)
.replace(/'stripe'/g, `'${schema}'`)
.replace(/\bstripe_/g, `${schema}_`) // index namesFragile - easy to miss edge cases. Option 2: Explicit placeholders sql.replace(/\{\{schema\}\}/g, config.schema)More upfront work but explicit and safe. I'm leaning toward Option 2 - cleaner and less error-prone. Which direction would you prefer? |
|
One more thought: Option 2 would make the migration files invalid SQL on their own - can't just run them with psql. Option 1 keeps them valid and defaults to |
|
I'd still opt for option 2 as option 1 can lead to accidental replacements When someone tries running the SQL files directly, it would be easy enough to do a manual replacement and it also leads to making a more explicit choice of the schema Could also have a utility function to print out all schema files with placeholders replaced, but that is more of a nice to have |
- Custom migration runner with advisory locks (no checksum validation)
- Use {{schema}} placeholders for configurable schema support
- Add getMigrations(schema) export for inspecting migrations
Fixes supabase#191, fixes supabase#77
731217c to
48220b7
Compare
|
Done, went with Option 2 - all migrations now use Also normalized quoting across all the migrations files while at it. |
|
I've been thinking, probably |
Fixes #191. Fixes #77.
Problem
Migrations failing if enum types exist in different schemas #191: Migrations fail when users have enums with the same name in
publicschema (e.g.,public.pricing_type). The enum existence checks weren't schema-scoped.0012_add_updated_at.sql hardcodes role name to postgres #77: Migration
0012_add_updated_at.sqlhardcodesOWNER TO postgres, failing for users without apostgresrole.We couldn't fix either because
pg-node-migrationsvalidates checksums and rejects modified migration files.Solution
Replace
pg-node-migrationswith a custom runner that skips checksum validation, then fix the migrations.Changes
pg_type JOIN pg_namespace)OWNER TO postgrespg-node-migrationsdependencyTesting
Compatibility
Compared with pg-node-migrations source:
-- postgres-migrations disable-transactionsupportedOpen questions
-- postgres-migrations disable-transaction. Remove?