Skip to content
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

Organize DB Schema Migration Code and DB Init Checks #842

Merged
merged 34 commits into from
Jan 19, 2021

Conversation

talanknight
Copy link
Contributor

@talanknight talanknight commented Dec 22, 2020

Reorganizes the code which applies schema changes in Boundary and sets up the ability (in future PRs) to migrate to newer versions of the database schema when needed.

Added checks when boundary server -config starts a controller to ensure that the database was initialized correctly. Also checks in boundary database init -config to ensure the database is in a state that can be initialized.

Fixes #805

@talanknight talanknight marked this pull request as ready for review January 5, 2021 19:11
…command is created. Replaced additional context.TODOs with context.Backgrounds.
jimlambrt
jimlambrt previously approved these changes Jan 15, 2021
Copy link
Collaborator

@jimlambrt jimlambrt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks great.

Copy link
Member

@mgaffney mgaffney left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall it looks great! Just a few questions and suggestions.

internal/db/schema/postgres_migration.gen.go Show resolved Hide resolved
internal/db/schema/schema.go Outdated Show resolved Hide resolved
internal/db/schema/schema.go Outdated Show resolved Hide resolved
return false, err
}
if st.Dirty {
return false, fmt.Errorf("The passed in database has had a failed migration applied to it.")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have the same question

internal/db/schema/postgres/postgres.go Outdated Show resolved Hide resolved
internal/db/schema/postgres/testing.go Outdated Show resolved Hide resolved
internal/db/schema/manager.go Outdated Show resolved Hide resolved
internal/db/schema/manager.go Outdated Show resolved Hide resolved
internal/db/schema/manager.go Outdated Show resolved Hide resolved
internal/docker/supported.go Show resolved Hide resolved
mgaffney
mgaffney previously approved these changes Jan 16, 2021
Copy link
Member

@mgaffney mgaffney left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great job!

internal/db/schema/manager.go Show resolved Hide resolved
// https://www.postgresql.org/docs/9.6/static/explicit-locking.html#ADVISORY-LOCKS
func (p *Postgres) TrySharedLock(ctx context.Context) error {
const op = "postgres.(Postgres).TrySharedLock"
r := p.conn.QueryRowContext(ctx, "SELECT pg_try_advisory_lock_shared($1)", schemaAccessLockId)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Previous comments are not showing up for some reason.

Me:

Why is there no equivalent unlock method for this (select pg_advisory_unlock_shared($1))?

Todd:

Just because we do not have a use for it yet.

Shouldn't we be releasing the lock by calling select pg_advisory_unlock_shared($1)? Or are we just letting Postgres do the cleanup when the connection closes? If it is the latter, are there any dangers on relying on that?

jimlambrt
jimlambrt previously approved these changes Jan 19, 2021
Copy link
Collaborator

@jimlambrt jimlambrt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great.

@talanknight talanknight dismissed stale reviews from jimlambrt and mgaffney via 8af94f1 January 19, 2021 15:30
@talanknight talanknight merged commit ec6151d into main Jan 19, 2021
@talanknight talanknight deleted the toddknight-schema-management branch January 19, 2021 17:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Migrations fail without PgCrypto and Postgres permissions but Boundary doesn't know
4 participants