sqlccl: handle descriptor changes in MVCC BACKUP/RESTORE

[dt]

Release note (enterprise change): experimental ‘revision_history’ backup/restore handles schema changes.

[cockroach-teamcity]

This change is 

[maddyblue]

Review status: 0 of 9 files reviewed at latest revision, 6 unresolved discussions, all commit checks successful.

---

pkg/ccl/sqlccl/backup.go, line 125 at r1 (raw file):

}

// getRelventDescChanges find the changes between start and end time to the SQL

finds

---

pkg/ccl/sqlccl/backup.go, line 128 at r1 (raw file):

// descriptors matching `descs` or `expandedDBs`, ordered by time. A descriptor
// revision matches if it is an earlier revision of a descriptor in descs (same
// ID) or has a a parentID in `expanded`. Deleted descriptors are represented as

"a a"

---

pkg/ccl/sqlccl/backup.go, line 130 at r1 (raw file):

// ID) or has a a parentID in `expanded`. Deleted descriptors are represented as
// nil.
func getRelventDescChanges(

Relvent is misspelled

---

pkg/ccl/sqlccl/backup.go, line 143 at r1 (raw file):

	}

	// If not descriptors changed, we can just stop now and have RESTORE use the

not -> no

---

pkg/ccl/sqlccl/backup.go, line 192 at r1 (raw file):

	for _, change := range allChanges {
		if _, ok := interestingIDs[change.ID]; ok {

Why don't need to care about interestingParents here too?

---

pkg/ccl/sqlccl/restore.go, line 84 at r1 (raw file):

	for _, b := range backupDescs {
		if asOf.Less(b.StartTime) {

In the case where asOf is less than the first b.StartTime, this will set lastBackupDesc to last descriptor in backupDescs from 5 lines above. Is that correct?

---

Comments from Reviewable

[dt]

Review status: 0 of 9 files reviewed at latest revision, 6 unresolved discussions, some commit checks pending.

---

pkg/ccl/sqlccl/backup.go, line 125 at r1 (raw file):

Previously, mjibson (Matt Jibson) wrote…

finds

Done.

---

pkg/ccl/sqlccl/backup.go, line 128 at r1 (raw file):

Previously, mjibson (Matt Jibson) wrote…

"a a"

Done

---

pkg/ccl/sqlccl/backup.go, line 130 at r1 (raw file):

Previously, mjibson (Matt Jibson) wrote…

Relvent is misspelled

Done.

---

pkg/ccl/sqlccl/backup.go, line 143 at r1 (raw file):

Previously, mjibson (Matt Jibson) wrote…

not -> no

Done.

---

pkg/ccl/sqlccl/backup.go, line 192 at r1 (raw file):

Previously, mjibson (Matt Jibson) wrote…

Why don't need to care about interestingParents here too?

yep, nice catch -- I meant to do this and forgot once I actually wrote it up.

While we've already used interestingParents above to seed interestingIDs with any tables that were in an interesting DB at the beginning of the interval -- and have added all tables in the matched set at the end of the window too -- if a table is moved into the interesting parents during the window, we need to note that as well -- not only is that and interesting change, but we need to then expand interestingIDs in case it is also deleted. Also added more comments to this effect.

---

pkg/ccl/sqlccl/restore.go, line 84 at r1 (raw file):

Previously, mjibson (Matt Jibson) wrote…

In the case where asOf is less than the first b.StartTime, this will set lastBackupDesc to last descriptor in backupDescs from 5 lines above. Is that correct?

That is not correct, but we have a check at the very beginning of RESTORE ... AS OF SYSTEM TIME that we have an MVCC backup covering that time.

---

Comments from Reviewable

[dt]

Okay, added tests with adding and drop tables and indexes, truncating and moving a table in and out of the target DB, RFAL.

The main missing piece left is non-DB backups, where we'll need #21895, but I don't want to block on that -- as discussed with @dianasaur323, the BACKUP DATABASE + RESTORE DATABASE workflow is our top priority, and that doesn't depend on #21895 since the DB of both the pre- and post-truncate table is the same.

[maddyblue]

I think? This stuff is complicated.

---

Review status: 0 of 9 files reviewed at latest revision, 7 unresolved discussions, all commit checks successful.

---

pkg/ccl/sqlccl/backup.go, line 354 at r2 (raw file):

	}
	// if there are desc revisions, ensure that we also add any index spans
	// in them that we didn't already get above.

Can you list the conditions where this happens in the comment?

---

pkg/ccl/sqlccl/restore.go, line 110 at r2 (raw file):

	for _, desc := range byID {
		if t := desc.GetTable(); t != nil {
			// A table revsions may have been captured before it was in a DB that is

revsions

---

Comments from Reviewable

[dt]

Review status: 0 of 9 files reviewed at latest revision, 7 unresolved discussions.

---

pkg/ccl/sqlccl/backup.go, line 354 at r2 (raw file):

Previously, mjibson (Matt Jibson) wrote…

Can you list the conditions where this happens in the comment?

Done.

---

pkg/ccl/sqlccl/restore.go, line 110 at r2 (raw file):

Previously, mjibson (Matt Jibson) wrote…

revsions

Done.

---

Comments from Reviewable