Add migration that brings line column to the migration table - #435
Merged
Conversation
brandur
force-pushed
the
brandur-migration-line
branch
from
July 8, 2024 03:42
f0cdbc1 to
dd43d26
Compare
brandur
force-pushed
the
brandur-migration-line
branch
from
July 8, 2024 05:48
dd43d26 to
9cf3342
Compare
bgentry
approved these changes
Jul 8, 2024
| // the migration `line` comes in. If migration to a point equal or above | ||
| // 005, we can remove migrations with a line included, but otherwise we | ||
| // must omit the `line` column from queries because it doesn't exist. | ||
| if m.line == riverdriver.MigrationLineMain && slices.Min(versions) <= 5 { |
Contributor
There was a problem hiding this comment.
The value 5 here might be clearer if put in a named constant
Contributor
Author
There was a problem hiding this comment.
Yep, good call. Changed.
Here, add a new River migration (version 005) that brings a `line`
column to the `river_migration` table, allowing non-main lines to be
supported. We also teach the migrator how to use it.
This one is a little trickier than it sounds (partly thanks to sqlc)
because the migrator needs to behave somewhat differently depending on
whether the `line` column exits yet. So for example, when migrating to
004 it needs to upsert migration records that _do not_ include a `line`
field, but then when migrating to 005 or beyond, a `line` value is
included. The same consideration goes for performing a down migration,
or checking the existing state of migrations in the database.
This is implemented by pairing driver functions related to migrations
into one for pre-`line` and another for post-`line`. e.g.
// MigrationDeleteAssumingMainMany deletes many migrations assuming
// everything is on the main line. This is suitable for use in databases on
// a version before the `line` column exists.
MigrationDeleteAssumingMainMany(ctx context.Context, versions []int) ([]*Migration, error)
// MigrationDeleteByLineAndVersionMany deletes many migration versions on a
// particular line.
MigrationDeleteByLineAndVersionMany(ctx context.Context, line string, versions []int) ([]*Migration, error)
We don't yet pull line support into the CLI, but this should make
alternate lines fully supported up to that point. The CLI needs a little
more thought because it might involve building a separate binary.
brandur
force-pushed
the
brandur-migration-line
branch
from
July 9, 2024 00:28
9cf3342 to
c815198
Compare
Contributor
Author
|
Thanks! |
brandur
added a commit
that referenced
this pull request
Jul 11, 2024
I'd totally forgotten I'd even created it this way, but going back through migrations recently I realized that the table's primary key is a `bigserial`. This isn't totally bad, and even has some advantages in that rows are easier to target from a psql session, but in this instance it feels a little on the sloppy side because it's fairly easy to conflate the serial with migration `version` -- both are expected to be small integers that a human could easily mix up. Since we haven't shipped a new version yet containing the addition of `line` in #435, here I propose that we modify that a little further to reshape `river_migration` into a slightly smaller table with a compound key on `(line, version)`: CREATE TABLE river_migration( created_at timestamptz NOT NULL DEFAULT NOW(), line TEXT NOT NULL, version bigint NOT NULL, CONSTRAINT line_length CHECK (char_length(line) > 0 AND char_length(line) < 128), CONSTRAINT version_gte_1 CHECK (version >= 1), PRIMARY KEY (line, version) ); This has two tiny side benefits that (1) the table is smaller (drops a column), and (2) we don't need an extra index on `(line, version)` since it's already covered by the primary key. Very similar in spirit to what it was before, but I think a little cleaner in design.
brandur
added a commit
that referenced
this pull request
Jul 13, 2024
I'd totally forgotten I'd even created it this way, but going back through migrations recently I realized that the table's primary key is a `bigserial`. This isn't totally bad, and even has some advantages in that rows are easier to target from a psql session, but in this instance it feels a little on the sloppy side because it's fairly easy to conflate the serial with migration `version` -- both are expected to be small integers that a human could easily mix up. Since we haven't shipped a new version yet containing the addition of `line` in #435, here I propose that we modify that a little further to reshape `river_migration` into a slightly smaller table with a compound key on `(line, version)`: CREATE TABLE river_migration( created_at timestamptz NOT NULL DEFAULT NOW(), line TEXT NOT NULL, version bigint NOT NULL, CONSTRAINT line_length CHECK (char_length(line) > 0 AND char_length(line) < 128), CONSTRAINT version_gte_1 CHECK (version >= 1), PRIMARY KEY (line, version) ); This has two tiny side benefits that (1) the table is smaller (drops a column), and (2) we don't need an extra index on `(line, version)` since it's already covered by the primary key. Very similar in spirit to what it was before, but I think a little cleaner in design.
brandur
added a commit
that referenced
this pull request
Jul 13, 2024
I'd totally forgotten I'd even created it this way, but going back through migrations recently I realized that the table's primary key is a `bigserial`. This isn't totally bad, and even has some advantages in that rows are easier to target from a psql session, but in this instance it feels a little on the sloppy side because it's fairly easy to conflate the serial with migration `version` -- both are expected to be small integers that a human could easily mix up. Since we haven't shipped a new version yet containing the addition of `line` in #435, here I propose that we modify that a little further to reshape `river_migration` into a slightly smaller table with a compound key on `(line, version)`: CREATE TABLE river_migration( created_at timestamptz NOT NULL DEFAULT NOW(), line TEXT NOT NULL, version bigint NOT NULL, CONSTRAINT line_length CHECK (char_length(line) > 0 AND char_length(line) < 128), CONSTRAINT version_gte_1 CHECK (version >= 1), PRIMARY KEY (line, version) ); This has two tiny side benefits that (1) the table is smaller (drops a column), and (2) we don't need an extra index on `(line, version)` since it's already covered by the primary key. Very similar in spirit to what it was before, but I think a little cleaner in design.
brandur
added a commit
that referenced
this pull request
Jul 13, 2024
I'd totally forgotten I'd even created it this way, but going back through migrations recently I realized that the table's primary key is a `bigserial`. This isn't totally bad, and even has some advantages in that rows are easier to target from a psql session, but in this instance it feels a little on the sloppy side because it's fairly easy to conflate the serial with migration `version` -- both are expected to be small integers that a human could easily mix up. Since we haven't shipped a new version yet containing the addition of `line` in #435, here I propose that we modify that a little further to reshape `river_migration` into a slightly smaller table with a compound key on `(line, version)`: CREATE TABLE river_migration( created_at timestamptz NOT NULL DEFAULT NOW(), line TEXT NOT NULL, version bigint NOT NULL, CONSTRAINT line_length CHECK (char_length(line) > 0 AND char_length(line) < 128), CONSTRAINT version_gte_1 CHECK (version >= 1), PRIMARY KEY (line, version) ); This has two tiny side benefits that (1) the table is smaller (drops a column), and (2) we don't need an extra index on `(line, version)` since it's already covered by the primary key. Very similar in spirit to what it was before, but I think a little cleaner in design.
brandur
added a commit
that referenced
this pull request
Jul 14, 2024
Here, break out the CLI so that most of its implementation no longer
lives in a `main.go`, and rather in a subpackage that can be imported
from somewhere else. This lets the majority of it to be reused in
another executable and augmented in various ways.
An addition is a new `DriverProcurer` interface that can provide a
driver for various databases:
type DriverProcurer interface {
ProcurePgxV5(pool *pgxpool.Pool) riverdriver.Driver[pgx.Tx]
}
For the main CLI, this gets a trivial implementation using `riverpgxv5`,
but could potentially be reimplemented elsewhere to swap in something
else;
type DriverProcurer struct{}
func (p *DriverProcurer) ProcurePgxV5(pool *pgxpool.Pool) riverdriver.Driver[pgx.Tx] {
return riverpgxv5.New(pool)
}
To make this more workable, I end up going through and refactoring quite
a lot of code, making it into a bit of a mini framework, and one that
could potentially support additional databases in the future without
having to refactor the world again.
We also add a `--line` flag to support the feature from #435 to the CLI.
brandur
added a commit
that referenced
this pull request
Jul 14, 2024
Here, break out the CLI so that most of its implementation no longer
lives in a `main.go`, and rather in a subpackage that can be imported
from somewhere else. This lets the majority of it to be reused in
another executable and augmented in various ways.
An addition is a new `DriverProcurer` interface that can provide a
driver for various databases:
type DriverProcurer interface {
ProcurePgxV5(pool *pgxpool.Pool) riverdriver.Driver[pgx.Tx]
}
For the main CLI, this gets a trivial implementation using `riverpgxv5`,
but could potentially be reimplemented elsewhere to swap in something
else;
type DriverProcurer struct{}
func (p *DriverProcurer) ProcurePgxV5(pool *pgxpool.Pool) riverdriver.Driver[pgx.Tx] {
return riverpgxv5.New(pool)
}
To make this more workable, I end up going through and refactoring quite
a lot of code, making it into a bit of a mini framework, and one that
could potentially support additional databases in the future without
having to refactor the world again.
We also add a `--line` flag to support the feature from #435 to the CLI.
brandur
added a commit
that referenced
this pull request
Jul 14, 2024
Here, break out the CLI so that most of its implementation no longer
lives in a `main.go`, and rather in a subpackage that can be imported
from somewhere else. This lets the majority of it to be reused in
another executable and augmented in various ways.
An addition is a new `DriverProcurer` interface that can provide a
driver for various databases:
type DriverProcurer interface {
ProcurePgxV5(pool *pgxpool.Pool) riverdriver.Driver[pgx.Tx]
}
For the main CLI, this gets a trivial implementation using `riverpgxv5`,
but could potentially be reimplemented elsewhere to swap in something
else;
type DriverProcurer struct{}
func (p *DriverProcurer) ProcurePgxV5(pool *pgxpool.Pool) riverdriver.Driver[pgx.Tx] {
return riverpgxv5.New(pool)
}
To make this more workable, I end up going through and refactoring quite
a lot of code, making it into a bit of a mini framework, and one that
could potentially support additional databases in the future without
having to refactor the world again.
We also add a `--line` flag to support the feature from #435 to the CLI.
brandur
added a commit
that referenced
this pull request
Jul 14, 2024
Here, break out the CLI so that most of its implementation no longer
lives in a `main.go`, and rather in a subpackage that can be imported
from somewhere else. This lets the majority of it to be reused in
another executable and augmented in various ways.
An addition is a new `DriverProcurer` interface that can provide a
driver for various databases:
type DriverProcurer interface {
ProcurePgxV5(pool *pgxpool.Pool) riverdriver.Driver[pgx.Tx]
}
For the main CLI, this gets a trivial implementation using `riverpgxv5`,
but could potentially be reimplemented elsewhere to swap in something
else;
type DriverProcurer struct{}
func (p *DriverProcurer) ProcurePgxV5(pool *pgxpool.Pool) riverdriver.Driver[pgx.Tx] {
return riverpgxv5.New(pool)
}
To make this more workable, I end up going through and refactoring quite
a lot of code, making it into a bit of a mini framework, and one that
could potentially support additional databases in the future without
having to refactor the world again.
We also add a `--line` flag to support the feature from #435 to the CLI.
brandur
added a commit
that referenced
this pull request
Jul 14, 2024
Here, break out the CLI so that most of its implementation no longer
lives in a `main.go`, and rather in a subpackage that can be imported
from somewhere else. This lets the majority of it to be reused in
another executable and augmented in various ways.
An addition is a new `DriverProcurer` interface that can provide a
driver for various databases:
type DriverProcurer interface {
ProcurePgxV5(pool *pgxpool.Pool) riverdriver.Driver[pgx.Tx]
}
For the main CLI, this gets a trivial implementation using `riverpgxv5`,
but could potentially be reimplemented elsewhere to swap in something
else;
type DriverProcurer struct{}
func (p *DriverProcurer) ProcurePgxV5(pool *pgxpool.Pool) riverdriver.Driver[pgx.Tx] {
return riverpgxv5.New(pool)
}
To make this more workable, I end up going through and refactoring quite
a lot of code, making it into a bit of a mini framework, and one that
could potentially support additional databases in the future without
having to refactor the world again.
We also add a `--line` flag to support the feature from #435 to the CLI.
brandur
added a commit
that referenced
this pull request
Jul 14, 2024
Here, break out the CLI so that most of its implementation no longer
lives in a `main.go`, and rather in a subpackage that can be imported
from somewhere else. This lets the majority of it to be reused in
another executable and augmented in various ways.
An addition is a new `DriverProcurer` interface that can provide a
driver for various databases:
type DriverProcurer interface {
ProcurePgxV5(pool *pgxpool.Pool) riverdriver.Driver[pgx.Tx]
}
For the main CLI, this gets a trivial implementation using `riverpgxv5`,
but could potentially be reimplemented elsewhere to swap in something
else;
type DriverProcurer struct{}
func (p *DriverProcurer) ProcurePgxV5(pool *pgxpool.Pool) riverdriver.Driver[pgx.Tx] {
return riverpgxv5.New(pool)
}
To make this more workable, I end up going through and refactoring quite
a lot of code, making it into a bit of a mini framework, and one that
could potentially support additional databases in the future without
having to refactor the world again.
We also add a `--line` flag to support the feature from #435 to the CLI.
brandur
added a commit
that referenced
this pull request
Jul 17, 2024
Here, break out the CLI so that most of its implementation no longer
lives in a `main.go`, and rather in a subpackage that can be imported
from somewhere else. This lets the majority of it to be reused in
another executable and augmented in various ways.
An addition is a new `DriverProcurer` interface that can provide a
driver for various databases:
type DriverProcurer interface {
ProcurePgxV5(pool *pgxpool.Pool) riverdriver.Driver[pgx.Tx]
}
For the main CLI, this gets a trivial implementation using `riverpgxv5`,
but could potentially be reimplemented elsewhere to swap in something
else;
type DriverProcurer struct{}
func (p *DriverProcurer) ProcurePgxV5(pool *pgxpool.Pool) riverdriver.Driver[pgx.Tx] {
return riverpgxv5.New(pool)
}
To make this more workable, I end up going through and refactoring quite
a lot of code, making it into a bit of a mini framework, and one that
could potentially support additional databases in the future without
having to refactor the world again.
We also add a `--line` flag to support the feature from #435 to the CLI.
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
2 participants
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.
Here, add a new River migration (version 005) that brings a
linecolumn to the
river_migrationtable, allowing non-main lines to besupported. We also teach the migrator how to use it.
This one is a little trickier than it sounds (partly thanks to sqlc)
because the migrator needs to behave somewhat differently depending on
whether the
linecolumn exits yet. So for example, when migrating to004 it needs to upsert migration records that do not include a
linefield, but then when migrating to 005 or beyond, a
linevalue isincluded. The same consideration goes for performing a down migration,
or checking the existing state of migrations in the database.
This is implemented by pairing driver functions related to migrations
into one for pre-
lineand another for post-line. e.g.We don't yet pull line support into the CLI, but this should make
alternate lines fully supported up to that point. The CLI needs a little
more thought because it might involve building a separate binary.