core: Ignore indexed-only constraints when updating
#734
Conversation
This file contains 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
kim
force-pushed
the
kim/ignore-indexed
branch
from
17197d1
to
974f751
Compare
mamcx
reviewed
Jan 30, 2024
crates/core/src/db/update.rs
Outdated
| /// corresponding [`Constraints::indexed()`] constraint (it will be unset). | ||
| /// The schema obtained from the database **will** have one such constraint, | ||
| /// however. | ||
| /// |
There was a problem hiding this comment.
This is because the conversion into TableSchema fills that info.
TableDef is meant to be a partially defined schema that is resolved/qualified when converted to TableSchema. See TableSchema::from_def
There was a problem hiding this comment.
Are you suggesting to compare TableSchema instead?
There was a problem hiding this comment.
The problem with this is that what the module produces is not a TableSchema. The module only specifies a TableDef, so by some mechanism we need to compare that to the existing schema (or at least the old def) to find out what changed. You'd have to create a fake TableSchema for the new module to compare against.
There was a problem hiding this comment.
The real problem is that we're not being very careful about what is actual database schema (the thing which requires data migration if it changes) and our various attributes (autoinc) or optimizations (indexes).
Indexes are not really part of the schema of a table in the precise meaning of the term schema, neither are attributes.
bfops
added a commit
that referenced
this pull request
Feb 5, 2024
bfops
added
bugfix
cloutiertyler
approved these changes
Feb 13, 2024
kim
force-pushed
the
kim/ignore-indexed
branch
from
974f751
to
f93f8c2
Compare
| index_id: IndexId(68), | ||
| table_id: TableId(42), | ||
| index_type: IndexType::BTree, | ||
| index_name: "bobson_dugnutt".into(), |
cloutiertyler
approved these changes
Feb 16, 2024
Compare `TableSchema` instead of `TableDef`, which should reject all modifications to a table. Note that the values require conversion for both the module and database schema to be comparable. With this change, updating a module which only adds tables or modifies reducers will be possible, but any change (including indexes, constraints, etc) to an existing table will fail the update.
kim
force-pushed
the
kim/ignore-indexed
branch
from
f93f8c2
to
0780c0b
Compare
kim
added a commit
that referenced
this pull request
Mar 14, 2024
It turns out that the changes introduced in #734 do not result in more reliable detection of incompatible schema updates. This is because the datastructures involved can be converted into each other, but that conversion is not bijective. Fix this by manually adjusting the schema of the existing table to be comparable to the proposed table. Also log details about a schema mismatch to the user-retrievable database log, in unified diff format.
kim
added a commit
that referenced
this pull request
Mar 15, 2024
It turns out that the changes introduced in #734 do not result in more reliable detection of incompatible schema updates. This is because the datastructures involved can be converted into each other, but that conversion is not bijective. Fix this by manually adjusting the schema of the existing table to be comparable to the proposed table. Also log details about a schema mismatch to the user-retrievable database log, in unified diff format.
github-merge-queue bot
pushed a commit
that referenced
this pull request
Mar 15, 2024
It turns out that the changes introduced in #734 do not result in more reliable detection of incompatible schema updates. This is because the datastructures involved can be converted into each other, but that conversion is not bijective. Fix this by manually adjusting the schema of the existing table to be comparable to the proposed table. Also log details about a schema mismatch to the user-retrievable database log, in unified diff format.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
As explained in the commentary.
This will allow updating a module which introduces an index via
#[spacetimedb(index, ..)], but it will not create that index.