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: fix error during span-merging optimization #66840

Merged
merged 1 commit into from
Jun 24, 2021

Conversation

pbardea
Copy link
Contributor

@pbardea pbardea commented Jun 24, 2021

This commit fixes a bug where a backup may crash if a full backup with
revision history is taken on a table that has index-merging
opportunities on descriptor revisions that lie before the GC TTL of the
table.

Since the merging of index-spans during backup planning is entirely an
optimization (in an effort to reduce the number of spans we place a
protectedts over), it is safe to ignore such errors and treat it as
un-mergable.

Fixes #66797.

Release note (bug fix): Fix an error that backup would produce in some
full backups with revision history. Previously some full backups would
erroneously produce an error in the form of batch timestamp <some_timestamp> must be after replica GC threshold <some_other_timestamp>.

@pbardea pbardea requested review from adityamaru and a team June 24, 2021 14:08
@cockroach-teamcity
Copy link
Member

This change is Reviewable

@pbardea pbardea removed the request for review from a team June 24, 2021 14:09
@pbardea pbardea force-pushed the batch-timestamp-err branch 2 times, most recently from 6b37e41 to 030c3f7 Compare June 24, 2021 14:11
Copy link
Contributor

@adityamaru adityamaru 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! 0 of 0 LGTMs obtained (waiting on @pbardea)


pkg/ccl/backupccl/backup_planning.go, line 366 at r1 (raw file):

		if rawTbl != nil && rawTbl.Public() {
			tbl := tabledesc.NewBuilder(rawTbl).BuildImmutableTable()
			// endTime is safe to pass in here because if the scan at endTime is different than

i'm a little confused by this comment since we are still passing in rev.Time.


pkg/ccl/backupccl/backup_test.go, line 8424 at r1 (raw file):

// doesn't produce an error when there are span-merging opportunities on
// descriptor revisions from before the GC threshold of the table.
func TestSpanMergeingBeforeGCThreshold(t *testing.T) {

really nice test


pkg/ccl/backupccl/backup_test.go, line 8439 at r1 (raw file):

	/*
	Produce a table with the following indexes over time:
				|

the formatting looks a little off in Reviewable.

@pbardea pbardea force-pushed the batch-timestamp-err branch from 030c3f7 to f70cc89 Compare June 24, 2021 14:36
Copy link
Contributor Author

@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.

Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (waiting on @adityamaru and @pbardea)


pkg/ccl/backupccl/backup_planning.go, line 366 at r1 (raw file):

Previously, adityamaru (Aditya Maru) wrote…

i'm a little confused by this comment since we are still passing in rev.Time.

Thanks for catching... this was leftover from some prototyping.


pkg/ccl/backupccl/backup_test.go, line 8439 at r1 (raw file):

Previously, adityamaru (Aditya Maru) wrote…

the formatting looks a little off in Reviewable.

Played around with it and I think this did the trick...

@pbardea pbardea requested a review from adityamaru June 24, 2021 14:37
This commit fixes a bug where a backup may crash if a full backup with
revision history is taken on a table that has index-merging
opportunities on descriptor revisions that lie before the GC TTL of the
table.

Since the merging of index-spans during backup planning is entirely an
optimization (in an effort to reduce the number of spans we place a
protectedts over), it is safe to ignore such errors and treat it as
un-mergable.

Release note (bug fix): Fix an error that backup would produce in some
full backups with revision history. Previously some full backups would
erroneously produce an error in the form of `batch timestamp
<some_timestamp> must be after replica GC threshold
<some_other_timestamp>`.
@pbardea pbardea force-pushed the batch-timestamp-err branch from f70cc89 to 622722c Compare June 24, 2021 18:22
@pbardea
Copy link
Contributor Author

pbardea commented Jun 24, 2021

Played around with the formatting of the comment a bit more to make the linter happy. I think all editors/github/reviewable should all see the same thing now though.
TFTR!
bors r+

@craig
Copy link
Contributor

craig bot commented Jun 24, 2021

Build succeeded:

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.

backupccl: revision_history full backups can fail when attempting to merge spans beyond table GC TTL
3 participants