sctest: Augmented BACKUP/RESTORE tests with table-level restore #87316
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
|
This change is |
ajwerner
approved these changes
Sep 7, 2022
There was a problem hiding this comment.
Reviewed 1 of 1 files at r1, 1 of 1 files at r2, all commit messages.
Reviewable status:complete! 1 of 0 LGTMs obtained (waiting on @Xiang-Gu)
pkg/sql/schemachanger/sctest/cumulative.go line 560 at r2 (raw file):
// reaches the expected state as if the back/restore had not happened at all. // Skip a backup randomly. type BackupConsumptionFlavor struct {
nit: don't export this type
pkg/sql/schemachanger/sctest/cumulative.go line 597 at r2 (raw file):
if len(tablesToRestore) > 0 { flavors = append(flavors, BackupConsumptionFlavor{ name: "restore all tables in database",
nit: add a TODO and/or file an issue to restore a subset?
Added comments around the Backup/Restore test for declarative schema changer, moved functions here and there, and added some new lines for better readability.
Xiang-Gu
commented
Sep 7, 2022
There was a problem hiding this comment.
Reviewable status:
pkg/sql/schemachanger/sctest/cumulative.go line 560 at r2 (raw file):
Previously, ajwerner wrote…
nit: don't export this type
done
pkg/sql/schemachanger/sctest/cumulative.go line 597 at r2 (raw file):
Previously, ajwerner wrote…
nit: add a TODO and/or file an issue to restore a subset?
done
Previously, the Backup test in declarative schema changer backups the
whole database and restore the whole database with `RESTORE DATABASE`.
This PR augments the test by adding another flavor to restore:
`RESTORE TABLE tbl1,...,tblN` where `tblx` are *all* the tables in the
backup. This will nicely give us coverage for `RESTORE TABLE` when
a declarative schema changer job is running.
Note that ideally we want to randomly restore only a subset of all the
table. Indeed I tried to implement that but realize it was blocked by
one limitation in the declarative shcema changer: We don't yet support
restore schema changer job that skips missing sequences (E.g. if we
have a table `t` and a sequence `s`, and I want to
`ALTER TABLE t ADD COLUMN c DEFAULT nextval('s')`, we backup database
in PostCommit phase. Later when we restore just `t`, the schema changer
job will run into error `error executing 'missing rewrite for id 109 in
<column_default_expression:<table_id:108 column_id:2
embedded_expr:<expr:"nextval(109:::REGCLASS)" uses_sequence_ids:109 >)`)
In other words, we have not implemented the equivalent of
`skip_missing_sequences`, which is an option for RESTORE,
for schema changer job.
Release justification: test-only changes
Release note: None
Xiang-Gu
force-pushed
the
add_table_level_backup_restore_test_for_new_schema_changer
branch
from
d405b27 to
f4b248e
Compare
|
TFTR! bors r+ |
|
Build succeeded: |
Xiang-Gu
deleted the
add_table_level_backup_restore_test_for_new_schema_changer
branch
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.
Commit 1: non-code minor changes (added comments, moved function a little bit)
Commit 2:
Previously, the Backup test in declarative schema changer backups the
whole database and restore the whole database with
RESTORE DATABASE.This PR augments the test by adding another flavor to restore:
RESTORE TABLE tbl1,...,tblNwheretblxare all the tables in thebackup. This will nicely give us coverage for
RESTORE TABLEwhena declarative schema changer job is restored.
Note that ideally we want to randomly restore only a subset of all the
table. Indeed I tried to implement that but realize it was blocked by
one limitation in the declarative shcema changer: We don't yet support
restore schema changer job that skips missing sequences (E.g. if we
have a table
tand a sequences, and I want toALTER TABLE t ADD COLUMN c DEFAULT nextval('s'), we backup databasein PostCommit phase. Later when we restore just
t, the schema changerjob will run into error
error executing 'missing rewrite for id 109 in <column_default_expression:<table_id:108 column_id:2 embedded_expr:<expr:"nextval(109:::REGCLASS)" uses_sequence_ids:109 >))This issue is tracked in #87518.
Fixes: #86835
Release justification: test-only changes
Release note: None