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

tapdb: Federation DB fixes #583

Merged
merged 1 commit into from
Oct 17, 2023
Merged

tapdb: Federation DB fixes #583

merged 1 commit into from
Oct 17, 2023

Conversation

jharveyb
Copy link
Collaborator

Fixes multiple issues with sync config load and store.

The first issue was the federation DB handle not returning the correct global config when the global config is modified for one proof type - this is resolved by loading the default config, and keying configs by proof type. Configs read from the DB then replace the default config with the matching proof type.

The second issue affected grouped assets, and led to group member asset IDs not being shown on the CLI.

The third issue was the UNIQUE constraint on universe sync configs not being enforced, since NULL values are distinct in UNIQUE constraints by default for SQLite and Postgres. This could lead to a bad configuration state with multiple entries for the same universe. This behavior is not configurable for SQLite, so extra columns were added that have a default value when one field of the universe ID is NULL. This should fix the DB constraint.

tapdb/universe_federation.go Outdated Show resolved Hide resolved
tapdb/universe_federation.go Outdated Show resolved Hide resolved
tapdb/multiverse.go Outdated Show resolved Hide resolved
tapdb/sqlc/migrations/000007_universe.up.sql Outdated Show resolved Hide resolved
@jharveyb
Copy link
Collaborator Author

After offline discussion, new plan is to actually not change the table here but instead just update the queries to insert a default values of all 0's when the underlying type is nil. This is sort of equivalent to the generated columns in making sure the UNIQUE constraint is active.

We can also add a migration to DROP any contents in this table. Since no deployment should have a local Universe config set, including the Universe servers we run, this should not be an issue.

@Roasbeef
Copy link
Member

Needs a rebase.

We can also add a migration to DROP any contents in this table. Since no deployment should have a local Universe config set, including the Universe servers we run, this should not be an issue.

I think this could work. Drop the old, then add on new.

Copy link
Member

@Roasbeef Roasbeef left a comment

Choose a reason for hiding this comment

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

LGTM 🛩️

Changes look good, just need to verify the upgrade path works out like we think it does (old config wiped, new one in place, everything works from there).

@jharveyb
Copy link
Collaborator Author

Verified locally on mainnet setup with the original issue that old local configs were dropped and upsert now works via CLI.

Copy link
Member

@guggero guggero left a comment

Choose a reason for hiding this comment

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

Approach looks good! Should replace the TODO with some SQL magic, then this is good to go!

tapdb/universe_federation.go Outdated Show resolved Hide resolved
Copy link
Member

@guggero guggero left a comment

Choose a reason for hiding this comment

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

Nice 💯

In this commit, we fix an issue with universe sync config storage. With
the old table schema, multiples entries could exist for the same
Universe because the UNIQUE constraint did not function as intended.
This was caused by NULL entries being treated as unique values vs. one
value. The new table has one entry per Universe ID, which fixes this.
@Roasbeef Roasbeef merged commit 75a67e5 into main Oct 17, 2023
14 checks passed
@guggero guggero deleted the federation_db_fixes branch October 17, 2023 17:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants