Temporary fix for DropTable/CreateTable issue around temporal tables #35225
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
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.
@maumar this is a bug in #32239, which I think is the result of a wider design issue in that PR; the trigger is simply having a DropTable followed by a CreateTable in the same migration. Your second processing loop for temporal tables (code) removes the temporal data from the table when it sees the DropTable, and when we get to the CreateTable we get an exception since there's no data at that point (code).
The hacky fix proposed here - which is probably OK for patching - is to simply not remove the data when we see DropTable, ensuring that it's still there when the CreateTable is processed. We've got two user reports on #35162; am waiting to hear how they got to having DropTable/CreateTable in the same migration in order to better understand how critical this bug is.
However, I think the design here is fundamentally problematic; you can't really do a pass over all operations (and then over the model for missing ones), storing the temporal data in a single dictionary; a single migration can create and drop the same table multiple times, each time with completely different temporal data. The current design flattens all that out, so that when you process e.g. the 1st CreateTable, you actually see the temporal data from the last CreateTable. IMHO the correct design here would be to have a single pass, extracting the temporal data when you see e.g. CreateTable, and remember it in the dictionary. If you have reach a table operation which doesn't contain temporal data, and don't have data in the dictionary (no e.g. CreateTable was present in the migration before), at that point you can consult the model, and again store the data in the dictionary. This way you always have the correct, up-to-date temporal data in the dictionary, as you're advancing in a single pass through all migration operations.
Fixes #35162