Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

backupccl: add SHOW BACKUPS IN Y and SHOW BACKUP x IN Y #52758

Merged
merged 1 commit into from
Aug 19, 2020

Conversation

dt
Copy link
Member

@dt dt commented Aug 13, 2020

This change teaches SHOW BACKUP about collections, specifically adding
support for listing the backups in a collection with SHOW BACKUPS IN dest
and for showing an individual backup with dest using SHOW BACKUP x IN dest.
The former is a thin wrapper around 'ls' on the destination while the latter
is an even thinner wrapper around 'SHOW BACKUP dest/x', but both should make
it easier to interact with backups in collections, which often have long and
cumbersome URIs (including keys), so the more we can keep the collection URI
constant and let you just specify which element in the collection you want
to SHOW, RESTORE, etc, the easier it will be.

Release note (enterprise change): SHOW BACKUPS can be used to list backups in a backup collection created by BACKUP INTO.

@dt dt requested review from pbardea and a team August 13, 2020 04:12
@cockroach-teamcity
Copy link
Member

This change is Reviewable

@dt dt marked this pull request as draft August 13, 2020 04:13
@dt
Copy link
Member Author

dt commented Aug 13, 2020

(still needs a test)

Copy link
Contributor

@miretskiy miretskiy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a very nice change. Thanks @dt.
It :lgtm: , but we probably do need some tests for this.

Reviewed 5 of 6 files at r1.
Reviewable status: :shipit: complete! 1 of 0 LGTMs obtained (waiting on @dt and @pbardea)


pkg/ccl/backupccl/show.go, line 415 at r1 (raw file):

) (sql.PlanHookRowFn, sqlbase.ResultColumns, []sql.PlanNode, bool, error) {

	toFn, err := p.TypeAsString(ctx, backup.InCollection, "SHOW BACKUPS")

nit: maybe different name for 'toFn' (fromFn? collectionFn?)


pkg/ccl/backupccl/show.go, line 439 at r1 (raw file):

		}
		for _, i := range res {
			resultsCh <- tree.Datums{tree.NewDString(strings.TrimSuffix(i, "/BACKUP"))}

do we want to emit more information (start/end time? Might not be possible if we have to read data)... At the same time, if collection has a lot of directories, it might be hard to compose subselect to find matching backups.)


pkg/sql/parser/parse_test.go, line 1511 at r1 (raw file):

		{`SHOW BACKUPS IN $1`},
		{`SHOW BACKUP 'foo' IN 'bar'`},
		{`SHOW BACKUP $1 IN $2 WITH foo = 'bar'`},

i guess we haven't switched show backup to use explicit backup options...

Copy link
Member Author

@dt dt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: :shipit: complete! 1 of 0 LGTMs obtained (waiting on @dt, @miretskiy, and @pbardea)


pkg/ccl/backupccl/show.go, line 439 at r1 (raw file):

Previously, miretskiy (Yevgeniy Miretskiy) wrote…

do we want to emit more information (start/end time? Might not be possible if we have to read data)... At the same time, if collection has a lot of directories, it might be hard to compose subselect to find matching backups.)

this was already discussed at length: https://cockroachlabs.slack.com/archives/C2C5FKPPB/p1597260345017000?thread_ts=1597259351.010600&cid=C2C5FKPPB

TL;DR: SHOW BACKUPS will just show you what backups you have and you can use SHOW BACKUP x IN dst to then find out what is in that backup (start/end time, revisions, tables, etc). If asked SHOW BACKUPS to read every manifest, 365 days * 24 hour * 4 backups an hour = 35k manifests to read. Nope. History file can provide a history of what you backed up where and include lots of detail about it, but SHOW should be a source-of-truth of what is actually in the destination bucket based on reading it, and thus will stick to what it can afford to actually derive, which is just the list of things that you can go query for more details.

@dt dt force-pushed the show-backups branch 2 times, most recently from fabb19d to 5ffaec8 Compare August 13, 2020 20:55
@dt dt marked this pull request as ready for review August 13, 2020 20:56
Copy link
Contributor

@pbardea pbardea left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:lgtm:

Reviewable status: :shipit: complete! 1 of 0 LGTMs obtained (and 1 stale) (waiting on @dt, @miretskiy, and @pbardea)


pkg/ccl/backupccl/show_test.go, line 310 at r2 (raw file):

	sqlDB.Exec(t, `BACKUP data.bank INTO LATEST IN $1`, full)
	sqlDB.Exec(t, `BACKUP data.bank INTO LATEST IN $1`, full)
	// Make a third full backup, add changes to it twice.

nit: it looks like we're only doing 1 inc backup here? Doesn't particularly matter, but would be nice to clean up the comment

This change teaches SHOW BACKUP about collections, specifically adding
support for listing the backups in a collection with SHOW BACKUPS IN dest
and for showing an individual backup with dest using SHOW BACKUP x IN dest.
The former is a thin wrapper around 'ls' on the destination while the latter
is an even thinner wrapper around 'SHOW BACKUP dest/x', but both should make
it easier to interact with backups in collections, which often have long and
cumbersome URIs (including keys), so the more we can keep the collection URI
constant and let you just specify which element in the collection you want
to SHOW, RESTORE, etc, the easier it will be.

Release note (enterprise change): SHOW BACKUPS can be used to list backups in a backup collection created by BACKUP INTO.
@dt
Copy link
Member Author

dt commented Aug 19, 2020

bors r+

@craig
Copy link
Contributor

craig bot commented Aug 19, 2020

Build succeeded:

@craig craig bot merged commit 935ae2e into cockroachdb:master Aug 19, 2020
@dt dt deleted the show-backups branch August 19, 2020 21:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants