Fix index removal and additions + add smoketest #1444
Merged
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
1. Add constraints when adding an index. 2. Make indexes more transactional. 3. Make replay behave wrt. schema changes.
Centril
changed the title
Centril
force-pushed
the
centril/fix-index-add-remove
branch
from
1645e6f
to
44df346
Compare
Centril
commented
Jun 18, 2024
Comment on lines
+358
to
+364
| // NOTE: Also add all the rows in the already committed table to the index. | ||
| // FIXME: Is this correct? Index scan iterators (incl. the existing `Locking` versions) | ||
| // appear to assume that a table's index refers only to rows within that table, | ||
| // and does not handle the case where a `TxState` index refers to `CommittedState` rows. | ||
| if let Some(committed_table) = commit_table { | ||
| insert_index.build_from_rows(&index.columns, committed_table.scan_rows(commit_blob_store))?; | ||
| } |
There was a problem hiding this comment.
This seems wrong to me, but currently a pre-commit test fails without it.
The logic is pre-existing and should ostensibly be replaced by doing some on-merge logic, rendering merges fallible.
There was a problem hiding this comment.
AFAICT, the idea here is that the same TX that creates the index may use it, even though it hasn't been added to the committed state yet, so the transient version in the insert state needs to also contain the committed state's rows. There are other, better ways we could accomplish the same thing, or we could just disallow doing create_index in arbitrary TXes, but that's the impetus behind the current behavior.
Centril
force-pushed
the
centril/fix-index-add-remove
branch
from
4f87b14
to
61e2fe0
Compare
Centril
force-pushed
the
centril/fix-index-add-remove
branch
from
61e2fe0
to
f799f30
Compare
kim
approved these changes
Jun 19, 2024
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.
Description of Changes
Fixes #1438.
API and ABI breaking changes
None
Expected complexity level and risk
2
Testing
call/ WARN test still works