backupccl: various user defined types fixes and tests#52033
backupccl: various user defined types fixes and tests#52033craig[bot] merged 1 commit intocockroachdb:masterfrom
Conversation
|
This change is |
222a2d3 to
c693caa
Compare
pbardea
left a comment
There was a problem hiding this comment.
LGTM - just left a few comments in the tests.
Reviewed 5 of 8 files at r1.
Reviewable status:complete! 0 of 0 LGTMs obtained (waiting on @dt, @pbardea, and @rohany)
pkg/ccl/backupccl/backup_test.go, line 1300 at r1 (raw file):
defer log.Scope(t).Close(t) // TODO (rohany): Add tests for backup/restore with revision history and
Should we keep the revision history part of this TODO here until those tests are added as well?
pkg/ccl/backupccl/backup_test.go, line 1528 at r1 (raw file):
sqlDB.Exec(t, `INSERT INTO d2.t2 VALUES (ARRAY['hi'::d2.greeting])`) // d.t and d.t2 should both have back references to d2.greeting.
The tables that have the backreference here are d2.t and d2.t2, right?
pkg/ccl/backupccl/backup_test.go, line 1582 at r1 (raw file):
sqlDB.Exec(t, `RESTORE TABLE d.t FROM 'nodelocal://0/test/' WITH into_db = 'd7'`) sqlDB.Exec(t, `INSERT INTO d7.t VALUES ('greetings')`) sqlDB.CheckQueryResults(t, `SELECT * FROM d7.t ORDER BY x`, [][]string{{"hello"}, {"greetings"}, {"howdy"}})
Might also be worth adding the backreference check here as well?
pkg/ccl/backupccl/restore_job.go, line 1053 at r1 (raw file):
for _, id := range typeIDs { // If the used type is part of the set of descriptors being restored, // then don't do anything.
nit: can we change "don't do anything" to something like "there's no need to add a backreference, since it already should exist".
This commit fixes some bugs introduced with the ability to change and drop types, and adds some more tests. In particular it * Ensures that backup+restore maintain type backreferences to tables that use them, as well as ensuring that backups that use existing types get those backreferences installed on the existing descriptors. * Adds a test to restore to an existing type that is a logical "superset" of an existing type. * Adds test to ensure created and dropped types are reflected in incremenetal backups and restore of those backups. Fixes cockroachdb#50321. Fixes cockroachdb#51787. Release note: None
c693caa to
fc3c078
Compare
rohany
left a comment
There was a problem hiding this comment.
Reviewable status:
pkg/ccl/backupccl/backup_test.go, line 1300 at r1 (raw file):
Previously, pbardea (Paul Bardea) wrote…
Should we keep the revision history part of this TODO here until those tests are added as well?
woops, got a little trigger happy
pkg/ccl/backupccl/backup_test.go, line 1528 at r1 (raw file):
Previously, pbardea (Paul Bardea) wrote…
The tables that have the backreference here are d2.t and d2.t2, right?
Yup, updated.
pkg/ccl/backupccl/backup_test.go, line 1582 at r1 (raw file):
Previously, pbardea (Paul Bardea) wrote…
Might also be worth adding the backreference check here as well?
Done
pkg/ccl/backupccl/restore_job.go, line 1053 at r1 (raw file):
Previously, pbardea (Paul Bardea) wrote…
nit: can we change "don't do anything" to something like "there's no need to add a backreference, since it already should exist".
Done
|
TFTR! bors r+ |
|
Build succeeded: |
This commit fixes some bugs introduced with the ability to change and
drop types, and adds some more tests. In particular it
that use them, as well as ensuring that backups that use existing
types get those backreferences installed on the existing descriptors.
"superset" of an existing type.
incremenetal backups and restore of those backups.
Fixes #50321.
Fixes #51787.
Release note: None