Add postgres migrations #12
Conversation
eminano
force-pushed
the
add-postgres-migrations
branch
from
f941e6e
to
fcc69e6
Compare
| } | ||
|
|
||
| func newPGMigrator() (*migrate.Migrate, error) { | ||
| src := bindata.Resource(pgmigrations.AssetNames(), pgmigrations.Asset) |
There was a problem hiding this comment.
if we think about pgstream the library, there is a lot of logic here (in the CLI). pgstream users will need to be able to do "init" from the library, I suggest this is moved into it. (not necessarily now)
There was a problem hiding this comment.
Yeah, this is far from done. It's just getting something out for testing, but the init functionality will definitely be shared by the library. Just need to figure out the best place for it when the time comes.
There was a problem hiding this comment.
Code looks good to me, also great to see the CLI shaping up!
A comment on the general approach. I know all these migration files are coming from the original project embed in Xata, but I'm wondering if this is a good opportunity to challenge that approach and simplify it, at least, for now.
For pgroll we created a single idempotent migration SQL, that worked well so far, without the need for keeping extra state or having different files: https://github.com/xataio/pgroll/blob/a87fa36dda1cd8213ebf62a4bffbdc9536777bc0/pkg/state/state.go#L17-L335
In all cases, I think it would be good to encapuslate "init" process as a function in the public lib, similar to pgroll.Init.
I did look into the way pgroll did the init migration, it just didn't feel very readable and I figured it would be easier to understand and expand if the well known migration approach was taken, while using a binary document to use it in the CLI/ library. I was expecting this to be a potential contention point though :D I figured that having the schema migrations under the pgstream schema should help keep things contained. If you feel this is not worth it, I can move it to a sql string and run it all in a single step. Maybe for a library it's better to keep things without state, but considering this is heavily linked to postgres, keeping some sort of state seems inevitable (all the tables being created already under the pgstream schema being an example of that). I agree with the library encapsulation of the init behaviour. It's just not there yet :D |
|
No strong opinion besides making this easy to use from a lib :), I would say just merge it as it is :) and follow up at some point |
This PR adds the postgres migrations required for pgstream to work. This includes tables, functions and triggers used to capture and track schema changes.
A CLI is created with init/tear down commands for now to run the postgres migrations against the configured database. This allows to start setting up an environment that can be used for e2e validation.